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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ExclusiveResource programmatically #2677

Closed
U1F984 opened this issue Jul 29, 2021 · 43 comments · Fixed by #3889 or #4053
Closed

Add ExclusiveResource programmatically #2677

U1F984 opened this issue Jul 29, 2021 · 43 comments · Fixed by #3889 or #4053

Comments

@U1F984
Copy link

U1F984 commented Jul 29, 2021

I have several test classes, each with multiple test methods inside. Some of those classes are injected with a parameter, and when running classes in parallel those classes should not run parallel. If I understand correctly, the @ResourceLock annotation can help here, but it would be very inconvenient to manually add that annotation to hundereds of tests. Ideally a extension could take a look at the class, see that it has this parameter and somehow add the ExclusiveResource to it.

My JUnit execution parameters:

-Djunit.jupiter.execution.parallel.enabled=true
-Djunit.jupiter.execution.parallel.mode.default=same_thread
-Djunit.jupiter.execution.parallel.mode.classes.default=concurrent
-Djunit.jupiter.execution.parallel.config.strategy=fixed
-Djunit.jupiter.execution.parallel.config.fixed.parallelism=8
@marcphilipp
Copy link
Member

marcphilipp commented Oct 31, 2021

We could add a new extension point for this:

interface ExclusiveResourceProvider extends Extension {
	default Set<ExclusiveResource> provideExclusiveResourcesForClass(Class<?> testClass, Set<ExclusiveResource> declaredResources) {
		return declaredResources;
	};
	default Set<ExclusiveResource> provideExclusiveResourcesForNestedClass(Class<?> nestedClass, Set<ExclusiveResource> declaredResources) {
		return declaredResources;
	};
	default Set<ExclusiveResource> provideExclusiveResourcesForMethod(Class<?> testClass, Method testMethod, Set<ExclusiveResource> declaredResources) {
		return declaredResources;
	};
}
interface ExclusiveResource {
	String getKey();
	ResourceAccessMode getAccessMode();
	static ExclusiveResource of(String key);
	static ExclusiveResource of(String key, ResourceAccessMode accessMode);
}

@jbduncan
Copy link
Contributor

jbduncan commented Nov 4, 2021

This feature might be quite useful for my PR on junit-pioneer to allow "resources" - arbitrary objects - to be instantiated and shared amongst tests declaratively.

Specifically, if each "resource" were to have its own ResourceLock that could be created programmatically, then it would simplify making my extension thread-safe.

The proposed API leaves me with one question, though: if my extension were to implement ResourceLockProvider, when would the provideX methods be called?

@marcphilipp
Copy link
Member

It would have to be called immediately before starting execution on the engine level.

@jbduncan
Copy link
Contributor

jbduncan commented Nov 5, 2021

Okay, thank you!

I wasn't sure at first if this would work for me, since my extension's resources are created at test runtime. But I think it could work now, as all shared resources in my extension are declared with @Shared annotations, which I imagine my potential ResourceLockProvider could search for reflectively.

So I'm happy with the proposed API. 👍

@marcphilipp
Copy link
Member

Team decision: Let's go with the above proposal.

@U1F984 @jbduncan Is one of you interested in working on this?

@jbduncan
Copy link
Contributor

jbduncan commented Nov 19, 2021

@marcphilipp I'd love to! However I still have junit-pioneer/junit-pioneer#348 to finish, so I'm happy for anyone else to pick this up before me.

@jbduncan
Copy link
Contributor

For clarity, my JUnit Pioneer work is not actually blocked on this feature, as I've found an alternative solution to programmatic ResourceLocks for my needs.

@jbduncan
Copy link
Contributor

@marcphilipp A follow-up query from me: do you know if JUnit Jupiter already ensures that ResourceLocks are obtained in such an order as to prevent the dining philosophers problem, by any chance?

If not, then I, or whoever picks this up, will need to ensure the locks are sorted when being moved from ResourceLockProviders to the rest of the test framework, by some definition of "sorted".

@marcphilipp
Copy link
Member

@jbduncan
Copy link
Contributor

@marcphilipp Great, thank you for answering so quickly. 👍

@jbduncan
Copy link
Contributor

@marcphilipp I'd love it if you assigned this to me, as I've managed to park my junit-pioneer work and find time to start looking at this.

@marcphilipp
Copy link
Member

@jbduncan Sorry it took 6 days, but you're now assigned.

@jbduncan
Copy link
Contributor

jbduncan commented Nov 14, 2022

Sorry for the silence on this! I was a bit lost looking through the JUnit 5 codebase when I looked at this issue last year. But now that my JUnit Pioneer PR that would have used this feature is finally done, I'll see if I can find the time to revisit this issue later this week, and write down anything I need help with. :)

@jbduncan
Copy link
Contributor

@marcphilipp I have a few follow-up questions which I was hoping you could help me with.

For the proposed ExclusiveResourceProvider,

  1. Does a provide* method's declaredResources contain just the resources found so far for the given class/method/nested class, or does it contain all resources found so far by JUnit?

  2. Do the declaredResources contain resources found through @ResourceLocks?

    @Test
    @ResourceLock(value = "a")
    @ExtendWith(MyExclusiveResourceProvider.class)
    void test() {
    }
  3. If so, does the ordering matter? As in, does MyExclusiveResourceProvider's declared resources contain resource "a" when running with test2, but not with test1?

    @Test
    @ExtendWith(MyExclusiveResourceProvider.class)
    @ResourceLock(value = "a")
    void test1() {
    }
    
    @Test
    @ResourceLock(value = "a")
    @ExtendWith(MyExclusiveResourceProvider.class)
    void test2() {
    }
    
    class MyExclusiveResourceProvider implements ExclusiveResourceProvider {
        @Override public Set<ExclusiveResource> provideExclusiveResourcesForMethod(Class<?> testClass, Method testMethod, Set<ExclusiveResource> declaredResources) {
            return declaredResources;
        }
    }

@jbduncan
Copy link
Contributor

I've pushed my ongoing spike to #3096 for the sake of transparency. :)

@jbduncan
Copy link
Contributor

@marcphilipp I'm feeling a bit stuck now, and I was wondering if you could help me.

It looks like NodeTreeWalker is responsible for discovering tests and, most importantly, their @ResourceLocks. However, to get resource locks from ExclusiveResourceProviders, JUnit would need to discover them as extensions, which are currently discovered by NodeTestTasks, which in turn depend on the NodeTreeWalker! So there's a cycle of dependencies (maybe even a web), and I can't see how to break it, so I was was wondering if you had any thoughts?

Alternatively, would you be up to discussing this remotely some time? If so, feel free to contact me on my GitHub email address.

@jbduncan
Copy link
Contributor

@marcphilipp I don't suppose you've had the time to look at my last comment recently, have you? 🙂

@marcphilipp
Copy link
Member

@jbduncan Sorry, I had fallen quite behind in reading my GitHub notifications. 😬

It looks like NodeTreeWalker is responsible for discovering tests and, most importantly, their @ResourceLocks. However, to get resource locks from ExclusiveResourceProviders, JUnit would need to discover them as extensions, which are currently discovered by NodeTestTasks, which in turn depend on the NodeTreeWalker!

I'm not sure I fully understand the question. ExclusiveResourceProvider can't be a regular extension since it needs to be applied at discovery time, i.e. it should not extend Extension. We already have a few of this kind of extension, e.g. DisplayNameGenerator and MethodOrderer. So I think we'll need a new annotation that goes along with ExclusiveResourceProvider, maybe @ExclusiveResources(MyExclusiveResourceProvider.class)?

@VladimirDmitrienko
Copy link
Contributor

VladimirDmitrienko commented Jul 24, 2024

Fair enough.

There are two options coming in my mind.

  1. What if we reuse existing @ResourceLocks annotation:
public @interface ResourceLocks {

	Class<? extends ResourceLocksProvider> provider() default EmptyResourceLocksProvider.class;
        // or
	Class<? extends ResourceLocksProvider>[] providers() default {};
}
  1. @ResourceLocksFrom(MyResourceLocksProvider.class)

@marcphilipp
Copy link
Member

What if we reuse existing @ResourceLocks annotation

That would break backward compatibility.

@ResourceLocksFrom(MyResourceLocksProvider.class)

Let's go with that. Renaming it as part of the PR should be easy enough if we come up with a better name.

@VladimirDmitrienko
Copy link
Contributor

VladimirDmitrienko commented Jul 25, 2024

Agreed 👍
As always, naming is hard 😅

Just curious, what kind of compatibility will it break?
I supposed adding a new method will be both a source and binary compatible change.

image

link

@marcphilipp
Copy link
Member

Did you mean adding another attribute to the container annotation? If so, you may be right but I still think it would be confusing to appropriate it in this way.

@VladimirDmitrienko
Copy link
Contributor

Got it, thank you 🙂
Will keep you updated on my work.

@VladimirDmitrienko
Copy link
Contributor

Hi @marcphilipp

I've updated the PR and now it's ready for review.
Looking forward to your feedback.

Btw, I've set API since to 5.12 but actually I'm not really sure in which release it will go, please, let me know and I'll update since values and add release notes.

@VladimirDmitrienko
Copy link
Contributor

Hi @marcphilipp
I would be happy to improve my PR and waiting for a feedback from your side.
Maybe you have any at least approximate estimates of when could I expect that?
Would be glad to receive any news on this one.

@marcphilipp
Copy link
Member

marcphilipp commented Sep 18, 2024

Sorry for the delay! I was on vacation. I should be able to get to it this week. 🤞

@VladimirDmitrienko
Copy link
Contributor

@marcphilipp Thanks for the update 👍

@marcphilipp
Copy link
Member

Reopening to track polishing work

@marcphilipp marcphilipp reopened this Oct 7, 2024
marcphilipp added a commit that referenced this issue Oct 7, 2024
Instead of looking up annotations on the declaring and all enclosing
classes for each test method, each `ResourceLockAware` test descriptor
now delegates to the `ExclusiveResourceCollector` of its ancestors which
cache the class-level annotations and `ResourceLocksProvider` instances.

Resolves #2677.
@marcphilipp
Copy link
Member

Thanks for your contribution, @VladimirDmitrienko! 👍

@nickh-stripe
Copy link

nickh-stripe commented Nov 3, 2024

little late to the party here, looking at the new experimental api for this: https://junit.org/junit5/docs/snapshot/api/org.junit.jupiter.api/org/junit/jupiter/api/parallel/ResourceLocksProvider.html

was it purposeful to exclude the test template / parameterised tests? seems you can only provide dynamically for class/method and nested class and not a templated or parameterised test which is what I was hoping for for my usecase?

In my scenario i want to use a property of the parameterised test input to define the resource lock key for a resource shared amongst a subset of tests..

@VladimirDmitrienko
Copy link
Contributor

Hi @nickh-stripe

Actually ResourceLocksProvider can be used to define locks for parameterized tests.
However, the same locks will be applied to each invocation of the test, regardless of the test input.

Let's consider such an example:

	@ParameterizedTest
	@ValueSource(strings = {"banana", "apple", "orange"})
	void fruitTest(String value) {
		// ...
	}

	static class Provider implements ResourceLocksProvider {

		@Override
		public Set<Lock> provideForMethod(Class<?> testClass, Method testMethod) {
			return Set.of(new Lock("fruitKey"));
		}
	}

The same lock applies to each invocation: fruitTest("banana"), fruitTest("apple"), fruitTest("orange").

It's impossible to define different locks based on the arguments, for example, setting the same lock for "banana" and "apple" but a different one for "orange". The reason is that locks are collected before test execution started: we first collect the locks and only then pass them to engine that executes tests.


Assigning locks depending on arguments could be achieved by splitting the test:

	@ParameterizedTest
	@ValueSource(strings = {"banana", "apple"})
	void bananaAndAppleTest(String value) {
		// ...
	}

	@ParameterizedTest
	@ValueSource(strings = {"orange"})
	void orangeTest(String value) {
		// ...
	}

	static class Provider implements ResourceLocksProvider {

		@Override
		public Set<Lock> provideForMethod(Class<?> testClass, Method testMethod) {
			if (testMethod.getName().equals("bananaAndAppleTest")) {
				return Set.of(new Lock("bananaAndAppleKey"));
			}
			else if (testMethod.getName().equals("orangeTest")) {
				return Set.of(new Lock("orangeKey"));
			}
			return Set.of();
		}
	}

@nickh-stripe
Copy link

thanks for the detailed explanation! the approach makes sense but is not possible for the shape of our problem unfortunately, splitting the tests based on their locks isn't really feasible unfortunately so i'll have to think about a different way to achieve it. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment