Allow specifying which Nodes require the global read lock (#5099)

Implementations of `HierarchicalTestEngine` can now specify which nodes
require the global read lock by overriding the new
`Node.isGlobalReadLockRequired()` method to return `false`.

This allows for more fine-grained locks when intermediate container do
not have any behavior of their own and thus do not require even the
global read lock.

Resolves #2982.

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-6.1.0-M1.adoc

@@ -31,6 +31,9 @@ repository on GitHub.
 * `OpenTestReportGeneratingListener` now supports redirecting XML events to a socket via
   the new `junit.platform.reporting.open.xml.socket` configuration parameter. When set to a
   port number, events are sent to `127.0.0.1:<port>` instead of being written to a file.
+* Allow implementations of `HierarchicalTestEngine` to specify which nodes require the
+  global read lock by overriding the `Node.isGlobalReadLockRequired()` method to return
+  `false`.
 
 
 [[release-notes-6.1.0-M1-junit-jupiter]]

junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/Node.java

@@ -11,6 +11,7 @@
 package org.junit.platform.engine.support.hierarchical;
 
 import static java.util.Collections.emptySet;
+import static org.apiguardian.api.API.Status.EXPERIMENTAL;
 import static org.apiguardian.api.API.Status.MAINTAINED;
 import static org.apiguardian.api.API.Status.STABLE;
 
@@ -182,6 +183,23 @@ default Set<ExclusiveResource> getExclusiveResources() {
 		return emptySet();
 	}
 
+	/**
+	 * {@return whether this node requires the global read-write lock}
+	 *
+	 * <p>Engines should return {@code true} for all nodes that have behavior,
+	 * including tests but also containers with before/after lifecycle hooks.
+	 *
+	 * <p>The default implementation returns {@code true}. The value returned by
+	 * engine-level nodes is ignored. Otherwise, if a container node returns
+	 * {@code true}, the value returned by its descendants is ignored.
+	 *
+	 * @since 6.1
+	 */
+	@API(status = EXPERIMENTAL, since = "6.1")
+	default boolean isGlobalReadLockRequired() {
+		return true;
+	}
+
 	/**
 	 * Get the preferred of {@linkplain ExecutionMode execution mode} for
 	 * parallel execution of this node.

junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java

@@ -13,11 +13,13 @@
 import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ;
 import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ_WRITE;
 import static org.junit.platform.engine.support.hierarchical.Node.ExecutionMode.SAME_THREAD;
+import static org.junit.platform.engine.support.hierarchical.NodeUtils.asNode;
 
 import java.util.HashSet;
 import java.util.Set;
 import java.util.function.Consumer;
 
+import org.jspecify.annotations.Nullable;
 import org.junit.platform.commons.util.Preconditions;
 import org.junit.platform.engine.TestDescriptor;
 
@@ -44,26 +46,34 @@ NodeExecutionAdvisor walk(TestDescriptor rootDescriptor) {
 		Preconditions.condition(getExclusiveResources(rootDescriptor).isEmpty(),
 			"Engine descriptor must not declare exclusive resources");
 		NodeExecutionAdvisor advisor = new NodeExecutionAdvisor();
-		rootDescriptor.getChildren().forEach(child -> walk(child, child, advisor));
+		rootDescriptor.getChildren().forEach(child -> walk(nullUnlessRequiresGlobalReadLock(child), child, advisor));
 		return advisor;
 	}
 
-	private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescriptor,
+	private void walk(@Nullable TestDescriptor globalLockDescriptor, TestDescriptor testDescriptor,
 			NodeExecutionAdvisor advisor) {
 
-		if (advisor.getResourceLock(globalLockDescriptor) == globalReadWriteLock) {
+		if (globalLockDescriptor != null && advisor.getResourceLock(globalLockDescriptor) == globalReadWriteLock) {
 			// Global read-write lock is already being enforced, so no additional locks are needed
 			return;
 		}
 
 		Set<ExclusiveResource> exclusiveResources = getExclusiveResources(testDescriptor);
 		if (exclusiveResources.isEmpty()) {
-			if (globalLockDescriptor.equals(testDescriptor)) {
+			if (globalLockDescriptor != null && globalLockDescriptor.equals(testDescriptor)) {
 				advisor.useResourceLock(globalLockDescriptor, globalReadLock);
 			}
-			testDescriptor.getChildren().forEach(child -> walk(globalLockDescriptor, child, advisor));
+			testDescriptor.getChildren().forEach(child -> {
+				var newGlobalLockDescriptor = globalLockDescriptor == null //
+						? nullUnlessRequiresGlobalReadLock(child) //
+						: globalLockDescriptor;
+				walk(newGlobalLockDescriptor, child, advisor);
+			});
 		}
 		else {
+			Preconditions.notNull(globalLockDescriptor,
+				() -> "Node requiring exclusive resources must also require global read lock: " + testDescriptor);
+
 			Set<ExclusiveResource> allResources = new HashSet<>(exclusiveResources);
 			if (isReadOnly(allResources)) {
 				doForChildrenRecursively(testDescriptor, child -> allResources.addAll(getExclusiveResources(child)));
@@ -109,7 +119,11 @@ private boolean isReadOnly(Set<ExclusiveResource> exclusiveResources) {
 	}
 
 	private Set<ExclusiveResource> getExclusiveResources(TestDescriptor testDescriptor) {
-		return NodeUtils.asNode(testDescriptor).getExclusiveResources();
+		return asNode(testDescriptor).getExclusiveResources();
+	}
+
+	private static @Nullable TestDescriptor nullUnlessRequiresGlobalReadLock(TestDescriptor testDescriptor) {
+		return asNode(testDescriptor).isGlobalReadLockRequired() ? testDescriptor : null;
 	}
 
 	private void doForChildrenRecursively(TestDescriptor parent, Consumer<TestDescriptor> consumer) {

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

@@ -11,6 +11,8 @@
 package org.junit.platform.engine.support.hierarchical;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.InstanceOfAssertFactories.LIST;
+import static org.junit.platform.commons.test.PreconditionAssertions.assertPreconditionViolationFor;
 import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement;
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
 import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ;
@@ -20,10 +22,13 @@
 import static org.junit.platform.engine.support.hierarchical.Node.ExecutionMode.SAME_THREAD;
 import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;
 
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.locks.Lock;
 import java.util.function.Function;
 
+import org.jspecify.annotations.NullMarked;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.ResourceAccessMode;
@@ -32,6 +37,8 @@
 import org.junit.jupiter.engine.JupiterTestEngine;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.UniqueId;
+import org.junit.platform.engine.support.descriptor.AbstractTestDescriptor;
+import org.junit.platform.engine.support.descriptor.EngineDescriptor;
 
 /**
  * @since 1.3
@@ -171,6 +178,55 @@ void coarsensGlobalLockToEngineDescriptorChild() {
 		assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).contains(SAME_THREAD);
 	}
 
+	@Test
+	void putsGlobalReadLockOnFirstNodeThatRequiresIt() {
+		var engineDescriptor = new EngineDescriptor(UniqueId.forEngine("dummy"), "Dummy");
+
+		var containerWithoutBehavior = new NodeStub(engineDescriptor.getUniqueId().append("container", "1"),
+			"Container 1") //
+					.withGlobalReadLockRequired(false);
+		var test1 = new NodeStub(containerWithoutBehavior.getUniqueId().append("test", "1"), "Test 1") //
+				.withExclusiveResource(new ExclusiveResource("key1", READ_WRITE));
+		containerWithoutBehavior.addChild(test1);
+
+		var containerWithBehavior = new NodeStub(engineDescriptor.getUniqueId().append("container", "2"), "Container 2") //
+				.withGlobalReadLockRequired(true);
+		var test2 = new NodeStub(containerWithBehavior.getUniqueId().append("test", "2"), "Test 2") //
+				.withExclusiveResource(new ExclusiveResource("key2", READ_WRITE));
+		containerWithBehavior.addChild(test2);
+
+		engineDescriptor.addChild(containerWithoutBehavior);
+		engineDescriptor.addChild(containerWithBehavior);
+
+		var advisor = nodeTreeWalker.walk(engineDescriptor);
+
+		assertThat(advisor.getResourceLock(containerWithoutBehavior)) //
+				.extracting(allLocks(), LIST) //
+				.isEmpty();
+		assertThat(advisor.getResourceLock(test1)) //
+				.extracting(allLocks(), LIST) //
+				.containsExactly(getLock(GLOBAL_READ), getReadWriteLock("key1"));
+
+		assertThat(advisor.getResourceLock(containerWithBehavior)) //
+				.extracting(allLocks(), LIST) //
+				.containsExactly(getLock(GLOBAL_READ));
+		assertThat(advisor.getResourceLock(test2)) //
+				.extracting(allLocks(), LIST) //
+				.containsExactly(getReadWriteLock("key2"));
+	}
+
+	@Test
+	void doesNotAllowExclusiveResourcesWithoutRequiringGlobalReadLock() {
+		var engineDescriptor = new EngineDescriptor(UniqueId.forEngine("dummy"), "Dummy");
+		var invalidNode = new NodeStub(engineDescriptor.getUniqueId().append("container", "1"), "Container") //
+				.withGlobalReadLockRequired(false) //
+				.withExclusiveResource(new ExclusiveResource("key", READ_WRITE));
+		engineDescriptor.addChild(invalidNode);
+
+		assertPreconditionViolationFor(() -> nodeTreeWalker.walk(engineDescriptor)) //
+				.withMessage("Node requiring exclusive resources must also require global read lock: " + invalidNode);
+	}
+
 	private static Function<org.junit.platform.engine.support.hierarchical.ResourceLock, List<Lock>> allLocks() {
 		return ResourceLockSupport::getLocks;
 	}
@@ -262,4 +318,41 @@ static class TestCaseWithResourceReadLockOnClassAndReadClockOnTestCase {
 		void test() {
 		}
 	}
+
+	@NullMarked
+	static class NodeStub extends AbstractTestDescriptor implements Node<EngineExecutionContext> {
+
+		private final Set<ExclusiveResource> exclusiveResources = new LinkedHashSet<>();
+		private boolean globalReadLockRequired = true;
+
+		NodeStub(UniqueId uniqueId, String displayName) {
+			super(uniqueId, displayName);
+		}
+
+		NodeStub withExclusiveResource(ExclusiveResource exclusiveResource) {
+			exclusiveResources.add(exclusiveResource);
+			return this;
+		}
+
+		NodeStub withGlobalReadLockRequired(boolean globalReadLockRequired) {
+			this.globalReadLockRequired = globalReadLockRequired;
+			return this;
+		}
+
+		@Override
+		public boolean isGlobalReadLockRequired() {
+			return globalReadLockRequired;
+		}
+
+		@Override
+		public Type getType() {
+			throw new UnsupportedOperationException("should not be called");
+		}
+
+		@Override
+		public Set<ExclusiveResource> getExclusiveResources() {
+			return exclusiveResources;
+		}
+	}
+
 }