JUnit 4 parameterized support as an extension (#715) #723

LiamClark · 2017-03-11T13:05:48Z

Overview

This allows running parameterized tests from JUnit 4 with Field and Constructor injection, fixes #715.
Fail silently when asked whether we can support the test case,
but fail explicitly with an exception when parameters can't be resolved.

It does this by combining a TestTemplateContextProvider that adds one context for every Object[] in the parameters Collection. These contexts contain an additional extension that performs the field injection if required.

However, when constructor injection is used, there needs to be a parameter resolver in place before the TestTemplateContextProvider is even triggered, therefore the parameter resolver is not an additional extension. Having the parameter resolver on the top level causes us to lose the context of which TemplateContext we were in and this information has to be regained by carefully counting the parameter resolutions. Is there a cleaner way to solve this?

I Store the results of the parameters factory function In the ClassContext store,
so we only call the parameters function once.

The tests are all integration tests, since doing unit tests resulted in unwieldy mocking, however, the amount of 'TestSubjectClasses' needed seems a bit much any idea's to clear this up?

This also ties in with the production code, I tried to keep everything as private as possible, maybe the tests could be simpler if I open up the access modifiers a bit, what would your design goals be here?

I tried to follow the behavior of JUnit 4 as closely as possible are there perhaps any behaviors missing?

Lastly, there is a lot of passing around of ExtensionContexts perhaps extracting a class to hold this could simplify the code a bit.

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

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

gaganis · 2017-03-11T14:20:58Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+			throws ParameterResolutionException {
+		return parameters(extensionContext).map(ArrayList::new).map(l -> l.get(this.parameterCount.get())).filter(
+			params -> params.length == parameterCount).orElseThrow(
+				ParameterizedExtension::unMatchedAmountOfParametersException);


I've had to put quite some effort to understand the code in this class because of the extensive use of lambdas. It is probably me... I am not smart enough or young enough to understand these(... well definitely both) :)

I was especially troubled by the above lambda chain. I think it would have helped me if this.parameterCount was named differently. In this case it has the same name as the int parameterCount. I think a more descriptive name would be something like parametersCollectionSize or parameterArraysCount

@gaganis ah yes definitely, that would be a much clearer name.
I was also a bit unhappy about this one. Let's see what it looks like with the new name, maybe changing some of the lambdas to functions with names might help too

gaganis · 2017-03-11T14:23:15Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+public class ParameterizedExtension implements TestTemplateInvocationContextProvider, ParameterResolver {
+	private static ExtensionContext.Namespace parameters = ExtensionContext.Namespace.create(
+		ParameterizedExtension.class);;
+	private AtomicInteger parameterCount = new AtomicInteger();


Why did you use an AtomicInteger here? Do reads and writes need to be atomic for this?

Thanks

It's mainly because I had this piece of code in the back of my head :
https://github.com/junit-team/junit5/blob/master/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateTestDescriptor.java#L93

but it seems that the AtomicInteger there is used to get around the effectively final rule rather than atomicity.
It seems that resolve is called sequentially so it could just be a normal int.

Hey, I rewrote TestTemplateTestDescriptor in #725 to remove the AtomicInteger. :)

jbduncan

I have a handful of minor suggestions to help make this code readable. :)

jbduncan · 2017-03-11T16:30:46Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+@API(Experimental)
+public class ParameterizedExtension implements TestTemplateInvocationContextProvider, ParameterResolver {
+	private static ExtensionContext.Namespace parameters = ExtensionContext.Namespace.create(
+		ParameterizedExtension.class);;


Two semi-colons? :)

jbduncan · 2017-03-11T16:31:20Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+
+	@Override
+	public Iterator<TestTemplateInvocationContext> provide(ContainerExtensionContext context) {
+		//grabbing the parent ensures the paremeters are stored in the same store.


No space between comment start and "grabbing"?

jbduncan · 2017-03-11T16:31:53Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+		Object[] parameters = resolveParametersForConstructor(extensionContext, parameterCount);
+
+		int parameterIndex = parameterContext.getIndex();
+		//move to the next set of parametersFields


No space between comment start and "move"?

jbduncan · 2017-03-11T16:33:46Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+			throws ParameterResolutionException {
+		return parameters(extensionContext).map(ArrayList::new).map(l -> l.get(this.parametersCollectionIndex)).filter(
+			params -> params.length == parameterCount).orElseThrow(
+				ParameterizedExtension::unMatchedAmountOfParametersException);


Consider using // @formatter:{off|on} to make this more readable:

// @formatter:off return parameters(extensionContext).map(ArrayList::new) .map(l -> l.get(this.parametersCollectionIndex)) .filter(params -> params.length == parameterCount) .orElseThrow(ParameterizedExtension::unMatchedAmountOfParametersException); // @formatter:on

jbduncan · 2017-03-11T16:34:40Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+
+	private static List<Integer> parameterIndexes(List<Field> fields) {
+		return fields.stream().map(f -> f.getAnnotation(Parameterized.Parameter.class)).map(
+			Parameterized.Parameter::value).collect(toList());


Consider using // @formatter:{off|on} here and all the other stream-using methods below.

jbduncan · 2017-03-11T16:38:17Z

...ion-support/src/test/java/org/junit/jupiter/migrationsupport/ParametrizedExtensionTests.java

+			it -> it.getPayload(TestExecutionResult.class)).map(
+				o -> o.flatMap(TestExecutionResult::getThrowable)).filter(Optional::isPresent).map(
+					Optional::get).collect(Collectors.toList());
+	}


Another case where // @formatter:{off|on} would help readability.

jbduncan · 2017-03-11T16:39:25Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+	 *
+	 * @return the object[] for this parameter iteration.
+	 * @throws ParameterResolutionException If the amount of arguments of the constructor doesn't match the amount
+	 * of arguments of the currently resolved object[]


object[] instead of Object[]?

marcphilipp · 2017-03-11T22:38:03Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+	}
+
+	@Override
+	public Iterator<TestTemplateInvocationContext> provide(ContainerExtensionContext context) {


I'm sorry, but I've changed provide to return a Stream in 410b47d which has just been merged into master. Please rebase this pull request and update this method.

@marcphilipp no problem at all, the iterator was being created from a stream anyways. So it actually simplified the code!

LiamClark · 2017-04-04T21:10:17Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+public class ParameterizedExtension implements TestTemplateInvocationContextProvider, ParameterResolver {
+	private static ExtensionContext.Namespace parameters = ExtensionContext.Namespace.create(
+		ParameterizedExtension.class);
+	private int parametersCollectionIndex = 0;


So this index should actually be saved in the store, since the extension is required to be stateless, however before changing that. I'd like to know if there is any way that might avoid this index.

Hmm, instead of implementing ParameterResolver in the extension itself, can you return a ParameterResolver from TestTemplateInvocationContext.getAdditionalExtensions()?

marcphilipp

I like the general direction, though! 👍

marcphilipp · 2017-04-09T12:47:29Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+public class ParameterizedExtension implements TestTemplateInvocationContextProvider, ParameterResolver {
+	private static ExtensionContext.Namespace parameters = ExtensionContext.Namespace.create(
+		ParameterizedExtension.class);
+	private int parametersCollectionIndex = 0;


Hmm, instead of implementing ParameterResolver in the extension itself, can you return a ParameterResolver from TestTemplateInvocationContext.getAdditionalExtensions()?

marcphilipp · 2017-04-09T12:47:57Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+		};
+	}
+
+	private static class InjectionExtension implements BeforeTestExecutionCallback {


Shouldn't this be a TestInstancePostProcessor instead?

marcphilipp · 2017-04-09T12:49:01Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+
+package org.junit.jupiter.migrationsupport.rules;


This should reside in a different package. How about org.junit.jupiter.migrationsupport.parameterized?

marcphilipp · 2017-04-09T12:53:09Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+
+	private static Optional<Collection<Object[]>> parameters(ExtensionContext context) {
+		return context.getStore(parameters).getOrComputeIfAbsent("parameterMethod",
+			k -> new ParameterWrapper(callParameters(context)), ParameterWrapper.class).getValue();


There should be no need to store the parameters if we make all extensions "local" and handle them in provide().

@marcphilipp It is moved into provide now :), but if there are multiple test templates in the class, it would still trigger the parameters method twice.

However is calling the parameters method once something we care about?

You sure? You already store it in the parent context's Store but only if it's not already there, don't you? Can you add a test for that?

@marcphilipp Yes, I already did here https://github.com/junit-team/junit5/pull/723/files#diff-c8fe8d61d6bba040fc43c22d9db3f16aR291 (Looking at it now it could definitely have a better name)

So in the current implementation the parameters method is indeed only called once, but it does still require storing them in the store. My comment was intended to show that even though the extensions are now local, I still need to store them for this behaviour.

Yeah, ok, I think that's acceptable in order to preserve the old behavior. 👍

marcphilipp · 2017-04-09T12:56:43Z

...n-support/src/main/java/org/junit/jupiter/migrationsupport/rules/ParameterizedExtension.java

+			return (Collection<Object[]>) o;
+		}
+		else {
+			throw wrongParametersReturnType();


Parameters methods may also return an Iterable or an Object[] (cf. https://github.com/junit-team/junit4/blob/master/src/main/java/org/junit/runners/Parameterized.java#L288-L297). You may want to use CollectionUtils.toStream(o) even though it supports additional types.

@marcphilipp is this only for the single parameter case or also in general?

LiamClark · 2017-04-09T15:25:34Z

@marcphilipp The reason the parameter resolver is not provided through the additional extensions, is because:

However, when constructor injection is used, there needs to be a parameter resolver in place before the TestTemplateContextProvider is even triggered, therefore the parameter resolver is not an additional extension. Having the parameter resolver on the top level causes us to lose the context of which TemplateContext we were in and this information has to be regained by carefully counting the parameter resolutions. Is there a cleaner way to solve this?

If no parameter resolver is present for the first constructor the test fails before the test template can even be considered. Perhaps we can resolve only the first constructor invocation from the top level parameter resolver and supply the rest of the parameters through the additional extensions?

marcphilipp · 2017-04-09T16:11:13Z

@LiamClark Sorry for not re-reading your comment properly. I now remember reading your comment when you opened the pull request. 😳

It sounds like you've actually tried returning the ParameterResolver from getAdditionalExtensions(), right?

LiamClark · 2017-04-09T16:28:15Z

@marcphilipp No problem!

I didn't exactly try to return the parameter resolver from getAdditionalExtensions(). It was indeed my original idea, but this problem showed it self simply trying to get the extension callback to trigger.

It of course would be the nicest spot to resolve the parameters in, but it seems like the mismatch between Junit4 and Jupiter is slightly to big.

marcphilipp · 2017-04-09T16:29:12Z

I'll look into it and get back to you.

marcphilipp · 2017-04-09T18:15:48Z

Although I was quite sure that this should already be supported, I've now added a test that verifies that in bae2768: A ParameterResolver returned by a TestTemplateInvocationContext may be used to resolve constructor arguments.

@LiamClark Can you try to change the code in this PR accordingly?

marcphilipp · 2017-04-11T07:31:25Z

...t/src/main/java/org/junit/jupiter/migrationsupport/parameterized/ParameterizedExtension.java

+
+@API(Experimental)
+public class ParameterizedExtension implements TestTemplateInvocationContextProvider {
+	private static ExtensionContext.Namespace parameters = ExtensionContext.Namespace.create(


This can also be final and then follow the naming convention for constants, right?

sbrannen · 2018-01-09T15:11:40Z

Hi @LiamClark,

Just wondering what the status of this effort is.

Can you shed any light on the subject?

marcphilipp · 2018-01-09T15:13:48Z

I think this would be a perfect use case for container templates.

sbrannen · 2018-01-09T15:17:26Z

I think this would be a perfect use case for container templates.

Yep... could well be!

LiamClark · 2018-01-09T18:51:18Z

So I believe all feedback in the last set of requested changes was actually implemented.

So mainly it would be adding it to the user guide, if you guys still want it.
It is however based on a now somewhat dated version of master.

I'd be willing to do the rebase and fix the code or if you want to explore the possibilities to use container templates for this, I have some time the next few weeks to do that as well.

let me know!

sbrannen · 2018-01-10T16:23:44Z

So I believe all feedback in the last set of requested changes was actually implemented.

OK. Thanks for the feedback.

I'd be willing to do the rebase and fix the code or if you want to explore the possibilities to use container templates for this, I have some time the next few weeks to do that as well.

Well, container templates don't exist yet. So let's hold off on rebasing/reworking until we make progress on container templates.

This allows for Field and Constructor injection. Fail silently when asked whether we can support the test case, but fail explicitly with an exception when parameters can't be resolved. Store the results of the parameters factory function In the ClassContext store

This also includes support for single parameter tests.

codecov · 2018-02-13T20:53:24Z

Codecov Report

Merging #723 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #723      +/-   ##
============================================
- Coverage     92.01%   91.99%   -0.03%     
+ Complexity     3252     3159      -93     
============================================
  Files           299      296       -3     
  Lines          7779     7666     -113     
  Branches        670      626      -44     
============================================
- Hits           7158     7052     -106     
+ Misses          466      447      -19     
- Partials        155      167      +12

Impacted Files	Coverage Δ	Complexity Δ
...atform/engine/support/descriptor/FilePosition.java	`73.07% <0%> (-22.58%)`	`13% <0%> (-11%)`
...ne/support/descriptor/ClasspathResourceSource.java	`75% <0%> (-20.84%)`	`12% <0%> (-2%)`
...c/main/java/org/junit/jupiter/api/DynamicNode.java	`80% <0%> (-20%)`	`2% <0%> (-2%)`
...platform/engine/support/descriptor/FileSource.java	`73.07% <0%> (-15.39%)`	`13% <0%> (-1%)`
...it/platform/commons/support/AnnotationSupport.java	`85.71% <0%> (-14.29%)`	`5% <0%> (-2%)`
...engine/support/descriptor/CompositeTestSource.java	`86.66% <0%> (-13.34%)`	`7% <0%> (-2%)`
...tform/engine/support/descriptor/PackageSource.java	`87.5% <0%> (-12.5%)`	`9% <0%> (-2%)`
...ter/params/ParameterizedTestParameterResolver.java	`87.5% <0%> (-9.87%)`	`7% <0%> (-9%)`
...ter/params/converter/DefaultArgumentConverter.java	`78.66% <0%> (-8%)`	`8% <0%> (-2%)`
...unit/platform/engine/discovery/MethodSelector.java	`78% <0%> (-8%)`	`15% <0%> (-2%)`
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 948841c...99d7f47. Read the comment docs.

marcphilipp · 2019-04-14T13:28:45Z

We're cleaning out the issue tracker and closing PRs for issues that we've not seen much demand to fix. Feel free to comment with additional justifications if you feel that this one should not have been closed.

LiamClark force-pushed the parameterized-migration-support branch from f0cc940 to 97bbf3f Compare March 11, 2017 13:10

gaganis reviewed Mar 11, 2017

View reviewed changes

LiamClark force-pushed the parameterized-migration-support branch from 97bbf3f to c4a5c67 Compare March 11, 2017 14:41

jbduncan reviewed Mar 11, 2017

View reviewed changes

marcphilipp reviewed Mar 11, 2017

View reviewed changes

LiamClark force-pushed the parameterized-migration-support branch from b875b29 to e8ade4b Compare March 11, 2017 22:49

gaganis mentioned this pull request Mar 12, 2017

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

Closed

7 tasks

LiamClark commented Apr 4, 2017

View reviewed changes

marcphilipp requested changes Apr 9, 2017

View reviewed changes

LiamClark force-pushed the parameterized-migration-support branch from e8ade4b to 90ba9e7 Compare April 9, 2017 15:11

LiamClark force-pushed the parameterized-migration-support branch 2 times, most recently from ca6df6c to c465613 Compare April 10, 2017 15:13

marcphilipp reviewed Apr 11, 2017

View reviewed changes

LiamClark force-pushed the parameterized-migration-support branch from c465613 to 26d5fa7 Compare April 11, 2017 13:01

sbrannen added component: Jupiter theme: extensions theme: programming model type: enhancement labels Aug 19, 2017

sbrannen added this to the General Backlog milestone Aug 19, 2017

marcphilipp mentioned this pull request Jan 9, 2018

Introduce extension API for container templates #871

Open

LiamClark added 7 commits February 13, 2018 11:23

Convert parametersCollection index to regular int

28cc2e5

Custom formatting for stream methods

bdefd20

make test-template provide return a Stream

45f6605

Move to migrationsupport.parameterized package

1ddc46d

Move parameter resolver into to the test template

c24740d

support Iterables and Arrays in the ParameterizedExtension

99d7f47

This also includes support for single parameter tests.

LiamClark force-pushed the parameterized-migration-support branch from 26d5fa7 to 99d7f47 Compare February 13, 2018 20:37

marcphilipp closed this Apr 14, 2019

marcphilipp removed this from the General Backlog milestone May 4, 2019

sbrannen mentioned this pull request Sep 5, 2019

Introduce migration support for JUnit 4's Parameterized Runner #715

Closed

3 tasks

JUnit 4 parameterized support as an extension (#715) #723

JUnit 4 parameterized support as an extension (#715) #723

Conversation

LiamClark commented Mar 11, 2017 • edited Loading

Overview

Definition of Done

Choose a reason for hiding this comment

LiamClark Mar 11, 2017 • edited Loading

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

jbduncan left a 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

marcphilipp left a 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

LiamClark commented Apr 9, 2017

marcphilipp commented Apr 9, 2017

LiamClark commented Apr 9, 2017

marcphilipp commented Apr 9, 2017

marcphilipp commented Apr 9, 2017

Choose a reason for hiding this comment

sbrannen commented Jan 9, 2018

marcphilipp commented Jan 9, 2018

sbrannen commented Jan 9, 2018

LiamClark commented Jan 9, 2018

sbrannen commented Jan 10, 2018

codecov bot commented Feb 13, 2018 • edited Loading

Codecov Report

marcphilipp commented Apr 14, 2019

LiamClark commented Mar 11, 2017 •

edited

Loading

LiamClark Mar 11, 2017 •

edited

Loading

codecov bot commented Feb 13, 2018 •

edited

Loading