-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Deprecate unsupported JRE
enum constants and update related conditions
#4516
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
base: develop/6.x
Are you sure you want to change the base?
Conversation
a6eb2b1
to
f263410
Compare
f263410
to
71c3d64
Compare
junit-jupiter-api/src/templates/resources/main/org/junit/jupiter/api/condition/JRE.java.jte
Show resolved
Hide resolved
junit-jupiter-api/src/main/java/org/junit/jupiter/api/condition/AbstractJreCondition.java
Outdated
Show resolved
Hide resolved
junit-jupiter-api/src/main/java/org/junit/jupiter/api/condition/AbstractJreRangeCondition.java
Outdated
Show resolved
Hide resolved
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.
Thanks for putting together this PR!
I've left some comments for discussion...
junit-jupiter-api/src/templates/resources/main/org/junit/jupiter/api/condition/JRE.java.jte
Show resolved
Hide resolved
junit-jupiter-api/src/templates/resources/main/org/junit/jupiter/api/condition/JRE.java.jte
Show resolved
Hide resolved
junit-jupiter-api/src/templates/resources/main/org/junit/jupiter/api/condition/JRE.java.jte
Outdated
Show resolved
Hide resolved
junit-jupiter-api/src/templates/resources/main/org/junit/jupiter/api/condition/JRE.java.jte
Show resolved
Hide resolved
* `@EnabledOnJre`, `@DisabledOnJre`, `@EnabledForJreRange`, and `@DisabledForJreRange` now | ||
fail if a JRE version less than 17 is specified. |
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 don't think we can do that, since that would break existing code.
In other words, deprecating the unsupported enum constants should suffice: people will see the warnings and update their code accordingly. But... existing code should not break.
For example, @EnabledForJreRange(min = JAVA_9, max = JAVA_21)
should still logically work with a Java 17 baseline, and the deprecation of JAVA_9
will encourage the user to migrate that declaration to @EnabledForJreRange(min = JAVA_17, max = JAVA_21)
.
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 don't think we can do that, since that would break existing code.
I had a softer validation mode at first where it would only fail if the range contains no version that is no longer supported at runtime (e.g. 9-11).
For example,
@EnabledForJreRange(min = JAVA_9, max = JAVA_21)
should still logically work with a Java 17 baseline, and the deprecation ofJAVA_9
will encourage the user to migrate that declaration to@EnabledForJreRange(min = JAVA_17, max = JAVA_21)
.
The deprecation only applies for the JRE
enum constants. If integers are used, there will be no deprecation warning. For example, @EnabledForJreRange(minVersion = 9, maxVersion = 11)
will not produce a compiler warning. The test, however, will never be executed because 17 is the minimum runtime version.
I think it would be acceptable to break some existing tests (at runtime) when users upgrade to 6.0. It's better than the alternative of tests always being skipped going unnoticed. We could relax the validation to only fail if a range contains no supported runtime version (so 9-21 would be allowed because contains 17-21). That would still break some tests but fewer.
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.
We could make the strictness of the validation configurable via a configuration parameter... 🤔
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.
We could relax the validation to only fail if a range contains no supported runtime version (so 9-21 would be allowed because contains 17-21).
Yes, let's do that.
If we can determine that the range is now obsolete, throw an exception.
If we can determine that the range is still valid (even if too broad), do not throw an exception.
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.
We could make the strictness of the validation configurable via a configuration parameter... 🤔
Mmmm... yeah, we could do that, but I'd rather avoid too many behavioral configuration flags if possible.
Maybe we should just see how the community reacts to "rejection of no longer applicable 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.
I've removed the strict check again and decided against the additional validation mainly because we already have tests in place that use @DisabledOnJre
with all defined JRE
enum constants (except JRE.UNDEFINED
) and verify that the test is skipped.
9ac1c76
to
0bbe8e4
Compare
0bbe8e4
to
415c4fe
Compare
415c4fe
to
d131aac
Compare
JRE.JAVA_8
...JAVA_16
min
of@EnabledForJreRange
and@DisabledForJreRange
toJRE.JAVA_17
Overview
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@API
annotations