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

Introduce support for parameterized containers (classes, records, etc) #878

Open
jkschneider opened this issue Jun 9, 2017 · 64 comments
Open

Comments

@jkschneider
Copy link

jkschneider commented Jun 9, 2017

Overview

Currently, the target of @ParameterizedTest is constrained to methods. When creating technology compatibility kits, it would be awesome to be able to apply this (or a similar annotation) to the test class so that all tests in that class are parameterized the same way.

Proposal

Rather than:

class MyTest {
   @ParameterizedTest
   @ArgumentSource(...)
   void feature1() { ... }

   @ParameterizedTest
   @ArgumentSource(...)
   void feature2() { ... }
}

Something like:

@ParameterizedTest
@ArgumentSource(...)
class MyTest {
   @Test // ?
   void feature1() { ... }
   
   @Test
   void feature2() { ... }
}

Related Issues

@marcphilipp
Copy link
Member

I can imagine supporting something like

@ArgumentSource(...)
class MyTest {
   @ParameterizedTest
   void feature1() { ... }
   
   @ParameterizedTest
   void feature2() { ... }
}

Would that suit your needs?

@jkschneider
Copy link
Author

I think that is a good solution.

@nipafx
Copy link
Contributor

nipafx commented Jun 16, 2017

Related issues: #871, #853

@jkschneider
Copy link
Author

For real-life examples, see GaugeTest and most of the other tests in its package.

@sbrannen sbrannen changed the title Allow @ParameterizedTest on class types for TCKs Allow @ParameterizedTest on class types for TCKs Jun 19, 2017
@sbrannen sbrannen changed the title Allow @ParameterizedTest on class types for TCKs Allow @ParameterizedTest declarations at type level for TCKs Jul 11, 2017
@marcphilipp
Copy link
Member

@jkschneider Do you have time to work on a PR?

@marcphilipp marcphilipp modified the milestones: 5.0 M6, 5.1 Backlog Jul 18, 2017
@smoyer64
Copy link
Contributor

If a parameter source annotation is placed on an individual method in a type that's been annotated like this would it make sense for the more specific annotation to take precedence?

@jkschneider
Copy link
Author

Yes as a matter of determinism? I think in practice, this would not be the way I'd recommend folks structure their test.

@wrlyonsjr
Copy link

Is this feature on the roadmap? I can't migrate to 5 without it.

@wrlyonsjr
Copy link

...unless someone knows of a workaround.

@jkschneider
Copy link
Author

jkschneider commented Oct 24, 2017

@wrlyonsjr I flipped it around with Micrometer's TCK by defining an abstract class with my test methods, each of which takes an implementation of MeterRegistry and for which there are many implementations. The RegistryResolver extension injects the implementation into each method at any nesting level.

Then there is a corresponding test for each implementation of MeterRegistry that extends this base class.

The approach has the disadvantage that you can't run the TCK abstract class from the IDE and execute the tests for all implementations, but this is the best I could do.

@wrlyonsjr
Copy link

It seems like my problem could be solved with TestTemplate et al.

@sbrannen sbrannen modified the milestones: 5.x Backlog, 5.1 Backlog Dec 13, 2017
@sbrannen
Copy link
Member

Moved to 5.1 backlog due to repeated requests for a feature like this at conferences, etc.

@dmitry-timofeev
Copy link
Contributor

Let me share my experience with parameterized tests in the new JUnit. Hope that helps to clarify the uses cases and requirements for Parameterized classes.

I think that this feature will be universally useful, not for TCK only. I see its ultimate goal in bringing down the cost of defining multiple tests sharing the same parameters source. Currently, the following code is duplicated:

  • Annotations, specifying the source (e.g., @MethodSource).
  • Formatting strings for parameters (e.g., "[{index}] = {3}").
  • Initialization & Clean-up code, if any. If it has to initialize multiple local variables (i.e., not extractable in a separate method as a whole), it has the highest cost in terms of LOCs.

Having to repeat that code discourages the users to write short, focused tests. On top of that, if developers do have a luxury of copy-pasting that code, reviewers and maintainers have to read all of it.

As an alternative, I've tried a dynamic test for a simple use case (little setup code, no teardown), but got both a personal impression and some reviews from colleagues that it's "overcomplicated".

Uses cases

A perfect use case for this feature, in my opinion, is the following:

  1. A user writes a couple of parameterized tests which share the same parameters source and setup code.
  2. When there are too many of them, a user extracts these tests in a nested parameterized class (ideally, an IDE inspection tells the user to do that).
    • Test method arguments become either constructor arguments, or injected with @Parameter (as in JUnit 4), or setup method (@BeforeEach) arguments.
    • Initialization code goes to the setup method. Any locals needed in tests become fields of the test class.
    • Clean-up code goes to the teardown method (@AfterEach).

@marcphilipp
Copy link
Member

I think supporting something like @Parameter makes sense for fields and method parameters.

I'm out of ideas where you'd put formatting strings for parameters that are shared for all parameterized tests, though.

@dmitry-timofeev
Copy link
Contributor

@marcphilipp , I can think of a class-level annotation with name attribute, or, if users need more flexibility, let them provide an instance method returning a test description + a method-level annotation, or a reference to the method in the class-level annotation (e.g., @Parameterized(name="#testDescription")).

If no one on the core team is planning to work on this issue soon, I may try to implement an MVP. I am new to the code base, and have a couple of questions about the requirements:

  • Shall anything except @ParameterizedTest be supported in a class annotated with @ArgumentsSource (@Test, other @TestTemplates)? It might get tricky for one of the most compelling use cases for parameterized classes is extracting test template parameters to the fields, and if there is no extension to resolve the values of these fields, it won't work, will it?
  • Shall the extension support a product of primary parameters (defined at the class level) and secondary parameters (defined at the method level, as in the current implementation), like this:
@CsvSource(/* primary parameters, injected into constructor/BeforeEach/fields */)
class FooTest {

  /** Invoked for each parameter in the source specified at the class level */
  @ParameterizedTest
  void foo() { }

  /** 
   * Invoked for the cartesian product of parameters from 
   * the source specified at the class level 
   * and parameters from the source at the method level.
   */
  @ParameterizedTest
  @ValueSource(/* secondary parameters for #bar only */)
  void bar(int secondaryParameter) { }
}

?

@sbrannen
Copy link
Member

Team Decision: during the face-to-face meeting this past week, we came up with the following use cases that we would ideally like to support.

@ParameterizedContainer
@CsvSource(textBlock = """
		apple, 1
		banana, 2
		""")
class ConstructorInjectionTestCase {

	private final String fruit;
	private final int score;

	ConstructorInjectionTestCase(String fruit, int score) {
		this.fruit = fruit;
		this.score = score;
	}

	@Test
	void test() {
		// Assert something based on fruit and score
	}

}
@ParameterizedContainer
@CsvSource(textBlock = """
		apple, 1
		banana, 2
		""")
class FieldInjectionTestCase {

	@Parameter(0)
	private String fruit;

	@Parameter(1)
	private int score;

	@Test
	void test() {
		// Assert something based on fruit and score
	}

}
@ParameterizedContainer
@CsvSource(textBlock = """
		apple,  1
		banana, 2
		""")
record CsvSourceRecordTestCase(String fruit, int score) {

	@Test
	void test() {
		// Assert something based on fruit and score
	}

}
@ParameterizedContainer
@MethodSource("fruitScores")
record MethodSourceRecordTestCase(String fruit, int score) {

	static Stream<Arguments> fruitScores() {
		return Stream.of(
			arguments("apple", 1),
			arguments("banana", 2));
	}

	@Test
	void test() {
		// Assert something based on fruit and score
	}

}

It turns out that Java records and Kotlin data classes would make a nice fit for parameterized containers. 😄

We are not yet making any promises about the final API, but the above is currently our goal.

@nipafx
Copy link
Contributor

nipafx commented Apr 15, 2023

Looks great! 👍🏾 And the synergy with records is just so good! 😃

Would @ParameterizedContainer use an extension point that other extensions could use, too?

@sormuras
Copy link
Member

sormuras commented Apr 15, 2023

For the record (pun!), I also like the approach to instantiate arguments directly, e.g. without using JUnit's reflection-based routines (but at the cost of some lifecycle support).

@ParameterizedContainer
@InstanceSource("instances")
record FruitScoreTests(String fruit, int score) {

	static Stream<FruitScoreTests> instances() {
		return Stream.of(
			new FruitScoreTests("apple", 1),
			new FruitScoreTests("banana", 2));
	}

	@Test
	void test() {
		// Assert something based on fruit and score
	}

}

@sbrannen
Copy link
Member

Looks great! 👍🏾 And the synergy with records is just so good! 😃

Yeah, the entire JUnit team is looking forward to being able to use parameterized records in real life. The moment I typed up that example during our brainstorming, we were all collectively like "Oooooh, I want that!". 😉

Would @ParameterizedContainer use an extension point that other extensions could use, too?

Yes, #871 is actually a prerequisite for @ParameterizedContainer.

@ParameterizedContainer would be a @ContainerTemplate that registers a ParameterizedContainerExtension which implements ContainerTemplateInvocationContextProvider. At least, that's the current thinking.

@sbrannen
Copy link
Member

Blocked until #871 is completed.

See #878 (comment) for details.

@sbrannen sbrannen changed the title Allow @ParameterizedTest declarations at type level for TCKs Introduce support for parameterized containers (classes, records, etc) Apr 15, 2023
@sbrannen
Copy link
Member

For the record (pun!), I also like the approach to instantiate arguments directly, e.g. without using JUnit's reflection-based routines (but at the cost of some lifecycle support).

Hey, wait a minute! How did you copy that from my computer? 😛

Disclaimer: For the record, I intentionally left that one out since we are not necessarily planning on supporting that in the initial release of the feature, but I do agree that it's an interesting idea.

And also "for the record", that was @sormuras' idea. 💡

Speaking of which, @sormuras, feel free to create a separate issue to officially propose and track that.

@tomwhoiscontrary
Copy link

Making the test class a record is a neat trick. But this is presumably incompatible with having other fields in the class which are initialised in their declaration or an @Before method. Roughly 100% of my test classes which are complicated enough to need parameterisation also have such fields, so i suspect that this will not be that valuable in actual use.

@marcphilipp
Copy link
Member

Thanks Tom for being contrary (SCNR) providing feedback on the proposal. A regular class with a constructor or @Parameter fields will work as well. 😉

@marcphilipp marcphilipp modified the milestones: 5.10.0-M1, 5.11.0-M1 Apr 21, 2023
@marcphilipp marcphilipp modified the milestones: 5.11 M1, 5.12 Jan 12, 2024
jerryshao pushed a commit to datastrato/gravitino that referenced this issue Jan 24, 2024
### What changes were proposed in this pull request?
add mysql 5.7 to  integerate IT

### Why are the changes needed?
junit5 doesn't support class level ParameterizedTest,
junit-team/junit5#878 . so use a separate
class.

Fix: #1367

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
integrate test
@marcphilipp marcphilipp modified the milestones: 5.12, 5.12 tmp Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment