Add dynamic container support in @TestFactory methods #813

sormuras · 2017-04-28T08:55:39Z

Overview

Addresses #695 and #604

Add dynamic container support in @testfactory methods.

https://travis-ci.org/junit-team/junit5/builds/228235319#L596 is generated by:

Stream.of("A", "B", "C")
	.map(input -> dynamicContainer("Container " + input, Stream.of(
		dynamicTest("not null", () -> assertNotNull(input)),
		dynamicContainer("properties", Stream.of(
			dynamicTest("length > 0", () -> assertTrue(input.length() > 0)),
			dynamicTest("not empty", () -> assertFalse(input.isEmpty()))
		))
	)));

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

Definition of Done

There are no TODOs left in the code
Review and polish introduced methods
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
All continuous integration builds pass

marcphilipp

I think this looks great overall! 👍

The fact that this did not require any changes outside of Jupiter tells me that our abstractions work, even though Node.isLeaf() feels a bit fishy.

marcphilipp · 2017-04-28T10:15:14Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/DynamicContainer.java

+		return new DynamicContainer(displayName, StreamSupport.stream(dynamicNodes.spliterator(), false));
+	}
+
+	public static DynamicContainer dynamicContainer(String displayName, Stream<? extends DynamicNode> dynamicNodes) {


I'm wondering if we should make them use Supplier<Iterable<? extends DynamicNode>>/Supplier<Stream<? extends DynamicNode>>. Thoughts?

Every overload that makes usage of dynamicContainer more elegant is good to have.

marcphilipp · 2017-04-28T10:17:06Z

...engine/src/main/java/org/junit/jupiter/engine/descriptor/DynamicContainerTestDescriptor.java

+	private final DynamicContainer dynamicContainer;
+	private final AtomicInteger index;
+
+	DynamicContainerTestDescriptor(UniqueId uniqueId, DynamicContainer dynamicContainer, AtomicInteger index,


AtomicInteger is mutable. I think we should pass the index as an int.

Never mind, if we follow my proposal below we don't need to pass the index at all.

marcphilipp · 2017-04-28T10:18:36Z

...engine/src/main/java/org/junit/jupiter/engine/descriptor/DynamicContainerTestDescriptor.java

+	public JupiterEngineExecutionContext execute(JupiterEngineExecutionContext context,
+			DynamicTestExecutor dynamicTestExecutor) throws Exception {
+		TestSource source = getSource().orElseThrow(AssertionError::new);
+		for (DynamicNode childNode : dynamicContainer.getDynamicNodes()) {


I think every level of the hierarchy should have separate indices starting with 1 on every level.

marcphilipp · 2017-04-28T10:20:02Z

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

@@ -71,11 +74,13 @@ protected void invokeTestMethod(JupiterEngineExecutionContext context, DynamicTe
 			Object instance = testExtensionContext.getTestInstance();
 			Object testFactoryMethodResult = executableInvoker.invoke(getTestMethod(), instance, testExtensionContext,
 				context.getExtensionRegistry());
-
-			try (Stream<DynamicTest> dynamicTestStream = toDynamicTestStream(testFactoryMethodResult)) {
+			TestSource source = getSource().orElseThrow(AssertionError::new);


I know this "cannot happen", but I'd be a little more comfortable with JUnitException::new.

marcphilipp · 2017-04-28T10:21:18Z

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

+			descriptor = new DynamicTestTestDescriptor(uniqueId, test, source);
+		}
+		else {
+			DynamicContainer container = (DynamicContainer) node; // ClassCastException is an error.


Do we need this comment?

marcphilipp · 2017-04-28T10:22:32Z

documentation/src/test/java/example/DynamicTestsDemo.java

+		return nodes;
+	}
+
+	@TestFactory


I don't think this is the best example for the documentation. I'd rather show some real nesting. Thoughts?

Sure. Any real example available?

sbrannen · 2017-04-28T14:18:47Z

@marcphilipp wrote:

I think this looks great overall! 👍

The fact that this did not require any changes outside of Jupiter tells me that our abstractions work, even though Node.isLeaf() feels a bit fishy.

👍

nipafx · 2017-04-30T08:37:16Z

Since I opened #604, @sormuras asked me to comment on this PR. Regarding the hierarchical organization of tests, I think it fits the bill very well. 👍 The other part of #604 is that it talks about an extension point for dynamic tests (like in #371) but the discussion regarding that can happen elsewhere.

@testfactory

Prior to this commit only a flat collection of dynamically generated tests was allowed as the return type of a @testfactory annotated method. The collection entry type was set to `DynamicTest`. This commit introduces the abstract base class `DynamicNode` and a `DynamicContainer` class collecting node instance. @testfactory annotated methods are now allowed to return a collection of nodes -- which can be a test or a named container of tests. Creating a dynamic tree of tests like that gives users the ability to better structure the generated tests. The old behaviour, returning only instances of `DynamicTest`, is preserved.

sormuras · 2017-05-03T11:34:19Z

Team decision: Merge this feature "as-is" and move the open documentation tasks into a new issue.

marcphilipp requested changes Apr 28, 2017

View reviewed changes

sormuras mentioned this pull request Apr 28, 2017

Improve documentation of DynamicTest lifecycle (round 2) #754

Closed

This was referenced Apr 30, 2017

Test Factory Extension #530

Closed

Add support for dynamic test instances #638

Closed

marcphilipp approved these changes May 3, 2017

View reviewed changes

sbrannen added this to the 5.0 M5 milestone May 3, 2017

sbrannen assigned sormuras May 3, 2017

sbrannen added type: enhancement component: Jupiter theme: programming model labels May 3, 2017

sormuras and others added 3 commits May 3, 2017 13:09

Shorten example for dynamicContainer()

e090017

Remove outdated isLeaf() method

4733a4e

sormuras force-pushed the dynamic_node branch from c2378e2 to 4733a4e Compare May 3, 2017 11:32

sormuras mentioned this pull request May 3, 2017

Document DynamicContainer feature #831

Closed

3 tasks

sormuras merged commit 3370ffb into master May 3, 2017

sormuras deleted the dynamic_node branch May 3, 2017 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic container support in @TestFactory methods #813

Add dynamic container support in @TestFactory methods #813

sormuras commented Apr 28, 2017 •

edited

Loading

marcphilipp left a comment

marcphilipp Apr 28, 2017

sormuras Apr 28, 2017

marcphilipp Apr 28, 2017

marcphilipp Apr 28, 2017

marcphilipp Apr 28, 2017

marcphilipp Apr 28, 2017

marcphilipp Apr 28, 2017

marcphilipp Apr 28, 2017

sormuras Apr 28, 2017

sbrannen commented Apr 28, 2017 •

edited

Loading

nipafx commented Apr 30, 2017 •

edited

Loading

sormuras commented May 3, 2017 •

edited

Loading

Add dynamic container support in @TestFactory methods #813

Add dynamic container support in @TestFactory methods #813

Conversation

sormuras commented Apr 28, 2017 • edited Loading

Overview

Definition of Done

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

sbrannen commented Apr 28, 2017 • edited Loading

nipafx commented Apr 30, 2017 • edited Loading

sormuras commented May 3, 2017 • edited Loading

sormuras commented Apr 28, 2017 •

edited

Loading

sbrannen commented Apr 28, 2017 •

edited

Loading

nipafx commented Apr 30, 2017 •

edited

Loading

sormuras commented May 3, 2017 •

edited

Loading