Introduced @ResourceLock(target = SELF | CHILDREN)
#3220
Conversation
Defaults to SELF to preserve existing behavior. Using CHILDREN has the same effect as declaring @ResourceLock on every @test method of a test class. Issue: junit-team#3102
| import org.apiguardian.api.API; | ||
|
|
||
| /** | ||
| * Indicates the target of a {@link ResourceLock}. |
There was a problem hiding this comment.
| * Indicates the target of a {@link ResourceLock}. | |
| * Indicates the target of a {@link ResourceLock}. |
| @@ -40,6 +40,9 @@ | |||
| * <p>Since JUnit Jupiter 5.4, this annotation is {@linkplain Inherited inherited} | |||
| * within class hierarchies. | |||
| * | |||
| * <p>Since JUnit Jupiter 5.10, this annotation can be used to specify the target | |||
| * of the lock with {@link ResourceLockTarget}. | |||
| * | |||
There was a problem hiding this comment.
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.
| * @param lockScope the lock scope to use to synchronize access to the | ||
| * resource; never {@code null} | ||
| */ | ||
| public ExclusiveResource(String key, LockMode lockMode, LockScope lockScope) { |
There was a problem hiding this comment.
I don't think this should be a Platform concept. Instead, Jupiter should return ExclusiveResource instances from the children TestDescriptors if their parent declares a @ResourceLock with target of CHILDREN.
There was a problem hiding this comment.
sounds good, no need to bother Platform with those details :) I drafted how it may look in https://github.com/junit-team/junit5/pull/3220/files#diff-63a01db8dd78b02e05c3888eb786c656af0cd3dc8263c816530003f2d7069daeR188
| Set<ExclusiveResource> resources = mapResourceLocksForTarget(getResourceLocks(), ResourceLockTarget.SELF); | ||
|
|
||
| Optional<TestDescriptor> nextParent = getParent(); | ||
| while (nextParent.isPresent()) { |
There was a problem hiding this comment.
IIUC @ResourceLock(target = CHILDREN) should only cause them to be applied to direct children.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I think we also need to think about the case where @ResourceLock(target = CHILDREN) is applied to methods. I think for @Test this should cause a failure. For test templates such as @ParameterizedTest and @RepeatedTest the invocations should get the ExclusiveResource (might already work with the code in this PR but we're missing a test). For @TestFactory methods, the direct DynamicNodeTestDescriptor children should get the ExclusiveResource.
|
@anatolyD Do you have time to continue working on this PR or should someone from the JUnit team take over? |
Hey, yes I would love to continue, but if time is a pressuring matter you may want to take it over. I plan to get to it within a week or two |
|
I don't think it's urgent, so that's fine. 👍 |
|
@anatolyD Are you still interested in continuing on this PR? |
|
Hey, right, it should be closed. Will not progress on it |
|
No worries, thanks for letting us know! |
Defaults to SELF to preserve existing behavior.
Using CHILDREN has the same effect as declaring @ResourceLock on every @test method of a test class.
Issue: #3102
Overview
Introduced
@ResourceLock(target = SELF | CHILDREN)where the target defaults toSELFto preserve existing behavior. UsingCHILDRENhas the same effect as declaring@ResourceLockon every@Testmethod of a test class.I've added a couple of integration tests to verify it works, plus one for NodeWalker. Happy to hear the feedback. 😄
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations