-
Notifications
You must be signed in to change notification settings - Fork 88
Adapt @EnabledOnJre
, @DisabledOnJre
, @EnabledForJreRange
and @DisabledForJreRange
for JUnit 6's Java 17 minimum
#795
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
Conversation
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
private static final String DISABLED_ON_JRE = "org.junit.jupiter.api.condition.DisabledOnJre"; | ||
private static final String ENABLED_FOR_JRE_RANGE = "org.junit.jupiter.api.condition.EnabledForJreRange"; | ||
private static final String DISABLED_FOR_JRE_RANGE = "org.junit.jupiter.api.condition.DisabledForJreRange"; | ||
private static final Annotated.Matcher TEST_ANNOTATION_MATCHER = new Annotated.Matcher("@org.junit.jupiter.api.Test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite a few more test annotations we'd want to support, like @ParameterizedTest
, @RepeatedTest
and others similarly covered for other recipes. Can we add support here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those other test annotations can be identified generically by looking for the @TestTemplate
meta-annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the meta-annotations in the type hierarchy on our lst's (nor do I know exactly where to look), but added it like other recipes are doing it.
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Show resolved
Hide resolved
void handleRangeWithOnlyMin(String range) { | ||
rewriteRun( | ||
java( | ||
""" | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.condition.EnabledForJreRange; | ||
import org.junit.jupiter.api.condition.JRE; | ||
|
||
class MyTest { | ||
@Test | ||
@EnabledForJreRange(%s) | ||
void testOnJava11AndLater() { | ||
System.out.println("Java 11+"); | ||
} | ||
} | ||
""".formatted(range) | ||
) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally here we'd want to set a new minimum of 17 going forward right, for open ranges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ranges and adapting them I had mentioned to do in another iteration of this recipe, but will get on this now that I have addressed the other review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; let me know when this is ready for another round of review and possible merge , or whether anything is still planned or in progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timtebeek
Ranges have been implemented causing a bunch of repeated code lines to be extracted into separate methods with a clear name on what they do.
No more pending items on this one
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
Outdated
Show resolved
Hide resolved
- Added more Test annotations support - Unwrap single value arrays - Do not remove condition annotations - ...
@EnabledOnJre
, @DisabledOnJre
, @EnabledForJreRange
and @DisabledForJreRange
for JUnit 6's Java 17 minimum
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
Outdated
Show resolved
Hide resolved
This last commit won't work I guess as versions can also be an J.Array of versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see! Looking forward to seeing how this will pan out in practice; did you already run against a larger suite of projects?
Ah then best reverted with an appropriate test added to guard against regressions. |
No,not yet! Do you mean this one only? |
I was wrong... I knew there was something to it. |
We can likely eye-ball the diffs this one only would produce on an org like Apache, even locally with the CLI, and from there possibly see possible improvements, or have enough confidence to merge this PR already. |
What would we want to do in this situation @timtebeek? Can we safely assume the comment on same line will be Rest is looking good though (ran against Apache ~ 1200 repos, no errors and 20'ish validated results)! I must say... It keeps feeling weird to remove test cases that verify "reflection-java-8/11/15-only-usecases" in certain java versions for a library just because the test framework runs tests in a higher junit version. |
I think it's fine not to handle that for now;if we did we should handle it in the visitor that removes the annotation instead.
Great! Thanks for looking through those.
I agree it looks odd, but perhaps it helps to look at the deprecated enum values? So then the way I see it they are there only to avoid compilation issues, but not actually used with the recommendation to use |
Add recipe to simplify JRE version conditions for JUnit 6 migration
Summary
This PR introduces a new recipe
MinimumJreConditions
that simplifies JUnit Jupiter test conditions based on a minimum JRE version requirement. As part of the JUnit 6 migration path, this recipe helps clean up obsolete JRE version conditions when migrating to a minimum Java version.What's Changed
New Recipe:
MinimumJreConditions
The recipe processes JUnit Jupiter's JRE conditional annotations (
@EnabledOnJre
,@DisabledOnJre
,@EnabledForJreRange
,@DisabledForJreRange
) and:Key Features
Supports multiple annotation formats:
JRE.JAVA_8
,JRE.JAVA_17
, etc. (also the statically imported ones)value = JRE.JAVA_17
versions = 17
orversions = { 17, 21 }
minVersion = 17, maxVersion = 21
Handles range conditions:
Smart annotation cleanup:
@EnabledOnJre(JRE.JAVA_17)
when 17 is the minimum)Integration with JUnit 6 Migration
The recipe has been added to the main JUnit 6 migration recipe with Java 17 as the default minimum version, aligning with JUnit 6's requirements.
Files Changed
src/main/java/org/openrewrite/java/testing/junit6/MinimumJreConditions.java
- Recipe implementationsrc/main/resources/META-INF/rewrite/junit6.yml
- Added recipe to JUnit 6 migrationsrc/test/java/org/openrewrite/java/testing/junit6/MinimumJreConditionsTest.java
- Comprehensive test coverageExample Transformations
Before
After (with minimum JRE 17)
Testing
Comprehensive test coverage includes:
JRE.OTHER
)Related Issues
This recipe supports the JUnit 6 migration effort by ensuring test suites are properly cleaned up when moving to newer minimum JRE requirements.