From 60f31252ccd7d21d6654e00166d17943e0083553 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Sat, 15 Dec 2018 15:54:27 +0100 Subject: [PATCH] Fix using resource locks on test templates Prior to this commit, it was not possible to use `@ResourceLock` on `@TestTemplate` methods because `TestTemplateInvocationTestDescriptor` returned a non-empty set of `ExclusiveResources` which is disallowed for dynamically added `TestDescriptors`. Now, an empty set is returned instead which works because the parent `TestTemplateTestDescriptor` returns the resources from the `@ResourceLock` annotation. Fixes #1697. --- .../release-notes/release-notes-5.4.0-M1.adoc | 4 +- .../TestTemplateInvocationTestDescriptor.java | 9 ++++ ...TemplateInvocationTestDescriptorTests.java | 54 +++++++++++++++++++ .../hierarchical/NodeExecutionAdvisor.java | 16 ++++-- .../support/hierarchical/NodeTreeWalker.java | 3 +- .../ParallelExecutionIntegrationTests.java | 18 +++++++ 6 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptorTests.java diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.4.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.4.0-M1.adoc index 6c3034af24da..b02ebf6e7f82 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.4.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.4.0-M1.adoc @@ -69,7 +69,9 @@ repository on GitHub. ==== Bug Fixes -* ❓ +* `@ResourceLock` can now be declared on test template methods such as `@RepeatedTest` and + `@ParameterizedTest` methods. If `@ResourceLock` is used, the invocations will run in + the same thread, even if parallel execution is enabled. ==== Deprecations and Breaking Changes diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java index c2ddebea0df5..6b7c43d5f31b 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptor.java @@ -10,9 +10,11 @@ package org.junit.jupiter.engine.descriptor; +import static java.util.Collections.emptySet; import static org.apiguardian.api.API.Status.INTERNAL; import java.lang.reflect.Method; +import java.util.Set; import org.apiguardian.api.API; import org.junit.jupiter.api.extension.TestTemplateInvocationContext; @@ -21,6 +23,7 @@ import org.junit.jupiter.engine.extension.ExtensionRegistry; import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.UniqueId; +import org.junit.platform.engine.support.hierarchical.ExclusiveResource; /** * {@link TestDescriptor} for a {@link org.junit.jupiter.api.TestTemplate @TestTemplate} @@ -43,6 +46,12 @@ public class TestTemplateInvocationTestDescriptor extends TestMethodTestDescript this.index = index; } + @Override + public Set getExclusiveResources() { + // @ResourceLock annotations are already collected and returned by the enclosing container + return emptySet(); + } + @Override public String getLegacyReportingName() { return super.getLegacyReportingName() + "[" + index + "]"; diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptorTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptorTests.java new file mode 100644 index 000000000000..da737a14ee0b --- /dev/null +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/TestTemplateInvocationTestDescriptorTests.java @@ -0,0 +1,54 @@ +/* + * Copyright 2015-2018 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * http://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.jupiter.engine.descriptor; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Method; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestTemplate; +import org.junit.jupiter.api.extension.TestTemplateInvocationContext; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.engine.config.JupiterConfiguration; +import org.junit.platform.engine.UniqueId; + +class TestTemplateInvocationTestDescriptorTests { + + @Test + void invocationsDoNotDeclareExclusiveResources() throws Exception { + Class testClass = MyTestCase.class; + Method testTemplateMethod = testClass.getDeclaredMethod("testTemplate"); + JupiterConfiguration configuration = mock(JupiterConfiguration.class); + TestTemplateTestDescriptor parent = new TestTemplateTestDescriptor(UniqueId.root("segment", "template"), + testClass, testTemplateMethod, configuration); + TestTemplateInvocationContext invocationContext = mock(TestTemplateInvocationContext.class); + when(invocationContext.getDisplayName(anyInt())).thenReturn("invocation"); + + TestTemplateInvocationTestDescriptor testDescriptor = new TestTemplateInvocationTestDescriptor( + parent.getUniqueId().append(TestTemplateInvocationTestDescriptor.SEGMENT_TYPE, "1"), testClass, + testTemplateMethod, invocationContext, 1, configuration); + + assertThat(parent.getExclusiveResources()).hasSize(1); + assertThat(testDescriptor.getExclusiveResources()).isEmpty(); + } + + static class MyTestCase { + @TestTemplate + @ResourceLock("a") + void testTemplate() { + } + } + +} diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java index 6919a2b348bc..b850a294d1e0 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java @@ -22,11 +22,11 @@ */ class NodeExecutionAdvisor { - private final Map forcedExecutionModeByTestDescriptor = new HashMap<>(); + private final Map forcedDescendantExecutionModeByTestDescriptor = new HashMap<>(); private final Map resourceLocksByTestDescriptor = new HashMap<>(); - void forceExecutionMode(TestDescriptor testDescriptor, ExecutionMode executionMode) { - forcedExecutionModeByTestDescriptor.put(testDescriptor, executionMode); + void forceDescendantExecutionMode(TestDescriptor testDescriptor, ExecutionMode executionMode) { + forcedDescendantExecutionModeByTestDescriptor.put(testDescriptor, executionMode); } void useResourceLock(TestDescriptor testDescriptor, ResourceLock resourceLock) { @@ -34,7 +34,15 @@ void useResourceLock(TestDescriptor testDescriptor, ResourceLock resourceLock) { } Optional getForcedExecutionMode(TestDescriptor testDescriptor) { - return Optional.ofNullable(forcedExecutionModeByTestDescriptor.get(testDescriptor)); + return testDescriptor.getParent().flatMap(this::lookupExecutionModeForcedByAncestor); + } + + private Optional lookupExecutionModeForcedByAncestor(TestDescriptor testDescriptor) { + ExecutionMode value = forcedDescendantExecutionModeByTestDescriptor.get(testDescriptor); + if (value != null) { + return Optional.of(value); + } + return testDescriptor.getParent().flatMap(this::lookupExecutionModeForcedByAncestor); } ResourceLock getResourceLock(TestDescriptor testDescriptor) { diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java index 628dbb52d798..69e98a4036fd 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java @@ -38,9 +38,10 @@ private void walk(TestDescriptor testDescriptor, NodeExecutionAdvisor advisor) { } else { Set allResources = new HashSet<>(exclusiveResources); + advisor.forceDescendantExecutionMode(testDescriptor, SAME_THREAD); doForChildrenRecursively(testDescriptor, child -> { allResources.addAll(getExclusiveResources(child)); - advisor.forceExecutionMode(child, SAME_THREAD); + advisor.forceDescendantExecutionMode(child, SAME_THREAD); }); advisor.useResourceLock(testDescriptor, lockManager.getLockForResources(allResources)); } diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java index f7e6559de881..e536202bf76d 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java @@ -171,6 +171,14 @@ void threadInterruptedByUserCode() { assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).size().isEqualTo(4); } + @Test + void executesTestTemplatesWithResourceLocksInSameThread() { + List events = execute(2, ConcurrentTemplateTestCase.class); + + assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).size().isEqualTo(10); + assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); + } + private List getTimestampsFor(List events, Condition condition) { // @formatter:off return events.stream() @@ -496,6 +504,16 @@ void test4() throws InterruptedException { } + @Execution(CONCURRENT) + @ExtendWith(ThreadReporter.class) + static class ConcurrentTemplateTestCase { + @RepeatedTest(10) + @ResourceLock("a") + void repeatedTest() throws Exception { + Thread.sleep(100); + } + } + private static void incrementBlockAndCheck(AtomicInteger sharedResource, CountDownLatch countDownLatch) throws InterruptedException { int value = incrementAndBlock(sharedResource, countDownLatch);