Add support for parallel test execution #1461

marcphilipp · 2018-06-08T15:24:14Z

This PR adds support for parallel test execution (#60). Please refer to the added user guide sections for details.

The majority of this work was done by @leonard84 and me over the course of the last few months. I intend to squash the commits before merging this PR. For reviewing, I've decided to keep them in place so it's easier to see why a change was made.

If you want to see it in action, just execute SlowTests in an IDE (tested in IntelliJ IDEA).

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

Thread-safety is achieved by using a synchronized map. `TreePrinter` now resorts to `Color.NONE` if no color mapping is specified for the current identifier.

# Conflicts: # junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java

@leonard84

Result of pair programming session with @leonard84. Issue: #60

Issue: #60

Improve CompositeLock to keep track of acquired locks, release only those. Use lockInterruptably to support clean shutdown while waiting for locks. Add basic support for ExecutionControl/Mode

# Conflicts: # junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/JupiterTestDescriptor.java # junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ExecutionTracker.java # platform-tests/src/test/java/org/junit/platform/console/tasks/XmlReportAssertions.java

- Limit max. number of threads - Set minimum runnable number of threads to parallelism

Co-authored-by: Leonard Brünings <[email protected]>

# Conflicts: # junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java

Co-authored-by: Leonard Brünings <[email protected]>

codecov · 2018-06-08T15:36:29Z

Codecov Report

Merging #1461 into master will increase coverage by 0.17%.
The diff coverage is 91.3%.

@@             Coverage Diff             @@
##             master   #1461      +/-   ##
===========================================
+ Coverage     91.73%   91.9%   +0.17%     
- Complexity     3214    3487     +273     
===========================================
  Files           297     317      +20     
  Lines          7740    8310     +570     
  Branches        662     723      +61     
===========================================
+ Hits           7100    7637     +537     
- Misses          477     503      +26     
- Partials        163     170       +7

Impacted Files	Coverage Δ	Complexity Δ
...jupiter/engine/descriptor/ClassTestDescriptor.java	`99.15% <100%> (+0.01%)`	`48 <2> (+2)`	⬆️
...r/engine/descriptor/DynamicNodeTestDescriptor.java	`100% <100%> (ø)`	`5 <1> (+1)`	⬆️
...tform/engine/support/hierarchical/LockManager.java	`100% <100%> (ø)`	`11 <11> (?)`
...junit/platform/engine/ConfigurationParameters.java	`100% <100%> (ø)`	`2 <2> (?)`
.../core/StreamInterceptingTestExecutionListener.java	`100% <100%> (ø)`	`16 <16> (?)`
...t/platform/console/tasks/TreePrintingListener.java	`100% <100%> (ø)`	`9 <9> (+2)`	⬆️
...form/engine/support/hierarchical/ResourceLock.java	`100% <100%> (ø)`	`1 <1> (?)`
...rchical/DefaultParallelExecutionConfiguration.java	`100% <100%> (ø)`	`6 <6> (?)`
...support/hierarchical/HierarchicalTestExecutor.java	`100% <100%> (+4.59%)`	`3 <3> (-1)`	⬇️
.../platform/engine/support/hierarchical/NopLock.java	`100% <100%> (ø)`	`3 <3> (?)`
... and 56 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 5d1e6ef...61441bb. Read the comment docs.

…l-execution

sbrannen · 2018-06-12T10:21:46Z

documentation/src/docs/asciidoc/user-guide/running-tests.adoc

+
+Since version 1.3, the JUnit Platform provides opt-in support for capturing output
+printed to `System.out` and `System.err`. To enable it, simply set the
+`junit.platform.launcher.capture.stdout` and/or `junit.platform.launcher.capture.stderr`


I think the prefix should be junit.platform.launcher.output.capture or junit.platform.output.capture.

sbrannen · 2018-06-12T10:38:03Z

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

@@ -30,6 +32,11 @@
 		this.index = index;
 	}

+	@Override
+	public ExecutionMode getExecutionMode() {
+		return getParent().map(parent -> ((JupiterTestDescriptor) parent).getExecutionMode()).orElse(CONCURRENT);


Will Jupiter now default to concurrent execution?

sbrannen · 2018-06-12T10:38:20Z

...-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/JupiterTestDescriptor.java

+						.filter(parent -> parent instanceof Node)
+						.map(parent -> ((Node<?>) parent).getExecutionMode())
+						.orElse(ExecutionMode.CONCURRENT));
+		// @formatter:on


Will Jupiter now default to concurrent execution?

Only if the junit.jupiter.execution.parallel.enabled config param is set to true.

OK. In that code it looks like it's defaulting to concurrent, so I'll have to review in greater depth to find where the flag is honored.

Just have a look at JupiterTestEngine. 😉

Alrighty. Will do...

sbrannen · 2018-06-12T10:45:37Z

junit-platform-engine/src/main/java/org/junit/platform/engine/ConfigurationParameters.java

+	 *
+	 * @param key the key to look up; never {@code null} or blank
+	 * @param transformer the transformer to apply in case a value is found;
+	 * never {@code null} or blank


remove "or blank" for the transformer

sbrannen · 2018-06-12T10:47:24Z

junit-platform-engine/src/main/java/org/junit/platform/engine/ConfigurationParameters.java

+				return transformer.apply(input);
+			}
+			catch (Exception ex) {
+				String message = String.format("Failed to convert configuration parameter with key '%s' for input: %s",


--> Failed to transform configuration parameter with key '%s' and initial value '%s'

sbrannen · 2018-06-12T10:48:03Z

.../src/main/java/org/junit/platform/engine/support/config/PrefixedConfigurationParameters.java

+	 * applies the supplied prefix to all queries.
+	 */
+	public PrefixedConfigurationParameters(ConfigurationParameters delegate, String prefix) {
+		this.delegate = delegate;


Needs precondition checks

sbrannen · 2018-06-12T11:20:02Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/parallel/UseResource.java

+@Retention(RetentionPolicy.RUNTIME)
+@Target({ ElementType.TYPE, ElementType.METHOD })
+@Repeatable(UseResources.class)
+public @interface UseResource {


Wouldn't @ResourceLock be a better suited name?

Possibly... let's discuss it in the next team call, shall we?

Isn't the fact that it is currently implemented via a lock an implementation detail? The idea of @UseResource was so that the tests could declare what resources they use and what guarantees they expect/observe. If we change the implementation to use something else than a Lock then @ResourceLock wouldn't make sense.

I think this is one of those scenarios where an implementation detail leaks to higher levels of the overall abstraction simply because it's part of the high-level use case.

To me, @UseResource is too generic in that it does not immediately provide a mental link to the concurrency model in question. After all, we're talking about executing things in parallel. So there is going to have to be some sort of locking in order to avoid contention -- right?

On the other hand, I understand your desire not to leak implementation details, but I'm still not fond of @UseResource. 😉

In terms of consistency, IIRC, the only places we use "verbs" for annotation names are for @ExtendWith and @RegisterExtension.

Otherwise, we try to avoid using verbs for annotation names.

Possibly... let's discuss it in the next team call, shall we?

If I'm there... gladly.

If people don't like @ResourceLock, I think that @SharedResource would at least be better than @UseResource.

Rationale: the developer is not instructing the framework to use a resource for the test. Rather, the developer is instructing the framework that the test itself uses a shared resource, to which the framework should then synchronize access.

FWIW, the demo test class is currently called SharedResourcesDemo, not UseResourcesDemo. 😉

I have to agree with @sbrannen that @UseResource is not a good name for this. @ResourceLock would make more sense to me.

Adding the Lock concept does help me to understand the underlying mental(concurrency) model.

When seeing for the first time @UseResource for some reason I imagined that this annotation would be added to some resource ie some critical piece of code eg a method, that would be shared and the framework would orchestrate tests around it. [In retrospect, feeling a bit stupid now for thinking that.]

After reviewing the code I think the important point of this mechanism is the exclusivity of execution of tests, ie these tests characterized as readers can execute at the same time while tests characterized as writers can only run by themselves.

The concept of a lock suggests the above quite well for me.

marcphilipp · 2018-06-12T16:29:43Z

I just pushed two additional commits that address most of the feedback. Only remaining issue is the name of @UseResource.

sbrannen · 2018-06-13T13:55:32Z

I approved the changes requested thus far and agree that we still need to decide on the name of @UseResource.

If I have time, I'll review the internals in greater detail.

gaganis · 2018-06-14T10:33:55Z

.../junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java

+				false, configuration.getCorePoolSize(), configuration.getMaxPoolSize(),
+				configuration.getMinimumRunnable(), null, configuration.getKeepAlive(), TimeUnit.SECONDS);
+		}
+		catch (Exception e) {


Wouldn't catching the all inclusive Exception here also mask actually crashes in the try part of this code, eg NPEs?

All ParallelExecutionConfiguration methods return primitive types thus I don't see where a NPE should come from. Am I missing something?

leonard84 · 2018-06-12T17:06:57Z

junit-platform-commons/src/main/java/org/junit/platform/commons/logging/LogRecordListener.java

@@ -30,7 +30,7 @@
 @API(status = INTERNAL, since = "1.1")
 public class LogRecordListener {

-	private final List<LogRecord> logRecords = new ArrayList<>();
+	private final List<LogRecord> logRecords = new CopyOnWriteArrayList<>();


CopyOnWriteArrayList is expensive to update, why not use a ThreadLocal<List<LogRecord>>?

Changed in 650fa52.

leonard84 · 2018-06-12T17:17:07Z

.../junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java

+		for (ExclusiveTask task : nonConcurrentTasks) {
+			task.compute();
+		}
+		for (ExclusiveTask forkedTask : concurrentTasksInReverseOrder) {


Although inline comments should be used sparingly it might be helpful here to explain a few steps.

I extracted three methods in 4bf6044 instead. Please let me know if that's easier to understand now.

leonard84 · 2018-06-14T17:30:22Z

...m-launcher/src/main/java/org/junit/platform/launcher/core/TestExecutionListenerRegistry.java

@@ -94,4 +109,12 @@ public void reportingEntryPublished(TestIdentifier testIdentifier, ReportEntry e

 	}

+	interface EagerTestExecutionListener extends TestExecutionListener {


What are eager EagerTestExecutionListener? Shouldn't we add ordering instead?

For now this is a one-off internal scenario. I found this easier to understand and less error-prone than adding ordering support.

leonard84 · 2018-06-14T17:32:46Z

platform-tests/src/test/java/org/junit/platform/console/tasks/XmlReportAssertions.java

@@ -26,24 +28,27 @@
 */
 class XmlReportAssertions {

-	private static Validator schemaValidator;
+	private static AtomicReference<Schema> schema = new AtomicReference<>();


Do you really need an AtomicReference why not just use getSchema as initializer?

It's surprisingly hard to implement a thread-safe singleton without locking. If you have a better solution, I'm open to change it. 😉

Did someone say thread-safe singleton? :)

Would Effective Java's recommended solution - a single-instance enum - work in this case? E.g.

enum ClassName implements AppropriateInterface { INSTANCE; // needed methods go here }

I had tried that at some point but it felt clunky. Revisiting it now, it's not actually that bad so I changed it in 135a07d.

@jbduncan Thanks! 🙂

@marcphilipp You're very welcome! 😃

leonard84 · 2018-06-14T17:36:09Z

...m-tests/src/test/java/org/junit/platform/engine/support/hierarchical/CompositeLockTests.java

+	}
+
+	@Test
+	void releasesAllLocksInReverseOrder() throws Exception {


A test for acquire inOrder is missing.

Added in ba7fb76.

leonard84 · 2018-06-14T17:40:12Z

...orm-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java

+	void returnsNopLockWithoutExclusiveResources() {
+		Collection<ExclusiveResource> resources = emptySet();
+
+		List<Lock> locks = getLocks(lockManager, resources);


This does not assert that a NopLock was returned, same goes for the other tests.

Improved in 4b40b64.

leonard84 · 2018-06-14T17:41:46Z

...orm-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java

+	}
+
+	@Test
+	void returnsSingleLockForExclusiveResourcWithBothLockModes() {


returnsSingleLockForExclusiveResourcWithBothLockModes + UsingStrongestLockMode

Renamed to returnsWriteLockForExclusiveResourceWithBothLockModes in 4b40b64.

marcphilipp · 2018-06-22T16:49:06Z

Team Decision: Use @ResourceLock for M1 and ask the community for feedback.

sormuras · 2018-06-22T17:54:52Z

Wohoo!

This commit adds opt-in support for parallel test execution and capturing output to `System.out` and `System.err`. Both features are disabled by default but can be enabled and configured using configuration parameters. The implementation is based on the Fork/Join Framework and designed to be reusable by other test engines that extend HierarchicalTestEngine. The Jupiter API provides annotations to declare which shared resources a test needs to access and in which way. Moreover, the execution mode of a test can be influenced. In addition, a number of TestExecutionListeners have been made thread-safe. The documentation subproject is now configured to execute tests in parallel. All other subprojects will have to wait as Gradle currently blows up when used with parallel test execution. Resolves #60. Closes #1461. Co-authored-by: Leonard Brünings <[email protected]> Co-authored-by: Christian Stein <[email protected]>

sbrannen · 2018-06-22T18:24:19Z

👏

This commit adds opt-in support for parallel test execution and capturing output to `System.out` and `System.err`. Both features are disabled by default but can be enabled and configured using configuration parameters. The implementation is based on the Fork/Join Framework and designed to be reusable by other test engines that extend HierarchicalTestEngine. The Jupiter API provides annotations to declare which shared resources a test needs to access and in which way. Moreover, the execution mode of a test can be influenced. In addition, a number of TestExecutionListeners have been made thread-safe. The documentation subproject is now configured to execute tests in parallel. All other subprojects will have to wait as Gradle currently blows up when used with parallel test execution. Resolves junit-team#60. Closes junit-team#1461. Co-authored-by: Leonard Brünings <[email protected]> Co-authored-by: Christian Stein <[email protected]>

marcphilipp and others added 30 commits November 9, 2017 20:49

Prototype implementation of parallel test execution

72bb365

Introduce TestTask parameter object

836bec8

Fix compiler warnings for AutoCloseable.close()

975e75b

Apply spotless

ff5027c

Make TreePrintingListener thread-safe

a6948be

Thread-safety is achieved by using a synchronized map. `TreePrinter` now resorts to `Color.NONE` if no color mapping is specified for the current identifier.

Merge branch 'master' into experiments/parallel-execution

aa38916

# Conflicts: # junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java

Prototype implementation of declarative read/write resource locks

eabacfb

Result of pair programming session with @leonard84. Issue: #60

Introduce value object instead of using annotation directly

bea2ee3

Issue: #60

Refactor CompositeLock into specialized locks, ...

897600e

Improve CompositeLock to keep track of acquired locks, release only those. Use lockInterruptably to support clean shutdown while waiting for locks. Add basic support for ExecutionControl/Mode

Implement Node.getExecutionMode() in Jupiter, fix build

f613910

WIP from session with @leonard84 on 2018-01-15

4599111

Introduce NodeWalker to compute execution plan

05637ec

Implement HierarchicalTestExecutorService based on work of NodeWalker

c289ce2

Make LogRecordListener thread-safe

02cd8a4

Make XmlReportAssertions thread-safe

7b88e58

Make tests for CloseableResource thread-safe

e4c19d7

A few minor refactorings

f86830d

Try to stabilize CI builds

9fa34dc

Make parallelism level configurable

e82d519

Increase parallelism by using ForkJoinTask API

5ca5bfb

Use ForkJoinPool constructor available in Java >= 9

4c96a22

- Limit max. number of threads - Set minimum runnable number of threads to parallelism

Optimize thread usage for nodes with only one child

d82a791

Co-authored-by: Leonard Brünings <[email protected]>

Use @testfactory's execution mode for dynamic tests/containers

1730fe7

Co-authored-by: Leonard Brünings <[email protected]>

Merge branch 'master' into experiments/parallel-execution

44a0764

# Conflicts: # junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java

Introduce StreamInterceptor

9a39995

Co-authored-by: Leonard Brünings <[email protected]>

Intercept and report stdout and stderr in DefaultLauncher

484b93a

Co-authored-by: Leonard Brünings <[email protected]>

Fix successfulParallelTest in ParallelExecutionTests on Windows

366f11b

Report stdout/stderr for containers

6b5ccae

marcphilipp self-assigned this Jun 8, 2018

ghost added the status: in progress label Jun 8, 2018

Merge remote-tracking branch 'origin/master' into experiments/paralle…

cdf9f93

…l-execution

sbrannen requested changes Jun 12, 2018

View reviewed changes

marcphilipp added 2 commits June 12, 2018 18:13

Rename launcher config properties

b7a6bec

Fix Javadoc and check additional preconditions

de69c49

marcphilipp added the status: team discussion label Jun 12, 2018

sbrannen approved these changes Jun 13, 2018

View reviewed changes

gaganis reviewed Jun 14, 2018

View reviewed changes

leonard84 requested changes Jun 14, 2018

View reviewed changes

marcphilipp added 5 commits June 21, 2018 20:59

Use ThreadLocal instead of CopyOnWriteArrayList

650fa52

Extract method for separate steps

4bf6044

Check that locks are acquired in order

ba7fb76

Check lock class in unit tests

4b40b64

Use enum singleton pattern to cache Schema

135a07d

Rename UseResource to ResourceLock

61441bb

marcphilipp closed this in f6ecfa4 Jun 22, 2018

ghost removed the status: in progress label Jun 22, 2018

marcphilipp deleted the experiments/parallel-execution branch June 22, 2018 17:58

sbrannen removed the status: team discussion label Jul 19, 2019

		@@ -94,4 +109,12 @@ public void reportingEntryPublished(TestIdentifier testIdentifier, ReportEntry e

		}

		interface EagerTestExecutionListener extends TestExecutionListener {

Add support for parallel test execution #1461

Add support for parallel test execution #1461

Conversation

marcphilipp commented Jun 8, 2018 • edited Loading

Definition of Done

codecov bot commented Jun 8, 2018 • edited Loading

Codecov Report

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 Jun 12, 2018 • 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

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

marcphilipp commented Jun 12, 2018

sbrannen commented Jun 13, 2018

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

jbduncan Jun 21, 2018 • 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

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 commented Jun 22, 2018

sormuras commented Jun 22, 2018

sbrannen commented Jun 22, 2018

marcphilipp commented Jun 8, 2018 •

edited

Loading

codecov bot commented Jun 8, 2018 •

edited

Loading

marcphilipp Jun 12, 2018 •

edited

Loading

jbduncan Jun 21, 2018 •

edited

Loading