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.

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

CartesianProductTestExtension from JUnit examples #321

Merged
merged 51 commits into from Oct 6, 2020
Merged

CartesianProductTestExtension from JUnit examples #321

merged 51 commits into from Oct 6, 2020

Conversation

Michael1993
Copy link
Member

@Michael1993 Michael1993 commented Aug 14, 2020

Opened to let us move forward with #68


PR checklist

The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.

Documentation (general)

  • There is documentation (Javadoc and site documentation; added or updated)
  • There is implementation information to describe why a non-obvious source code / solution got implemented
  • Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style (see Documentation: Personal or formal style #265)

Documentation (new extension)

  • The docs/docs-nav.yml navigation has an entry for the new extension
  • The package-info.java contains information about the new extension

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 (see Require configuration failiure tests? #164)
  • 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)

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Michael1993
Copy link
Member Author

Michael1993 commented Aug 14, 2020

Things to consider:

  • Explicit factory method option (i.e. @CartesianProductTest(factory = "getSources"))
  • Define parameter source with a separate annotation
  • Supporting @ArgumentsSource annotations (which are not repeatable, apparently) -> relevant JUnit 5 issue
  • What should this throw, AssertionError or ExtensionConfigurationException?
  • How do different argument sources interact? Should it be illegal to specify multiple argument sources?
  • Should CartesianProductTest be moved to org.junitpioneer.jupiter.params instead of org.junitpioneer.jupiter?
  • Internally, we use List but should we enforce unique elements? It'd make sense, since {1, 1} ⨯ {2, 3} is functionally equivalent to {1} ⨯ {2, 3}.

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@Michael1993 Michael1993 marked this pull request as ready for review August 14, 2020 23:18
…mentation

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

My request changes are mostly about docs. The rest are more questions.

docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Show resolved Hide resolved
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@beatngu13
Copy link
Member

I just briefly skimmed over the PR, really like that feature. Personally, I already needed the Cartesian product several times in the past. Kudos @Michael1993!

I have only one question: When it comes to supplying test parameters, I just wonder if it would be helpful to stick to the style of @ParameterizedTest, where the parameter source is defined via a separate annotation?

Copy link
Member

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

Some smaller things in the docs I obviusly missed on the first turn. Sorry for that. After that it's okay for me.

docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved
docs/cartesian-product.adoc Outdated Show resolved Hide resolved

@Override
public boolean supportsTestTemplate(ExtensionContext context) {
return findAnnotation(context.getTestMethod(), CartesianProductTest.class).isPresent();
Copy link
Member

Choose a reason for hiding this comment

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

Then you argue to throw Pioneer Annotations out, because we can use the JUnit things. The idea was Pioneer Annotation was to encapsulate the JUnit thing with our own, improved methods. But I'll leave it to you / the others which way we use.

Same for TestKit or PioneerAssertions 🤷‍♂️

@Michael1993
Copy link
Member Author

When it comes to supplying test parameters, I just wonder if it would be helpful to stick to the style of @ParameterizedTest, where the parameter source is defined via a separate annotation?

@beatngu13 There is a bit of a clash.

When you create a @ParameterizedTest, you supply a single (!) @ArgumentsSource annotation where you define every test case. This is also the reason why annotations annotated with @ArgumentsSource (e.g.: @CsvSource or @ValueSource) are not @Repeatable.

This is different from @CartesianProductTest, where you want to define argument sets for each parameter individually and let the extension define the test cases. This is an entirely different approach from @ParameterizedTest.

We cannot support the annotations used by @ParameterizedTest because we would need them to be @Repeatable.
They never will be repeatable because their design philosophy is different from what we need.

We can create our own annotation(s), (e.g.: @CartesianValueSource, which works) that are repeatable - the question is, how confusing is the difference between these argument sources and their behaviour?

…to `CartesianProductTest`

Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
Signed-off-by: Mihaly Verhas <misi.verhas@gmail.com>
@beatngu13
Copy link
Member

@Michael1993 ah, I see. Thanks a lot for that detailed explanation!

I think this is important:

When you create a @ParameterizedTest, you supply a single (!) @ArgumentsSource annotation where you define every test case. […] This is different from @CartesianProductTest, where you want to define argument sets for each parameter individually and let the extension define the test cases.

So, @CartesianProductTest is more like a form of dynamic tests? I just want(ed) to find out if there is an existing JUnit 5 concept / API style we can build upon.

However, the PR is already great, @Bukama likes it and since we are pioneers … 😉 Let's see what @nicolaiparlog thinks.

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.

A few details...

Comment on lines +27 to +31
* {@code @CartesianProductTest} is a JUnit Jupiter extension that marks
* a test to be executed with all possible input combinations.
*
* <p>Methods annotated with this annotation should not be annotated with {@code Test}.
* </p>
Copy link
Member

Choose a reason for hiding this comment

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

Can we get a little more detail on how to use the extension and a link to the website with details?


@Override
public boolean supportsTestTemplate(ExtensionContext context) {
return findAnnotation(context.getTestMethod(), CartesianProductTest.class).isPresent();
Copy link
Member

Choose a reason for hiding this comment

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

The advantage of our annotation methods is that they make it easy to find meta annotations that are _indirectly present: (if a supertype of the element is annotated), meta-present (if an annotation that is present on the element is itself annotated), or _enclosing-present (if an enclosing type [think opposite of @Nested] is annotated). As I see it, only meta-present is relevant here, but that should indeed be allowed. Not sure if AnnotationSupport.findAnnotation does that.

@Michael1993
Copy link
Member Author

From the documentation of AnnotationSupport:

Find the first annotation of annotationType that is either directly present, meta-present, or indirectly present on the supplied element.
If the element is a class and the annotation is neither directly present nor meta-present on the class, this method will additionally search on interfaces implemented by the class before finding an annotation that is indirectly present on the class.

It should be enough to use here.

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants