-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce adding 'shared resources' programmatically #3889
base: main
Are you sure you want to change the base?
Introduce adding 'shared resources' programmatically #3889
Conversation
50377ba
to
194832d
Compare
c0b3d77
to
a90636d
Compare
Issue: junit-team#2677
3e1d2a6
to
7d894c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very promising! Sorry for the late request to change the API.
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
@Inherited | ||
public @interface ResourceLocksFrom { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion in the team, we'd prefer adding a Class<? extends ResourceLocksProvider>[] providers() default {}
attribute to the existing @ResourceLock
annotation (and making value()
optional by giving it an empty default value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
junit-jupiter-api/src/main/java/org/junit/jupiter/api/parallel/ResourceLocksProvider.java
Show resolved
Hide resolved
[source,java] | ||
.Declaring shared resources provider with `{ResourceLocksFrom}` annotation | ||
---- | ||
include::{testDir}/example/SharedResourcesWithResourceLocksFromAnnotationDemo.java[tags=user_guide] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also mention here (and in the annotations' Javadoc) that "static" and "dynamic" locks are combined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Mentioned in Javadoc for @ResourceLock
and ResourceLocksProvider
.
…Tests'." This reverts commit ff3c786. Issue: junit-team#2677
…nto #2677_add_exclusive_resource_programmatically
Hi @marcphilipp |
getExclusiveResourcesFromAnnotationValue(element), | ||
getExclusiveResourcesFromAnnotationProviders(element, providerToLocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid searching for the annotation twice here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ResourceLock}, they would be _flaky_. Sometimes they would pass, and at other times they | ||
would fail due to the inherent race condition of writing and then reading the same JVM | ||
System Property. | ||
Since Junit Jupiter 5.12 `{ResourceLock}` annotation has 'providers' attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the User Guide is versioned, we usually avoid mentioning specific versions when features were introduced. Thus, please remove that bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Done
This guarantee extends to lifecycle methods of a test class or method. | ||
For example, if a test method is annotated with a `{ResourceLock}` annotation, | ||
the "lock" will be acquired before any `@BeforeEach` methods are executed | ||
and released after all `@AfterEach` methods have been executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the changes made to the formatting of these two paragraphs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@Override | ||
public Set<Lock> provideForMethod(Class<?> testClass, Method testMethod) { | ||
ResourceAccessMode mode; | ||
if (testMethod.getName().equals("customPropertyIsNotSetByDefault")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make for a better example if we'd check for testMethod.getName().startsWith("canSet")
and invert the if
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Done
providerToLocks | ||
); | ||
Stream<ExclusiveResource> fromProvidersInClassLevelAnnotation = getExclusiveResourcesFromAnnotationProviders( | ||
getTestClass(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about @Nested
test classes? I think we'll have to collect exclusive resources from providers from the test class and, if it's a @Nested
one, also from its enclosing class, recursively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, there is no need to apply locks from the class level to methods as it is redundant. When executing the method, the class-level locks are already held.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
@Nested
test classes? I think we'll have to collect exclusive resources from providers from the test class and, if it's a@Nested
one, also from its enclosing class, recursively.
I suppose that the behaviour of ResourceLocksProvider
should be similar to the plain @ResourceLock("...")
.
Let's consider such an example:
@ResourceLock("A")
static class TestClass {
@Nested
@ResourceLock("B")
class NestedTestClass {
}
}
NestedTestClass
gets only B
but does not get A
.
That's why I'm not sure that @Nested
classes should invoke providers of enclosing classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please disregard my initial comment. We should neither check the test class nor any of its enclosing classes since that's already being done by the parent test descriptors. Thus, please remove the extra code that sets fromProvidersInClassLevelAnnotation
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this code then LocksProvider#provideForMethod()
will not be invoked for test()
:
@ResourceLock(providers = LocksProvider.class)
static class TestClass {
@Test
void test() {
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. I didn't think about that use case. In that case, we also need to ensure LocksProvider#provideForMethod()
gets called for nested classes, e.g.:
@ResourceLock(providers = LocksProvider.class)
static class TestClass { // A: `LocksProvider#provideForClass()` is called
@Nested
class Inner { // B: `LocksProvider#provideForNestedClass()` is called
@Test
void test() { // C: `LocksProvider#provideForMethod()` is called
}
}
}
If I'm reading the current code correctly, neither B nor C would be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.
Done
providerToLocks | ||
); | ||
Stream<ExclusiveResource> fromProvidersInClassLevelAnnotation = getExclusiveResourcesFromAnnotationProviders( | ||
getTestClass(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. I didn't think about that use case. In that case, we also need to ensure LocksProvider#provideForMethod()
gets called for nested classes, e.g.:
@ResourceLock(providers = LocksProvider.class)
static class TestClass { // A: `LocksProvider#provideForClass()` is called
@Nested
class Inner { // B: `LocksProvider#provideForNestedClass()` is called
@Test
void test() { // C: `LocksProvider#provideForMethod()` is called
}
}
}
If I'm reading the current code correctly, neither B nor C would be called.
assertThat(nestedClassResources).containsExactlyInAnyOrder( | ||
new ExclusiveResource("c1", LockMode.READ_WRITE), | ||
new ExclusiveResource("c2", LockMode.READ_WRITE) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provider declared on the outermost class is called as well, right? I think we should change the test to show that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right.
But such case is already covered in ResourceLocksProviderTests#ClassLevelProviderTestCase
.
Do you think we should cover it here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it would be good to have a test that checks that both providers are called and the provider on the nested class does not override the one on the outer class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. We didn't test such case.
Added
Solves: #2677
Overview
Introduce adding
shared resources
programmatically via@ResourceLock(providers = ...)
attributewhich accepts an array of one or more classes implementing
ResourceLocksProvider
interface.It allows to add resources in runtime (immediately before starting execution on the engine level).
Resources can be distributed based on any test class / nested test class / test method attributes (e.g. package name, class / method name etc.) or any other user logic.
This approach serves as a more flexible and less verbose alternative for cases in which:
@ResourceLock(value, mode)
annotations manually may be inconvenient;I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@API
annotations