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
Allow non-static Cartesian factory methods with PER_CLASS lifecycle #628
Allow non-static Cartesian factory methods with PER_CLASS lifecycle #628
Conversation
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.
Hi! 👋
Thank you for this, looks great so far! Could you please also add an example with a non-static factory method to the documentation? Either to showcase the wrong usage (i.e. with a missing @TestInstance
annotation) or the correct usage.
Don't forget spotless! 👿 |
The errors are an update in the year in the licenses. I think that the spotless configuration needs to be updated, because the second year should be 2022, not 2021. Spotless suggest changing it back to 2021. |
Please rebase or merge main. |
Whoops. 😅 |
Looks good, but agree with @Michael1993 that an example should be added to the docs. Good job in the commit message, btw! |
The rebase is complete, I'll work on the documentation shortly. |
I've added the requested example. While working on the example I found a case where my change would have caused failures - using a non-static factory method from another class. I've improved the check that determines whether or not the factory method must be required, and added some tests. There are now three, in addition to the unmodified tests that were already on
As a bonus I added support for nested test classes in |
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.
LGTM 👍
Thank you!
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 started a review a week ago and forgot to send it, stupid me. Just added another comment - can you take a look at this?
@nipafx can you check again? I've rebased from main and split off the example, as was recently done on main for other Cartesian test examples. |
src/demo/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtensionDemo.java
Outdated
Show resolved
Hide resolved
JUnit allows non-static factory methods as long as the test class is annotated with @testinstance(Lifecycle.PER_CLASS). The same is currently not true for @CartesianTest.MethodFactory. This commit fixes that. The existing non-static test is unmodified, indicating that non-static is still not allowed if the test class is not annotated with @testinstance(Lifecycle.PER_CLASS). PR: #628
When allowing non-static factory methods in combination with @testinstance(Lifecycle.PER_CLASS), there would be failures if the factory method was defined in a different class. The factory method now must also be static if it's not defined in the test class (or a super class of the test class). Also added an example on how to use @CartesianTest.MethodFactory in a nested test class. PR: #628
Merged - thank you for your contribution @robtimus! ❤️ |
You're welcome 😄 |
…eer#628) JUnit allows non-static factory methods as long as the test class is annotated with `@Testinstance(Lifecycle.PER_CLASS)`. The same is currently not true for `@CartesianTest.MethodFactory`. This change fixes that. The existing non-static test is unmodified, indicating that non-static is still not allowed if the test class is not annotated with `@Testinstance(Lifecycle.PER_CLASS)`. PR: junit-pioneer#628
PR checklist
Documentation (general)
.adoc
files)Code
Contributing
README.md
mentions the new contribution (real name optional)