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

Investigate parameter targeting of CartesianProductTest arguments source annotations #415

Closed
Michael1993 opened this issue Jan 25, 2021 · 10 comments · Fixed by #487
Closed

Comments

@Michael1993
Copy link
Member

A very strong argument can be made that it's much more convenient to define a CartesianProductTest like the following:

@CartesianProductTest
void test(
    @CartesianValueSource(strings = { "A", "B", "C" }) String str,
    @CartesianEnumSource(ChronoUnit.class) ChronoUnit unit
) {
    // some passing test code here
}

Investigate if this is possible - it is likely it is not possible, because of how the annotations invoke their corresponding ArgumentsProvider implementations, but it might work.

@scordio
Copy link
Contributor

scordio commented Jan 25, 2021

My two cents: I personally consider mimicking JUnit's @ParameterizedTest a strong characteristic of @CartesianProductTest, which makes its adoption very quick. Targeting the parameters would deviate from the known JUnit patterns. On the other side, sticking to those patterns might limit the growth of the @CartesianProductTest feature.

It could still be fine if the example above is an alternative way of declaring the parameters, but then I am wondering if it's worth offering multiple ways to do the same thing (and maybe supporting combinations of both ways?)

@Michael1993
Copy link
Member Author

Michael1993 commented Feb 8, 2021

The main reason I opened this issue is because this is a frequently asked question. I would like to have a discussion/discovery about this, so I can link to it as an explanation, i.e.: "This is not possible, we investigated this in #415" or "While this is possible, the team came to a decision to not support it - see discussion on #415" or we might just decide to support it but discourage users from using it, etc.

ETA: "frequently" as in 2 people have asked this so far.

@Michael1993
Copy link
Member Author

Michael1993 commented Mar 19, 2021

With the discovery of the repeatable annotations bug (#447), this investigation should have a bit more priority (it could solve the bug - if it works).

@nipafx
Copy link
Member

nipafx commented Apr 6, 2021

The similarity to @ParameterizedTests is pretty nice, but there's one important difference between the two extensions: For parameterized tests, annotating individual arguments makes no sense. For this extension it does (and a lot, at that). I'm really interested in this idea, particularly as it seems to offer a great solution to #447.

Let's discuss first whether we think this is a good idea - maybe you can support that conversation by digging into the implementation or even working on a proof of concept.

Once we know whether we prefer this solution and by how much and how complex it is to implement, we can discuss whether we offer one (which one?) or both approaches and how to possibly migrate if that becomes necessary.

@nipafx
Copy link
Member

nipafx commented May 5, 2021

While developing the proof of concept for #49 yesterday, I created annotations @RandomString and @RandomInt to be used as follows:

@ParameterizedTest
@RandomArguments(count = 4)
void test(
		@RandomString(minLength = 5, maxLength = 7) String beer,
		@RandomInt(min = 8, max = 24) int proof) {
	// ...
}

That worked really well. I think there's potential to revamp the Cartesian product extension and make it much more powerful by more easily interacting with a wide variety of different annotations:

@CartesianProductTest
void test(
		@StringValues( "A", "B", "C" ) String str,
		@EnumValues(ChronoUnit.class) ChronoUnit unit,
		@LongRange(from = 1, to = 10) long l,
		@FromMethod("userSupplier") User u,
		@RandomIntegers(count = 4) int i) {
	// ...
}

@nipafx
Copy link
Member

nipafx commented May 29, 2021

While reviewing a part of #487, I wrote this comment:

The long names are really unfortunate. If we just used @Values, it would look so much better. But that name really looks it would collide with something else sooner or later. Do we need a package jupiter.cartesian? If the break the extension to that degree, we might as well rename @CartesianProductTest to @CartesianTest. Break all the things! (imagine meme)

Thoughts?

@nipafx
Copy link
Member

nipafx commented May 29, 2021

I want to point out that we didn't have subpackages so far, so starting to do that would be quite the change and open the "which extensions need their own packages" can of worms that we prevented so far.

@nipafx
Copy link
Member

nipafx commented Jun 15, 2021

While working on #487, we stumbled on a very interesting question: Now that we consider adding a new package for Cartesian product tests, can we shorten the names of the annotations that work with it? While discussing this, we discovered five ways to organize and name things.

Side note: In any of the following summaries, I'll assume we'll rename @CartesianProductTest to @CartesianTest, but if not, that makes no difference.

Side note II: Range sources have to keep their names because they need to fit in with the parameterized test annotations.

Long Names

Basically as they are, in the existing or a new package:

  • in org.junitpioneer.jupiter or org.junitpioneer.jupiter.cartesian
  • @CartesianTest
  • @CartesianValueSource
  • @CartesianEnumSource
  • @CartesianFactorySource

Short Names

In a dedicated package, we could shorten the names:

  • in org.junitpioneer.jupiter.cartesian
  • @CartesianTest
  • @ValueSource
  • @EnumSource
  • @FactorySource

Very Short Names

In a dedicated package, we could shorten the names even more:

  • in org.junitpioneer.jupiter.cartesian
  • @CartesianTest
  • @Values
  • @Enums
  • @Factory

Nested

Nested in @CartesianTest, we could use the very short names above:

  • in org.junitpioneer.jupiter or org.junitpioneer.jupiter.cartesian
  • @CartesianTest
  • @CartesianTest.Values
  • @CartesianTest.Enums
  • @CartesianTest.Factory

When importing inner classes (which IntelliJ doesn't do by default [by default?]), CartesianTest can be left out.

Duplicated and Nested

Duplicated and then the duplication nested within the original (cue Inception joke);

  • in org.junitpioneer.jupiter or org.junitpioneer.jupiter.cartesian
  • @CartesianTest
  • @CartesianValueSource.Values
  • @CartesianEnumSource.Enums
  • @CartesianFactorySource.Factory

Users can choose between the outer or inner annotations.

Thoughts?

Thoughts?

@nipafx
Copy link
Member

nipafx commented Jun 15, 2021

My thoughts:

  • Long Names: Most obvious default, but cumbersome and inelegant.
  • Short Names: Clash with Jupiter, so I think it's off the table
  • Very Short Names: Ballsy, I like it, but runs the risk of clashing with other extensions who also take an aggressive approach to naming
  • Nested: Interesting middle ground - devs can use short versions (e.g. @Values; with appropriate imports) if possible and revert to the outer class variant (@CartesianTest.Values) if necessary. It will bloat CartesianTest, though.
  • Duplicated and Nested: It's clever, but... KISS?

@Bukama
Copy link
Member

Bukama commented Jun 20, 2021

I like the Long and the Nested names, cause they are clearly associated with the CartesainProduct-Extection
From those two the Long are somehow clumsy, so I would like to go with the Nested

nipafx pushed a commit that referenced this issue Sep 2, 2021
The former @CartesianProductTest extension closely mirrored Jupiter's
@ParameterizedTest annotation. Since then users came up with the idea
that instead of providing arguments in annotations on the method,
they could be provided on the parameters themselves.

// BEFORE
@CartesianProductTest
@CartesianValueSource(ints = { 1, 2 })
@CartesianValueSource(ints = { 3, 4 })
void myCartesianTestMethod(int x, int y) {
    // passing test code
}

// NOW
@CartesianTest
void myCartesianTestMethod(
		@values(ints = { 1, 2 }) int x,
		@values(ints = { 3, 4 }) int y) {
	// passing test code
}

The old variant is still around, but deprecated and will be removed
in the next major release.

The new variant is different in more ways than just where annotations
are placed:

* new package `org.junitpioneer.jupiter.cartesian`
* shorter name `@CartesianTest`
* to keep parameter annotations short while avoiding collisions,
  they are now inner types of `@CartesianTest`
  (so in the example above, the full name is `@CartesianTest.Values`

Closes: #415
PR: #487
nipafx pushed a commit that referenced this issue Feb 19, 2022
Cartesian tests version 1 required users to annotate the test method
to define individual arguments. While version 2 of this extension
moves the bulk of configuration to parameter-level annotations it
should still allow method-level annotations to define the full set of
sets, for example to define them in a CSV or JSON file.

This is now possible. It's achieved by turning
`CartesianArgumentsProvider` into an internal marker interface with
two public extensions:

* `CartesianParameterArgumentsProvider`
  (for providing arguments for an individual parameter,
   i.e. takes on the former meaning of `CartesianArgumentsProvider`)
* `CartesianMethodArgumentsProvider`
  (for providing arguments for all parameters)

Related to #415
PR: #540
nipafx pushed a commit that referenced this issue Feb 19, 2022
* unlist old documentation from /docs
* fix cross-links
* mention the correct version (1.6.0)
* guess later 2.0 release date (2022)

Related to: #415
nipafx pushed a commit that referenced this issue Feb 21, 2022
JUnit (more precisely the module `org.junit.platform.commons`)
needs to reflectively access Pioneer packages. Since some classes
aren't public, this fails with an `InaccessibleObjectException` if
Pioneer is placed on the module path. To fix this, the packages
must be opened.

Furthermore, this change fixes a mistake from #415 and exports the
new package `org.junitpioneer.jupiter.cartesian`.

Closes: #539
PR: #597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants