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

Support for array arguments in @CartesianTest #528

Merged
merged 5 commits into from Nov 14, 2021

Conversation

filiphr
Copy link
Contributor

@filiphr filiphr commented Sep 23, 2021

Proposed commit message:

Support array arguments in `@CartesianTest` (#523 / ${pull-request}) [max 70 characters]

${body} [max 70 characters per line]

${references}: ${issues}
PR: ${pull-request}

This has a prerequisite on PR #519. It can only be reviewed / merged after that one.

While working on this I realised that there is no point in using Arguments from JUnit Jupiter. That API has support for providing an array of arguments that should then be resolved to the test method. However, with @CartesianTest you want to provider to provide a stream of single object values for a single parameter. Basically always using the first value of the Arguments.

In order to avoid complexity in the validations I removed the usage of Arguments and instead now we return only a stream of object. I think that it might be better if we can create an Argument interface and return Stream<? extends Argument> or make the CartesianArgumentsProvider generic and return Stream<T>. Let me know what you think.


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

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

@nipafx
Copy link
Member

nipafx commented Nov 14, 2021

Do we have to update the documentation as well?

@filiphr filiphr force-pushed the 523-cartesian-array-arguments branch from ca3103e to 2d249de Compare November 14, 2021 08:23
@filiphr
Copy link
Contributor Author

filiphr commented Nov 14, 2021

Just rebased this, most of the commits were from the previous PR. There are indeed updates to the docs.

@nipafx nipafx force-pushed the 523-cartesian-array-arguments branch from d9a9de9 to ab7f318 Compare November 14, 2021 09:52
@nipafx
Copy link
Member

nipafx commented Nov 14, 2021

I gave the generification a shot, but it doesn't add any value. For implementors of the interface (users & us), it adds no extra type safety that they couldn't get from making their class generic and for users of the interface (us), we need to use <?> anyway, so they don't get anything out of it either.

For completeness' sake, here's the diff with that change:

diff --git a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianArgumentsProvider.java b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianArgumentsProvider.java
index 97abaf9..542659a 100644
--- a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianArgumentsProvider.java
+++ b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianArgumentsProvider.java
@@ -24,7 +24,7 @@ import org.junit.jupiter.api.extension.ExtensionContext;
  * @see org.junit.jupiter.params.provider.ArgumentsProvider
  * @see CartesianTestExtension
  */
-public interface CartesianArgumentsProvider {
+public interface CartesianArgumentsProvider<T> {
 
 	/**
 	 * Provider a {@link Stream} of values that needs to be used for a single parameter in {@code @CartesianTest}.
@@ -33,6 +33,6 @@ public interface CartesianArgumentsProvider {
 	 * @param parameter the parameter for which the arguments needs to be provided
 	 * @return a stream of values; never {@code null}
 	 */
-	Stream<?> provideArguments(ExtensionContext context, Parameter parameter) throws Exception;
+	Stream<T> provideArguments(ExtensionContext context, Parameter parameter) throws Exception;
 
 }
diff --git a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianEnumArgumentsProvider.java b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianEnumArgumentsProvider.java
index 086159e..8982424 100644
--- a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianEnumArgumentsProvider.java
+++ b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianEnumArgumentsProvider.java
@@ -30,10 +30,10 @@ import org.junit.platform.commons.support.AnnotationSupport;
  * @implNote This class does not implement {@code ArgumentsProvider} since the Jupiter's {@code EnumSource}
  * should be used for that.
  */
-class CartesianEnumArgumentsProvider implements CartesianArgumentsProvider {
+class CartesianEnumArgumentsProvider<E extends Enum<E>> implements CartesianArgumentsProvider<E> {
 
 	@Override
-	public Stream<? extends Enum<?>> provideArguments(ExtensionContext context, Parameter parameter) {
+	public Stream<E> provideArguments(ExtensionContext context, Parameter parameter) {
 		Class<?> parameterType = parameter.getType();
 		if (!Enum.class.isAssignableFrom(parameterType))
 			throw new PreconditionViolationException(String
@@ -45,7 +45,7 @@ class CartesianEnumArgumentsProvider implements CartesianArgumentsProvider {
 				.orElseThrow(() -> new PreconditionViolationException(
 					"Parameter has to be annotated with " + CartesianTest.Enum.class.getName()));
 
-		Set<? extends Enum<?>> constants = getEnumConstants(enumSource, parameterType);
+		Set<E> constants = getEnumConstants(enumSource, parameterType);
 		CartesianTest.Enum.Mode mode = enumSource.mode();
 		String[] declaredConstantNames = enumSource.names();
 		if (declaredConstantNames.length > 0) {
@@ -59,7 +59,7 @@ class CartesianEnumArgumentsProvider implements CartesianArgumentsProvider {
 		return constants.stream();
 	}
 
-	private <E extends Enum<E>> Set<? extends E> getEnumConstants(CartesianTest.Enum enumSource,
+	private Set<E> getEnumConstants(CartesianTest.Enum enumSource,
 			Class<?> parameterType) {
 		Class<E> enumClass = determineEnumClass(enumSource, parameterType);
 		return EnumSet.allOf(enumClass);
diff --git a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtension.java b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtension.java
index 22a941d..c5fdd73 100644
--- a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtension.java
+++ b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtension.java
@@ -88,9 +88,9 @@ class CartesianTestExtension implements TestTemplateInvocationContextProvider {
 		return sets;
 	}
 
-	private List<Object> getSetFromAnnotation(ExtensionContext context, Annotation source, Parameter parameter) {
+	private List<?> getSetFromAnnotation(ExtensionContext context, Annotation source, Parameter parameter) {
 		try {
-			CartesianArgumentsProvider provider = initializeArgumentsProvider(source, parameter);
+			CartesianArgumentsProvider<?> provider = initializeArgumentsProvider(source, parameter);
 			return provideArguments(context, parameter, provider);
 		}
 		catch (Exception ex) {
@@ -98,7 +98,7 @@ class CartesianTestExtension implements TestTemplateInvocationContextProvider {
 		}
 	}
 
-	private CartesianArgumentsProvider initializeArgumentsProvider(Annotation source, Parameter parameter) {
+	private CartesianArgumentsProvider<?> initializeArgumentsProvider(Annotation source, Parameter parameter) {
 		Optional<CartesianArgumentsSource> cartesianProviderAnnotation = AnnotationSupport
 				.findAnnotation(parameter, CartesianArgumentsSource.class);
 
@@ -121,8 +121,8 @@ class CartesianTestExtension implements TestTemplateInvocationContextProvider {
 				format("%s does not implement the CartesianArgumentsProvider interface.", provider.getClass()));
 	}
 
-	private List<Object> provideArguments(ExtensionContext context, Parameter source,
-			CartesianArgumentsProvider provider) throws Exception {
+	private List<?> provideArguments(ExtensionContext context, Parameter source,
+			CartesianArgumentsProvider<?> provider) throws Exception {
 		return provider
 				.provideArguments(context, source)
 				.distinct()
diff --git a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianValueArgumentsProvider.java b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianValueArgumentsProvider.java
index 71849af..0195d8c 100644
--- a/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianValueArgumentsProvider.java
+++ b/src/main/java/org/junitpioneer/jupiter/cartesian/CartesianValueArgumentsProvider.java
@@ -31,7 +31,7 @@ import org.junit.platform.commons.PreconditionViolationException;
  * @implNote This class does not implement {@code ArgumentsProvider} since the Jupiter's {@code ValueSource}
  * should be used for that.
  */
-class CartesianValueArgumentsProvider implements CartesianArgumentsProvider, AnnotationConsumer<CartesianTest.Values> {
+class CartesianValueArgumentsProvider implements CartesianArgumentsProvider<Object>, AnnotationConsumer<CartesianTest.Values> {
 
 	private Object[] arguments;
 
diff --git a/src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java b/src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java
index 4c3c020..ced1508 100644
--- a/src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java
+++ b/src/main/java/org/junitpioneer/jupiter/params/RangeSourceArgumentsProvider.java
@@ -45,13 +45,13 @@ import org.junitpioneer.jupiter.cartesian.CartesianArgumentsProvider;
  * @see FloatRangeSource
  */
 class RangeSourceArgumentsProvider
-		implements ArgumentsProvider, CartesianAnnotationConsumer<Annotation>, CartesianArgumentsProvider { //NOSONAR deprecated interface use will be removed in later release
+		implements ArgumentsProvider, CartesianAnnotationConsumer<Annotation>, CartesianArgumentsProvider<Number> { //NOSONAR deprecated interface use will be removed in later release
 
 	// Once the CartesianAnnotationConsumer is removed we can make this provider stateless.
 	private Annotation argumentsSource;
 
 	@Override
-	public Stream<? extends Number> provideArguments(ExtensionContext context, Parameter parameter) throws Exception {
+	public Stream<Number> provideArguments(ExtensionContext context, Parameter parameter) throws Exception {
 		initArgumentsSource(parameter);
 		return provideArguments(argumentsSource);
 	}
@@ -66,7 +66,7 @@ class RangeSourceArgumentsProvider
 		return provideArguments(argumentsSource).map(Arguments::of);
 	}
 
-	private Stream<? extends Number> provideArguments(Annotation argumentsSource) throws Exception {
+	private Stream<Number> provideArguments(Annotation argumentsSource) throws Exception {
 		Class<? extends Annotation> argumentsSourceClass = argumentsSource.annotationType();
 		Class<? extends Range> rangeClass = argumentsSourceClass.getAnnotation(RangeClass.class).value();
 
@@ -89,7 +89,7 @@ class RangeSourceArgumentsProvider
 		argumentsSource = argumentsSources.get(0);
 	}
 
-	private Stream<? extends Number> asStream(Range<?> range) {
+	private Stream<Number> asStream(Range<?> range) {
 		return StreamSupport.stream(Spliterators.spliteratorUnknownSize(range, Spliterator.ORDERED), false);
 	}

@nipafx
Copy link
Member

nipafx commented Nov 14, 2021

Proposed commit message:

Remove `Arguments` from `CartesianArgumentsProvider` API (#523 / #528)

Jupiter's `Arguments` interface is used for parameterized tests, where each
`Arguments` instance corresponds to one test execution and thus carries an
`Object[]` with ine argument per parameter. A `CartesianArgumentsProvider`,
on the other hand, provides a set of arguments for a single parameter and
thus there's no need to wrap them in a container. They can simply return the
arguments directly.

As a welcome side effect, this change now allows passing a arrays to a
parameter without accidentally flattening it, which is what #523 was mainly
concerned with.

Closes: #523
PR: #528

@nipafx
Copy link
Member

nipafx commented Nov 14, 2021

After a lot of back and forth, I decided I like the generic solution a tad better after all. Not because it brings a lot to the table, but because it's just "the normal thing to do" and it's a bit weird to let implementors return "whatever".

@nipafx nipafx added the full-build Triggers full build suite on PR label Nov 14, 2021
@nipafx nipafx merged commit 97376ba into junit-pioneer:main Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants