-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Initial draft for providing @DefaultLocale and @DefaultTimeZone extensions
#5142
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: main
Are you sure you want to change the base?
Conversation
@DefaultLocale and @DefaulTimezone extensions
marcphilipp
left a comment
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 this initial push! 👍
How to use JUnit's internal testkit
We just use EngineTestKit directly, without the abstraction that Pioneer has. I commented on one such test.
Why checkstyle fails just right after
spotless:applygoal was run
Checkstyle can fail even though Spotless formatted the code. Spotless is purely about formatting, while we use Checkstyle for some more semantic checks.
That ArchUnit rules are in place
What did you want to check for?
jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/DefaultTimeZoneTests.java
Outdated
Show resolved
Hide resolved
...upiter-engine/src/main/java/org/junit/jupiter/engine/extension/DefaultTimeZoneExtension.java
Outdated
Show resolved
Hide resolved
jupiter-tests/src/test/java/org/junit/jupiter/api/locale/DefaultLocaleTests.java
Show resolved
Hide resolved
jupiter-tests/src/test/java/org/junit/jupiter/api/timezone/DefaultTimeZoneTests.java
Show resolved
Hide resolved
...upiter-engine/src/main/java/org/junit/jupiter/engine/extension/DefaultTimeZoneExtension.java
Outdated
Show resolved
Hide resolved
jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/DefaultTimeZoneTests.java
Outdated
Show resolved
Hide resolved
@DefaultLocale and @DefaulTimezone extensions@DefaultLocale and @DefaultTimeZone extensions
|
Thank you for your feedback, I think (hope) I applied all of it correctly. I find the testkit methods very confusing and verbose, but I guess that's a thing how used you are to them.
In general I'm done with most parts, I now have to figure out which are the correct |
| AnnotatedElement element = context.getElement().orElse(null); | ||
| AnnotationSupport.findAnnotation(element, DefaultLocale.class).ifPresent( |
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.
This won't find annotations on top-level and nested classes (which is why the test fail). You can either search the hierarchy upward (like the Pioneer implementation does) or implement BeforeAllCallback and store the value in the class-level store so it can be used as a fallback 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.
As discussed via email, I've pushed a commit that uses BeforeAllCallback to achieve this. The second commit moves the extension implementation to junit-jupiter-api to avoid its overhead since it should only be active when the @DefaultLocale annotation is used.
@Bukama Could you please do the same for @DefaultTimeZone?
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.
Thank you for your help. Applied it to @DefaultTimeZone, but need to figure out, where I have a circular dependency (cause ArchUnit-Report is truncated).
Had to laugh when reading your move, cause the first thing I did when porting was to split api and extension, because I understood your comment in the issue to do so :D
Another question: As you released 6.1-RC1 today, I guess I should switch the @since to 6.2?
edit: Will search again tomorrow, I'm tired after 16h awake
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.
Had to laugh when reading your move, cause the first thing I did when porting was to split api and extension, because I understood your comment in the issue to do so :D
Sorry about that!
Another question: As you released 6.1-RC1 today, I guess I should switch the
@sinceto 6.2?
I only released a milestone so some of the features that are already in can be tested sooner. This PR is still targeted for 6.1. Please add a release note entry for 6.1.0-M2 (you'll need to rebase/merge first).
dda60f6 to
14a75bc
Compare
|
To keep you up to date: |
Hmm, I hadn't thought of that. We should probably move |
Actually, I've just done this on main so it should suffice for you to rebase this PR. |
Great, I will work later this day / tomorrow |
cada4ef to
9d1e8c1
Compare
|
I give up. After wasting another few hours for getting those tests green which check module system files that do nowhere exists and complain about modules would not exists while the compiler says they do. |
This is a first draft (cause I'm getting tired and will pause for today) of passing the
@DefaultLocaleand@DefaultTimezoneextensions to JUnit.Points I have to check (feel free to gimme some hints)
spotless:applygoal was runI hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations