Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduced @ResourceLock(target = SELF | CHILDREN) #3220

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -41,7 +41,10 @@
* within class hierarchies.
*
* <p>Since JUnit Jupiter 5.10, this annotation can be used to specify the target
* of the lock with {@link ResourceLockTarget}.
* of the lock with {@link ResourceLockTarget}. In case the same resource is announced
* for the parent with a target {@link ResourceLockTarget#CHILDREN} and for the child
* with {@link ResourceLockTarget#SELF} they will be merged as if they were declared
* on the child altogether with {@link ResourceLockTarget#SELF} target.
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a class has a @ResourceLock annotation with target CHILDREN and a method has one for the same resource? Does the one from the method override the one from the class? This should be documented here.

* @see Isolated
* @see Resources
Expand Down
Expand Up @@ -15,7 +15,7 @@
import org.apiguardian.api.API;

/**
* Indicates the target of a {@link ResourceLock}.
* Indicates the target of a {@link ResourceLock}.
*
* @since 5.10
* @see ResourceLock
Expand Down
Expand Up @@ -22,6 +22,7 @@
import static org.junit.jupiter.engine.descriptor.LifecycleMethodUtils.findBeforeEachMethods;
import static org.junit.jupiter.engine.descriptor.TestInstanceLifecycleUtils.getTestInstanceLifecycle;
import static org.junit.jupiter.engine.support.JupiterThrowableCollectorFactory.createThrowableCollector;
import static org.junit.platform.commons.util.AnnotationUtils.findRepeatableAnnotations;
import static org.junit.platform.commons.util.CollectionUtils.forEachInReverseOrder;

import java.lang.reflect.Constructor;
Expand Down Expand Up @@ -52,6 +53,7 @@
import org.junit.jupiter.api.extension.TestInstances;
import org.junit.jupiter.api.extension.TestInstantiationException;
import org.junit.jupiter.api.function.Executable;
import org.junit.jupiter.api.parallel.ResourceLock;
import org.junit.jupiter.engine.config.JupiterConfiguration;
import org.junit.jupiter.engine.execution.AfterEachMethodAdapter;
import org.junit.jupiter.engine.execution.BeforeEachMethodAdapter;
Expand Down Expand Up @@ -142,7 +144,12 @@ public void setDefaultChildExecutionMode(ExecutionMode defaultChildExecutionMode

@Override
public Set<ExclusiveResource> getExclusiveResources() {
return getExclusiveResourcesFromAnnotation(getTestClass());
return collectExclusiveResourcesFromHierarchy();
}

@Override
protected List<ResourceLock> getResourceLocks() {
return findRepeatableAnnotations(getTestClass(), ResourceLock.class);
}

@Override
Expand Down
Expand Up @@ -12,7 +12,6 @@

import static java.util.stream.Collectors.collectingAndThen;
import static java.util.stream.Collectors.toCollection;
import static java.util.stream.Collectors.toSet;
import static org.apiguardian.api.API.Status.INTERNAL;
import static org.junit.jupiter.engine.descriptor.DisplayNameUtils.determineDisplayName;
import static org.junit.platform.commons.util.AnnotationUtils.findAnnotation;
Expand All @@ -25,6 +24,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import org.apiguardian.api.API;
import org.junit.jupiter.api.Tag;
Expand All @@ -50,7 +50,6 @@
import org.junit.platform.engine.support.descriptor.AbstractTestDescriptor;
import org.junit.platform.engine.support.hierarchical.ExclusiveResource;
import org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockMode;
import org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockScope;
import org.junit.platform.engine.support.hierarchical.Node;

/**
Expand Down Expand Up @@ -182,15 +181,32 @@ public static ExecutionMode toExecutionMode(org.junit.jupiter.api.parallel.Execu
throw new JUnitException("Unknown ExecutionMode: " + mode);
}

Set<ExclusiveResource> getExclusiveResourcesFromAnnotation(AnnotatedElement element) {
protected List<ResourceLock> getResourceLocks() {
return Collections.emptyList();
}

protected Set<ExclusiveResource> collectExclusiveResourcesFromHierarchy() {
Set<ExclusiveResource> resources = mapResourceLocksForTarget(getResourceLocks(), ResourceLockTarget.SELF);

Optional<TestDescriptor> nextParent = getParent();
while (nextParent.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC @ResourceLock(target = CHILDREN) should only cause them to be applied to direct children.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC @ResourceLock(target = CHILDREN) should only cause them to be applied to direct children.

Is there any reason for this? Is it harmful in someway for it to not be just direct children? e.g. in our project our tests inherit from base classes scattered throughout our packages and those base classes tend to inherit from a single source parent. If I have a resource that I want read locked for everything (main server component for example) it makes more sense to me to have the lock specified in one place instead of having to remember to add it to all the base classes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Squibbles This is unrelated to inheritance in class hierarchies. The @ResourceLock annotation is inherited to subclasses already. However, the way resource locks are applied is that if a node acquires a lock, its children already hold it while being executed. Therefore it doesn't make sense to recursively declare it for all descendants.

TestDescriptor testDescriptor = nextParent.get();
if (testDescriptor instanceof JupiterTestDescriptor) {
List<ResourceLock> parentLocks = ((JupiterTestDescriptor) testDescriptor).getResourceLocks();
resources.addAll(mapResourceLocksForTarget(parentLocks, ResourceLockTarget.CHILDREN));
}
nextParent = testDescriptor.getParent();
}
return resources;
}

private Set<ExclusiveResource> mapResourceLocksForTarget(List<ResourceLock> ResourceLocks,
ResourceLockTarget target) {
// @formatter:off
return findRepeatableAnnotations(element, ResourceLock.class).stream()
.map(resource -> new ExclusiveResource(
resource.value(),
toLockMode(resource.mode()),
toLockScope(resource.target()))
)
.collect(toSet());
return ResourceLocks.stream()
.filter(lock -> lock.target() == target)
.map(resource -> new ExclusiveResource(resource.value(), toLockMode(resource.mode())))
.collect(Collectors.toSet());
// @formatter:on
}

Expand All @@ -204,16 +220,6 @@ private static LockMode toLockMode(ResourceAccessMode mode) {
throw new JUnitException("Unknown ResourceAccessMode: " + mode);
}

private static LockScope toLockScope(ResourceLockTarget lockTarget) {
switch (lockTarget) {
case SELF:
return LockScope.SELF;
case CHILDREN:
return LockScope.CHILDREN;
}
throw new JUnitException("Unknown ResourceLockTarget: " + lockTarget);
}

@Override
public SkipResult shouldBeSkipped(JupiterEngineExecutionContext context) throws Exception {
context.getThrowableCollector().assertEmpty();
Expand Down
Expand Up @@ -12,6 +12,7 @@

import static org.apiguardian.api.API.Status.INTERNAL;
import static org.junit.jupiter.engine.descriptor.DisplayNameUtils.determineDisplayNameForMethod;
import static org.junit.platform.commons.util.AnnotationUtils.findRepeatableAnnotations;
import static org.junit.platform.commons.util.CollectionUtils.forEachInReverseOrder;

import java.lang.reflect.Method;
Expand All @@ -20,10 +21,12 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Predicate;

import org.apiguardian.api.API;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.TestWatcher;
import org.junit.jupiter.api.parallel.ResourceLock;
import org.junit.jupiter.engine.config.JupiterConfiguration;
import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
import org.junit.platform.commons.logging.Logger;
Expand All @@ -47,6 +50,7 @@
public abstract class MethodBasedTestDescriptor extends JupiterTestDescriptor {

private static final Logger logger = LoggerFactory.getLogger(MethodBasedTestDescriptor.class);
public static final Predicate<ResourceLock> ACCEPT_ALL = lock -> true;

private final Class<?> testClass;
private final Method testMethod;
Expand Down Expand Up @@ -81,7 +85,12 @@ public final Set<TestTag> getTags() {

@Override
public Set<ExclusiveResource> getExclusiveResources() {
return getExclusiveResourcesFromAnnotation(getTestMethod());
return collectExclusiveResourcesFromHierarchy();
}

@Override
protected List<ResourceLock> getResourceLocks() {
return findRepeatableAnnotations(getTestMethod(), ResourceLock.class);
}

@Override
Expand Down
Expand Up @@ -52,34 +52,18 @@ public class ExclusiveResource {

private final String key;
private final LockMode lockMode;
private final LockScope lockScope;
private int hash;

/**
* Create a new {@code ExclusiveResource} with a default lock scope {@link LockScope#SELF}
*
* @param key the identifier of the resource; never {@code null} or blank
* @param lockMode the lock mode to use to synchronize access to the
* resource; never {@code null}
*
*/
public ExclusiveResource(String key, LockMode lockMode) {
this(key, lockMode, LockScope.SELF);
}

/**
* Create a new {@code ExclusiveResource}.
*
* @param key the identifier of the resource; never {@code null} or blank
* @param lockMode the lock mode to use to synchronize access to the
* resource; never {@code null}
* @param lockScope the lock scope to use to synchronize access to the
* resource; never {@code null}
*/
public ExclusiveResource(String key, LockMode lockMode, LockScope lockScope) {
public ExclusiveResource(String key, LockMode lockMode) {
this.key = Preconditions.notBlank(key, "key must not be blank");
this.lockMode = Preconditions.notNull(lockMode, "lockMode must not be null");
this.lockScope = Preconditions.notNull(lockScope, "lockScope must not be null");
}

/**
Expand All @@ -96,13 +80,6 @@ public LockMode getLockMode() {
return lockMode;
}

/**
* Get the lock scope of this resource.
*/
public LockScope getLockScope() {
return lockScope;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -112,26 +89,21 @@ public boolean equals(Object o) {
return false;
}
ExclusiveResource that = (ExclusiveResource) o;
return Objects.equals(key, that.key) && lockMode == that.lockMode && lockScope == that.lockScope;
return Objects.equals(key, that.key) && lockMode == that.lockMode;
}

@Override
public int hashCode() {
int h = hash;
if (h == 0) {
h = hash = Objects.hash(key, lockMode, lockScope);
h = hash = Objects.hash(key, lockMode);
}
return h;
}

@Override
public String toString() {
return new ToStringBuilder(this).append("key", key).append("lockMode", lockMode).append("lockScope",
lockScope).toString();
}

public ExclusiveResource convertToSelfTarget() {
return new ExclusiveResource(key, lockMode, LockScope.SELF);
return new ToStringBuilder(this).append("key", key).append("lockMode", lockMode).toString();
}

/**
Expand Down Expand Up @@ -159,21 +131,4 @@ public enum LockMode {

}

/**
* {@code LockTarget} defines the scope of the lock.
*/
public enum LockScope {

/**
* Lock the resource for the node itself.
*/
SELF,

/**
* Lock the resource for all children of the node. Bypass the node itself.
*/
CHILDREN

}

}