Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require JDK 17 rather than JDK 11 #603

Merged
merged 22 commits into from
Feb 11, 2024
Merged

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Apr 27, 2023

This PR is on top of #604. ❗ This now targets master.

This PR is spun off from #198. The idea is that it is merged first; the scope of #198 is then reduced to text block support for CompilationTestHelper and BugCheckerRefactoringTestHelper.

❗ A lot of discussion in this PR was around use of Jabel. This turned out to cause flaky GitHub Actions builds, so eventually we decided to simply raise the project's baseline to JDK 17. This was the now obsolete suggested commit message:

Use Jabel to enable use of JDK >11 constructs while still targeting JDK 11 (#603)

The build now effectively requires JDK 17, though continued compatibility with
JDK 11 is validated by means of Surefire tests that use Maven Toolchains 
support to run JDK 11. 

Through Jabel, only a subset of new language features can be made compatible
with earlier versions of Java, so not all JDK 17 functionality can be used yet.
Similarly, APIs introduced after JDK 11 can also not be used yet.

Constructs that can now be used include text blocks, switch expressions and 
`instanceof` pattern matching. The code has been updated to make use of these
new constructs.

While there, make sure that the `sonar` Maven profile is picked up during
SonarCloud analysis.

See:
- https://github.com/bsideup/jabel
- https://maven.apache.org/guides/mini/guide-using-toolchains.html

New suggested commit message:

Require JDK 17 rather than JDK 11 (#603)

By raising this baseline the project can now use Java 17 language features such
as text blocks, switch expressions and `instanceof` pattern matching. The code
has been updated to make use of these constructs.

Note that the project can still be used by builds that target an older version
of Java, as long as those builds are executed using JDK 17+.

@Stephan202 Stephan202 added this to the 0.10.0 milestone Apr 27, 2023
@Stephan202 Stephan202 requested a review from rickie April 27, 2023 17:03
.github/workflows/build-jdk11.yaml Fixed Show fixed Hide fixed
.github/workflows/build-jdk11.yaml Fixed Show fixed Hide fixed
@Stephan202
Copy link
Member Author

Open points (will circle back to this PR over the coming days):

  1. A closer look at the JDK 20 compilation error; likely something due to Jabel.
  2. Addressing the SonarCloud-reported issues.
  3. Reviewing whether the Pitest-reported missing coverage is accurate/matches coverage on the target branch.

@Stephan202
Copy link
Member Author

I filed bsideup/jabel#177 for the JDK 20 build error. (We should also update the GitHub Actions workflow to use the latest JDK 20 release, but I'll open a separate PR for that.)

@Stephan202
Copy link
Member Author

but I'll open a separate PR for that.

Filed #604.

@Stephan202 Stephan202 force-pushed the sschroevers/require-jdk-17 branch from 9c61a7d to aa78360 Compare April 28, 2023 15:03
@Stephan202 Stephan202 changed the base branch from master to sschroevers/update-github-actions-jdks April 28, 2023 15:03
@Stephan202
Copy link
Member Author

Rebased PR on top of #604.

@Stephan202 Stephan202 force-pushed the sschroevers/require-jdk-17 branch 2 times, most recently from eeae7c2 to c52841e Compare April 28, 2023 16:48
@Stephan202
Copy link
Member Author

Updated the branch. The SonarCloud alerts have been resolved. I verified that all surviving mutations reported by Pitest are also present on the target branch; resolving those is out of scope.

The only open point is the Jabel JDK 20 incompatibility; let's wait for a few days to see whether we can get a new release for that. Alternatively we can temporarily disable the JDK 20 build.

@PicnicSupermarket PicnicSupermarket deleted a comment from github-actions bot Apr 28, 2023
@PicnicSupermarket PicnicSupermarket deleted a comment from github-actions bot Apr 28, 2023
@PicnicSupermarket PicnicSupermarket deleted a comment from github-actions bot Apr 28, 2023
@PicnicSupermarket PicnicSupermarket deleted a comment from github-actions bot Apr 28, 2023
@PicnicSupermarket PicnicSupermarket deleted a comment from github-actions bot Apr 28, 2023
@rickie rickie force-pushed the sschroevers/update-github-actions-jdks branch 2 times, most recently from 921801d to 7b9ef35 Compare May 2, 2023 06:02
Base automatically changed from sschroevers/update-github-actions-jdks to master May 2, 2023 06:51
@rickie rickie force-pushed the sschroevers/require-jdk-17 branch from c52841e to 0ccbf97 Compare May 2, 2023 08:18
@rickie rickie requested a review from werli May 2, 2023 11:04
@rickie rickie modified the milestones: 0.10.0, 0.11.0 May 4, 2023
@Stephan202 Stephan202 removed this from the 0.11.0 milestone May 14, 2023
@Stephan202 Stephan202 force-pushed the sschroevers/require-jdk-17 branch from 7fdbd0d to 6ed8caf Compare February 11, 2024 15:45
Copy link

Copy link
Member Author

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased and added two more small commits, introducing a few more text blocks (and thus resolving some more SonarCloud violations).

I'll merge once built, so that I can start rebasing other PRs. (@rickie I realize that this skips a review opportunity; happy to open follow-up PRs.)

Comment on lines -279 to +282
String.format(
"Rule `%s` matches on line %s, while it should match in a method named `test%s`.",
e.getValue(), e.getKey(), e.getValue()))
"""
Rule `%s` matches on line %s, while it should match in a method named \
`test%s`."""
.formatted(e.getValue(), e.getKey(), e.getValue()))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use .formatted elsewhere, but i.c.w. text blocks it makes sense. Let's review whether we want to switch all String.format usages.

Copy link

  • Surviving mutants in this change: 11
  • Killed mutants in this change: 172
class surviving killed
🧟tech.picnic.errorprone.utils.AnnotationAttributeMatcher 3 0
🧟tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage 2 17
🧟tech.picnic.errorprone.bugpatterns.SpringMvcAnnotation 1 9
🧟tech.picnic.errorprone.bugpatterns.PrimitiveComparison 1 13
🧟tech.picnic.errorprone.refaster.runner.Refaster 1 2
🧟tech.picnic.errorprone.guidelines.bugpatterns.RefasterAnyOfUsage 1 8
🧟tech.picnic.errorprone.bugpatterns.CanonicalAnnotationSyntax 1 4
🧟tech.picnic.errorprone.bugpatterns.LexicographicalAnnotationAttributeListing$1 1 1
🎉tech.picnic.errorprone.bugpatterns.StaticImport 0 4
🎉tech.picnic.errorprone.documentation.BugPatternTestExtractor$BugPatternTestCollector 0 8
🎉tech.picnic.errorprone.refaster.test.RefasterRuleCollection 0 4
🎉tech.picnic.errorprone.bugpatterns.RedundantStringConversion 0 9
🎉tech.picnic.errorprone.bugpatterns.LexicographicalAnnotationAttributeListing 0 4
🎉tech.picnic.errorprone.bugpatterns.JUnitValueSource 0 8
🎉tech.picnic.errorprone.bugpatterns.NonStaticImport 0 2
🎉tech.picnic.errorprone.bugpatterns.DirectReturn 0 9
🎉tech.picnic.errorprone.guidelines.bugpatterns.UnqualifiedSuggestedFixImport 0 4
🎉tech.picnic.errorprone.bugpatterns.FormatStringConcatenation$ReplacementArgumentsConstructor 0 2
🎉tech.picnic.errorprone.refaster.matchers.ThrowsCheckedException 0 8
🎉tech.picnic.errorprone.bugpatterns.AmbiguousJsonCreator 0 2
🎉tech.picnic.errorprone.utils.MoreJUnitMatchers 0 4
🎉tech.picnic.errorprone.bugpatterns.CanonicalClassNameUsage 0 4
🎉tech.picnic.errorprone.bugpatterns.MockitoMockClassReference 0 8
🎉tech.picnic.errorprone.refaster.test.RefasterRuleCollection$UnexpectedMatchReporter 0 3
🎉tech.picnic.errorprone.bugpatterns.IsInstanceLambdaUsage 0 4
🎉tech.picnic.errorprone.refaster.matchers.IsEmpty 0 2
🎉tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation 0 29

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 added breaking change and removed chore A task not related to code (build, formatting, process, ...) labels Feb 11, 2024
@Stephan202 Stephan202 merged commit 433b8b9 into master Feb 11, 2024
15 checks passed
@Stephan202 Stephan202 deleted the sschroevers/require-jdk-17 branch February 11, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants