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.

Sign up for GitHub

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

Jump to bottom

Allow non-static Cartesian factory methods with PER_CLASS lifecycle #628

Merged
merged 3 commits into from May 25, 2022
Merged

Allow non-static Cartesian factory methods with PER_CLASS lifecycle #628

merged 3 commits into from May 25, 2022

Conversation

robtimus
Copy link
Contributor

@robtimus robtimus commented May 8, 2022 •
edited 

Allow non-static Cartesian factory methods with PER_CLASS (#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 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

PR checklist

Documentation (general)

  • There is documentation (Javadoc and site documentation; added or updated)
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style

Code

  • Code adheres to code style, naming conventions etc.
  • Successful tests cover all changes
  • There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
  • Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

  • A prepared commit message exists
  • The list of contributions inside README.md mentions the new contribution (real name optional)

Michael1993
Michael1993 requested changes May 12, 2022
View reviewed changes
Copy link
Member

@Michael1993 Michael1993 left a 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.

@Michael1993
Copy link
Member

Don't forget spotless! 👿

@rob-spoor
Copy link

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.

@Michael1993
Copy link
Member

Please rebase or merge main.

@Michael1993
Copy link
Member

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.

Whoops. 😅

@nipafx
Copy link
Member

nipafx commented May 12, 2022

Looks good, but agree with @Michael1993 that an example should be added to the docs.

Good job in the commit message, btw!

@robtimus
Copy link
Contributor Author

The rebase is complete, I'll work on the documentation shortly.

@robtimus robtimus requested a review from Michael1993 May 14, 2022 11:41
@robtimus
Copy link
Contributor Author

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 main:

  • Using a non-static factory method from another class in a class annotated with @TestInstance(PER_CLASS) still fails.
  • Using a static factory method from another class in a class annotated with @TestInstance(PER_CLASS) still works.
  • Using a non-static factory method from the same class in a class annotated with @TestInstance(PER_CLASS) works (I already added this test before).

As a bonus I added support for nested test classes in PioneerTestKit. I made those changes generic enough to also support entire nested test classes and no-argument methods in nested test classes.

Michael1993
Michael1993 approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@Michael1993 Michael1993 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thank you!

nipafx
nipafx reviewed May 19, 2022
View reviewed changes
Copy link
Member

@nipafx nipafx left a 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?

docs/cartesian-product.adoc Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
@robtimus robtimus requested a review from Michael1993 May 21, 2022 11:07
@robtimus
Copy link
Contributor Author

robtimus commented May 21, 2022 •
edited

@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.

@Bukama Bukama requested a review from nipafx May 24, 2022 15:53
robtimus and others added 3 commits May 25, 2022 13:38
@robtimus
Allow non-static Cartesian factory methods with PER_CLASS lifecycle
6164e95 
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
@robtimus
Improved requiredness check for static factory methods (628)
be9f231 
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
Merge branch 'main' into allow-non-static-methods-with-PER_CLASS
1e85ec7
@nipafx nipafx merged commit 58c4571 into junit-pioneer:main May 25, 2022
@nipafx
Copy link
Member

nipafx commented May 25, 2022

Merged - thank you for your contribution @robtimus! ❤️

@robtimus robtimus deleted the allow-non-static-methods-with-PER_CLASS branch May 25, 2022 15:40
@robtimus
Copy link
Contributor Author

You're welcome 😄

Bukama pushed a commit to Bukama/junit-pioneer that referenced this pull request Sep 20, 2022
@robtimus @Bukama
Allow non-static Cartesian factory methods with PER_CLASS (junit-pion…
252aee3 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nipafx nipafx nipafx left review comments

@Michael1993 Michael1993 Awaiting requested review from Michael1993

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@robtimus @Michael1993 @rob-spoor @nipafx