Introduce junit-jupiter-params #728

marcphilipp · 2017-03-11T21:29:51Z

Overview

This pull request adds a new module junit-jupiter-params as discussed in the last team call. See ParamsApiPlayground for examples.

This branch is still lacking Javadoc, API annotations, documentation, and, in some cases, unit tests. We've decided to merge it into master now to reduce conflicts with other pull requests and other work. The remaining tasks will be addressed in #14.

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

Definition of Done

~~There are no TODOs left in the code~~ There are TODOs that will be addressed as part of Introduce support for parameterized tests #14.
~~Method preconditions are checked and documented in the method's Javadoc~~ Javadoc will be added as part of Introduce support for parameterized tests #14.
Coding conventions (e.g. for logging) have been followed
~~Change is covered by automated tests~~ A few classes that I've extracted from ParameterizedTestExtension still lack proper unit tests.
~~Public API has Javadoc and @API annotations~~ Will be added as part of Introduce support for parameterized tests #14.
~~Change is documented in the User Guide and Release Notes~~ Will be added as part of Introduce support for parameterized tests #14.
All continuous integration builds pass

While Iterators cannot be closed, Streams can. Since providers now return Streams we close them after consumption. Thus, providers may now read directly from files etc. and can register a hook using Stream.onClose() to close their resources.

Streams are already implement AutoCloseable so there’s no need to implement our own mechanism to close ArgumentsProviders or the Iterators they used to return.

sbrannen · 2017-03-12T11:10:27Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTest.java

+@ExtendWith(ParameterizedTestExtension.class)
+public @interface ParameterizedTest {
+
+	String name() default "[{index}] {arguments}";


I am not in favor of introducing a name attribute in @ParameterizedTest since we decided against doing that for @Test, @Step, etc.

@DisplayName would be more natural and, more importantly, consistent.

The only issue is determining when the above default should be used for parameterized tests. Maybe that logic could be built into the ParameterizedTestExtension, by looking for a DisplayName annotation instead of the ParameterizedTest. If @DisplayName is not present, you could then use "[{index}] {arguments}" as the default.

I thought about using @DisplayName. But then the container descriptor for the method would also get that display name. IMHO that doesn't make sense so I decided to do it differently, in a way like we do for TestFactory methods.

Aha.... the container name. I see.

Makes sense now.

sbrannen · 2017-03-12T11:18:42Z

...upiter-params/src/main/java/org/junit/jupiter/params/ParameterizedTestParameterResolver.java

+
+	@Override
+	public boolean supports(ParameterContext parameterContext, ExtensionContext extensionContext) {
+		return parameterContext.getIndex() < arguments.length;


How is this intended to be used in conjunction with other parameter resolvers (e.g., mocks, DI)?

Are "parameters" required to come first as arguments, and then it's open game for other resolvers to contribute?

Yes, for now we would require them to be the first arguments. We could later introduce an annotation, e.g. @Parameter with an optional index value to mark them explicitly.

OK. We'll just have to make sure to properly document that behavior.

sbrannen · 2017-03-12T11:20:57Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/converter/ArgumentConverter.java

+
+import org.junit.jupiter.api.extension.ParameterContext;
+
+public interface ArgumentConverter {


I'd still like to see this declared with generics <S, T> (for source and target) even if it only serves to improve readability for the time being.

What would we S and T for JavaTimeArgumentConverter and DefaultArgumentConverter? Both Object?

If we add generics we will have to use unchecked calls to the converter and make sure we catch resulting ClassCastExceptions in ParameterizedTestParameterResolver. I'm not sure that makes it more readable.

I was thinking as an idealist for what I think a Converter API should look like, but now that I've looked at the code in the IDE... I agree with you: it's best to leave off the generics in this particular case.

sbrannen · 2017-03-12T11:23:04Z

...upiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java

+				+ input.getClass().getName() + " to type " + targetClass.getName());
+	}
+
+	interface StringConversion {


Please rename all *Conversion classes to *Converter.

Changed locally.

sbrannen · 2017-03-12T11:24:08Z

...upiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java

+
+	interface StringConversion {
+
+		boolean isResponsible(Class<?> targetClass);


Please rename to canConvert.

Changed locally.

sbrannen · 2017-03-12T12:20:31Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/support/AnnotationInitializer.java

+					&& Annotation.class.isAssignableFrom(method.getParameterTypes()[0]);
+			Method method = ReflectionUtils.findMethods(instance.getClass(), methodPredicate,
+				HierarchyTraversalMode.BOTTOM_UP).get(0);
+			Class<? extends Annotation> annotationType = (Class<? extends Annotation>) method.getParameterTypes()[0];


Move the declaration of annotationType higher up to avoid breaking the DRY principle.

Never mind: that made zero sense.

sbrannen · 2017-03-12T12:21:21Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/support/AnnotationInitializer.java

+			instance.accept(annotation);
+		}
+		catch (Exception ex) {
+			throw new JUnitException("Failed to initialize instance: " + instance, ex);


Can we come up with a better term than instance in this context, something more meaningful?

Changed locally.

sbrannen · 2017-03-12T12:21:49Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/support/package-info.java

@@ -0,0 +1,8 @@
+/**
+ * Support classes for building


For building argument...

Changed locally.

sbrannen · 2017-03-12T12:24:41Z

junit-platform-commons/src/main/java/org/junit/platform/commons/util/CollectionUtils.java

@@ -78,4 +84,31 @@ private CollectionUtils() {
 		return collectingAndThen(toList(), Collections::unmodifiableList);
 	}

+	/**
+	 * Convert a well-known object into a {@code Stream}.


Please document which well-known object types are supported.

sbrannen · 2017-03-12T12:25:31Z

junit-platform-commons/src/main/java/org/junit/platform/commons/util/CollectionUtils.java

+			return Arrays.stream((Object[]) object);
+		}
+		throw new PreconditionViolationException(
+			"Cannot convert instance of " + object.getClass().getName() + " into a Stream: " + object);


And... if object is null... Boom! 💥

Fixed locally.

Good catch! 😳

sbrannen · 2017-03-12T12:27:56Z

@marcphilipp, thanks for all of the hard work on this!

Looks really nice, and I'm looking forward to seeing it released in M4.

I've made a some minor suggestions for improvements, but otherwise I think it looks good (at least from within the browser 😉).

LiamClark · 2017-03-12T12:32:19Z

...iter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestFactoryTestDescriptor.java

@@ -88,33 +84,20 @@ protected void invokeTestMethod(JupiterEngineExecutionContext context, DynamicTe
 					dynamicTest -> registerAndExecute(dynamicTest, index.incrementAndGet(), dynamicTestExecutor));
 			}
 			catch (ClassCastException ex) {
-				throw invalidReturnTypeException(testExtensionContext);
+				throw invalidReturnTypeException(ex);
 			}
 		});
 	}

 	@SuppressWarnings("unchecked")
 	private Stream<DynamicTest> toDynamicTestStream(TestExtensionContext testExtensionContext,


If you guys don't mind me chiming in.
testExtensionContext now seems to be an unused argument. 😄

It is still used by executableInvoker.invoke but it might be slightly better if it was moved inside the lambda's scope.

I actually mean the TestExtensionContext in toDynamicTestStream but the comment is placed a but akwardly on the diff.

@LiamClark, good catch. I'll remove that unused argument.

marcphilipp and others added 30 commits March 11, 2017 22:09

First draft of params API

2bd6f1a

Executes ParameterizedTest for simple ArgumentsSources

964135f

Add first argument to display name

576865c

Add integration test for single @StringSource

1126a5b

Add support for custom display names

aa04ab0

Add EnumSource with provider and example

f359f06

Make custom @ArgumentsSource annotations non-repeatable for now

9f1e5dd

Prototypes of CSV-based and method ArgumentProviders

b94c8d2

Change ArgumentsProvider to return Stream

94359ee

Streams are already implement AutoCloseable so there’s no need to implement our own mechanism to close ArgumentsProviders or the Iterators they used to return.

Unit tests for ArgumentsProviders

7bfdf00

Extract AnnotationInitializer

2094e45

Enhance EnumSource with constant name filter feature

111ba0a

Rename StringSource and FileSource to clarify they accept CSV input

a3dda20

Introduce @valuesource

819579c

First step to support explicit ArgumentConverters

cb334a1

DefaultArgumentConverter is aware of wrapper types for primitives

9d1f4ee

Add implicit conversions for primitives, enums, and java.time types

19a2135

Rename sources package to provider

afacc61

Reorganize classes into packages: providers, converters, and support

56b1e83

Convert inner classes to top-level classes

56c4553

Use single quotes as escape character for @CsvSource

f34b3ac

Support multiple methods in @MethodSource

8a91d95

Change @CsvFileSource to use classpath resources

6cb6215

Add support for multiple classpath resources to @CsvFileSource

5041f0e

More unit tests

5fbd987

Re-use ReflectionUtils.getWrapperType

644b836

Introduce CollectionUtils.toStream to remove duplication

ac58f48

Support Object arrays in CollectionUtils.toStream

db627a1

Rename AnnotationInitialized to AnnotationConsumer

18cb93a

sormuras approved these changes Mar 11, 2017

View reviewed changes

elygre mentioned this pull request Mar 11, 2017

Introduce support for parameterized tests #14

Closed

16 tasks

marcphilipp merged commit ca24ac4 into master Mar 11, 2017

marcphilipp deleted the issues/14-jupiter-params branch March 11, 2017 21:41

marcphilipp added this to the 5.0 M4 milestone Mar 11, 2017

marcphilipp mentioned this pull request Mar 11, 2017

Replace AtomicInteger with int in TestTemplateTestDescriptor.execute() #725

Closed

7 tasks

sbrannen added type: enhancement component: Jupiter theme: programming model labels Mar 12, 2017

sbrannen reviewed Mar 12, 2017

View reviewed changes

LiamClark reviewed Mar 12, 2017

View reviewed changes

nipafx mentioned this pull request May 22, 2017

Remove separate artifact for parameterized tests #858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce junit-jupiter-params #728

Introduce junit-jupiter-params #728

marcphilipp commented Mar 11, 2017 •

edited

Loading

sbrannen Mar 12, 2017

marcphilipp Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

marcphilipp Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

marcphilipp Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

sbrannen Mar 12, 2017

marcphilipp Mar 12, 2017

sbrannen commented Mar 12, 2017

LiamClark Mar 12, 2017

gaganis Mar 12, 2017

LiamClark Mar 12, 2017

sbrannen Mar 12, 2017


		import org.junit.jupiter.api.extension.ParameterContext;

		public interface ArgumentConverter {


		interface StringConversion {

		boolean isResponsible(Class<?> targetClass);

Introduce junit-jupiter-params #728

Introduce junit-jupiter-params #728

Conversation

marcphilipp commented Mar 11, 2017 • edited Loading

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbrannen commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp commented Mar 11, 2017 •

edited

Loading