Provide mechanism for managing resources across engines and executions #4281

YongGoose · 2025-01-27T11:42:17Z

Resolves #2816

Overview

Designing a class to manage context at the platform level
Adding a session-level store to LauncherSession
Adding a request-level store to Launcher
Exposing the request-level store in ~~EngineDiscoveryRequest and TestPlan~~ ExecutionRequest
Utilizing the request-level store in ExtensionContext
Tests for Jupiter, Suite engine, EngineTestKit etc.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes

Issue: junit-team#2816 Signed-off-by: yongjunhong <kevin0928@naver.com>

Issue: junit-team#2816

Issue: junit-team#2816 Signed-off-by: yongjunhong <kevin0928@naver.com>

Issue: junit-team#2816

This reverts commit 4de6db8.

Issue: junit-team#2816

YongGoose · 2025-01-27T12:00:54Z

What's the use case for this method? Test engines don't have access to the Launcher so we'll need to pass this store as part of LauncherDiscoveryRequest and ExecutionRequest, don't we?

I’ve drafted an initial version 02ef45b , but I think there’s still a lot of room for improvement.
If you could point out any mistakes I’ve made, I’ll make sure to fix them right away.

Even though I’ve been studying, I find it quite challenging.

PS. While resolving a git conflict, I accidentally included the commit history from the main branch, So i create a new PR 😁

YongGoose · 2025-01-27T12:12:29Z

@marcphilipp

I think you must be very busy with the release of version 5.12 M1.
Please feel free to leave your comments whenever it’s convenient for you.

YongGoose · 2025-02-06T10:53:01Z

@marcphilipp

~~This PR is something I’ve personally been eager to work on for a long time.~~
~~However, I still don’t have a clear sense of how to proceed with it.~~

~~If this feature gets added to JUnit 5, it would be a huge learning opportunity for me and a great addition to my skill set.~~
~~That said, I don’t want to cause any fatigue for the JUnit team—especially you, Marc.~~

~~So if you feel that I’m not quite ready to take on this PR yet, please let me know, and I’ll close it.~~
~~Either way, I’ll keep looking for other issues and do my best to contribute! 🙂~~

I think I sounded a bit too weak.
After completing the other PR, I will make sure to implement this perfectly, no matter what.

~~But if you’re willing to let me see it through, I’ll do my best as always.~~
~~I’ll likely need more guidance than usual compared to other tasks.~~

marcphilipp · 2025-02-09T10:08:27Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java

+	 * @return the store for session-level data
+	 * @since 5.13
+	 */
+	Store getSessionLevelStore();


These also need an @API annotation:

@API(status = EXPERIMENTAL, since = "5.13")

Done in 33f5fb8

...ngine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java

junit-platform-engine/src/main/java/org/junit/platform/engine/Namespace.java

marcphilipp · 2025-02-09T10:11:57Z

junit-platform-engine/src/main/java/org/junit/platform/engine/Namespace.java

+ * https://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.platform.engine;


Can we move this to the same packaged as NamespacedHierarchicalStore?

Done in 9fd6fd8 !

marcphilipp · 2025-02-09T10:13:38Z

junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncher.java

@@ -59,6 +65,8 @@ class DefaultLauncher implements Launcher {
 			"PostDiscoveryFilter array must not contain null elements");
 		this.discoveryOrchestrator = new EngineDiscoveryOrchestrator(testEngines,
 			unmodifiableCollection(postDiscoveryFilters), listenerRegistry.launcherDiscoveryListeners);
+		this.launcherSession = LauncherFactory.openSession(config);


We should not open a session here but pass in the sessionStore to the constructor.

marcphilipp · 2025-02-09T10:15:28Z

...platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncherSession.java

@@ -43,6 +45,7 @@ public void close() {
 	private final LauncherInterceptor interceptor;
 	private final LauncherSessionListener listener;
 	private final DelegatingLauncher launcher;
+	private final NamespacedHierarchicalStore<Namespace> store;

 	DefaultLauncherSession(List<LauncherInterceptor> interceptors, Supplier<LauncherSessionListener> listenerSupplier,
 			Supplier<Launcher> launcherSupplier) {


This probably needs to be changed to the following:

Suggested change

Supplier<Launcher> launcherSupplier) {

Function<NamespacedHierarchicalStore<Namespace>, Launcher> launcherFactory) {

I'm curious about the reason for changing from Supplier to Function.

Also, I'd like to understand the purpose of this variable.
More specifically, what kind of value is intended to be generated through the apply method?

This is to make creating the Launcher lazy so it can be intercepted (see else branch below).

Since it now needs to be passed the session-level store to its constructor, I think we'll need to pass that in.

Based on the comments you left on the PR and the comments in the issue, I've made some progress on the implementation.

Provide mechanism for managing resources across engines and executions #2816 (comment)

Provide mechanism for managing resources across engines and executions #2816 (comment)

I've marked the parts where I'm still unsure about how to pass things with TODOs.

Thanks to your help, I'm starting to get a better understanding. I'll keep working hard. Thank you!

…Namespace.java Co-authored-by: Marc Philipp <mail@marcphilipp.de>

…her/core/DefaultLauncherSession.java Co-authored-by: Marc Philipp <mail@marcphilipp.de>

Issue: junit-team#2816

YongGoose · 2025-02-12T14:43:49Z

...piter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java

+	@Override
+	public Store getSessionLevelStore() {
+		return getStore(Namespace.create("session"));
+	}
+
+	@Override
+	public Store getRequestLevelStore() {
+		return getStore(Namespace.create("request"));
+	}


@marcphilipp

Is this the correct approach to creating session and request stores?

I understand that a Namespace is distinguished based on its name. However, I'm not sure if it's appropriate to create a Namespace by directly using the names of the session and request.

No, the namespace is an orthogonal concept here. Instead these methods should take a Namespace parameter. Sorry for omitting that in #2816 (comment)! They should return an adapted (they use a different Namespace type and Store interface) version of the NamespacedHierarchicalStore objects passed to the Jupiter engine. Please let me know if I should provide more details.

Sorry for omitting that in #2816 (comment)!

No, I think I should have asked earlier. 🥹

No, the namespace is an orthogonal concept here.

May I ask about the concept of Namespace?

Also, how does the Namespace in jupiter.api.extension differ from the one I created?
A more overall detailed explanation would be really helpful for my understanding. 🙂

@marcphilipp

I studied on my own and found some answers.

Namespace is used to define the scope of data within a Store.
From what I understand, Jupiter uses a different Store, which is why it also requires a different type of Namespace. And as you suggested, I’ve modified the method to take Namespace as a parameter. Please review it when you have a chance. 5d2c2b8

However, I still don’t fully understand the NamespacedHierarchicalStore that is passed to the Jupiter engine.

Utilizing the request-level store in JupiterEngineExtensionContext

I expect this to be related to the following steps outlined in the ReadMe.

Namespace is used to define the scope of data within a Store.

That's correct. The original intention was that Jupiter extensions could store data in the Store without worrying about unintended side-effects (for example, another extension storing something else under the same key) while also allowing extensions to share state by using the same Namespace on purpose.

From what I understand, Jupiter uses a different Store, which is why it also requires a different type of Namespace.

That's correct, but Jupiter's implementation uses NamespacedHierarchicalStore internally.

I pushed 4624808 and bac5250 to outline what I mean. ExecutionRequest is still using the wrong NamespacedHierarchicalStore, though, and I didn't add any tests.

ExecutionRequest is still using the wrong NamespacedHierarchicalStore, though, and I didn't add any tests.

I’ll take care of that task myself!

ExecutionRequest is still using the wrong NamespacedHierarchicalStore

First, I decided to modify the NamespacedHierarchicalStore in ExecutionRequest and then implement test cases.

ExecutionRequest is created by the EngineExecutionOrchestrator initialized by the Launcher. At this time, the store in ExecutionRequest serves as a request-level storage.

Accordingly, I implemented it so that ExecutionRequest receives the RequestLevelStore created in DefaultLauncher. 6b4ee03

Issue: junit-team#2816

YongGoose · 2025-02-13T11:24:23Z

junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/LauncherFactory.java

@@ -63,6 +65,8 @@
 @API(status = STABLE, since = "1.0")
 public class LauncherFactory {

+	private static final NamespacedHierarchicalStore<Namespace> SESSION_STORE = new NamespacedHierarchicalStore<>(null);


@marcphilipp

After much consideration, I’ve decided to focus on the Launcher part first.

To proceed, I need feedback on how LauncherFactory creates Launcher and LauncherSession.

My current approach is as follows:
LauncherFactory creates a session-level store and passes it to Launcher, setting it as the parent store for the request-level store.
And, LauncherSession receives the session-level store.

I’d like to know if this approach makes sense and whether the code is appropriate. I’d appreciate your feedback!

Since the stores need to be closed, we need to attach them to the LauncherSession. I pushed 60f66f5 to illustrate what I mean.

That was exactly the approach I had in mind!

Thank you so much. 🙇🏻

Issue: junit-team#2816

...orm-launcher/src/main/java/org/junit/platform/launcher/core/EngineExecutionOrchestrator.java

platform-tests/src/test/java/org/junit/platform/launcher/core/DefaultLauncherTests.java

YongGoose

Removing the store from the discovery process sounds like a good idea.
As the next step, I will proceed with the following task.

Tests for Jupiter, Suite engine, EngineTestKit etc.

Thank you, Marc!

junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncher.java

YongGoose · 2025-02-25T10:07:37Z

...platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncherSession.java

+	@Override
+	public NamespacedHierarchicalStore<Namespace> getStore() {
+		return store;
+	}


Suggested change

@Override

public NamespacedHierarchicalStore<Namespace> getStore() {

return store;

}

@Override

public NamespacedHierarchicalStore<Namespace> getSessionLevelStore() {

return sessionLevelStore;

}

WDYT?

I'm torn. On one hand that would make it more consistent, on the other hand, what else should it be? 🤔

The store in DefaultLauncherSession is only used for creating the Launcher.
So, setting it as store would be fine, but for consistency, I set it as sessionLevelStore.

Hmm... I can't think of any other approach at the moment. 😬
For now, shall we put this on hold and decide once we have a clearer answer?

YongGoose · 2025-02-25T10:08:40Z

...platform-launcher/src/main/java/org/junit/platform/launcher/core/DefaultLauncherSession.java

+	private final NamespacedHierarchicalStore<Namespace> store = new NamespacedHierarchicalStore<>(null,
+		closeAutoCloseables());


Suggested change

private final NamespacedHierarchicalStore<Namespace> store = new NamespacedHierarchicalStore<>(null,

closeAutoCloseables());

private final NamespacedHierarchicalStore<Namespace> sessionLevelStore = new NamespacedHierarchicalStore<>(null,

closeAutoCloseables());

YongGoose · 2025-02-25T10:09:44Z

junit-platform-launcher/src/main/java/org/junit/platform/launcher/LauncherSession.java

@@ -47,4 +49,6 @@ public interface LauncherSession extends AutoCloseable {
 	@Override
 	void close();

+	NamespacedHierarchicalStore<Namespace> getStore();


Suggested change

NamespacedHierarchicalStore<Namespace> getStore();

NamespacedHierarchicalStore<Namespace> getSessionLevelStore();

Issue: junit-team#2816

YongGoose · 2025-03-03T05:48:32Z

@marcphilipp

While working on the tests, I have a question.

As per your guidance, I am adding tests for JupiterEngine, SuiteEngine, and EngineKit. However, I am wondering if SuiteEngine requires furthermore testing.

Since the main focus of this PR is adding request-level and session-level stores, I am designing the tests around that.

The execute method in SuiteEngine utilizes the requestLevelStore from ExecutionRequest to run multiple tests. In my view, the core role of execute is to execute multiple tests, and this functionality is already being validated by existing tests.

Do you think any additional tests are needed for SuiteEngine?

PS. I truly appreciate how you not only explain things but also implement the code to make communication more efficient—it’s been incredibly helpful. 👍🏻
However, this time, I’d like to try implementing it on my own! 🙂

YongGoose and others added 13 commits January 27, 2025 20:31

Generate ResourceContext for store resources

9bbe717

Issue: junit-team#2816 Signed-off-by: yongjunhong <kevin0928@naver.com>

Add getStore method in LauncherSession

f4d7f6b

Issue: junit-team#2816 Signed-off-by: yongjunhong <kevin0928@naver.com>

Add Namespace by reusing Jupiter's Namespace class

17bcccb

Issue: junit-team#2816

Apply comment

4b766ad

Issue: junit-team#2816

Adding a request-level store to Launcher

6ad80de

Issue: junit-team#2816

Apply comment

d5528fb

Issue: junit-team#2816

Generate ResourceContext for store resources

7bae8bc

Issue: junit-team#2816 Signed-off-by: yongjunhong <kevin0928@naver.com>

Add Namespace by reusing Jupiter's Namespace class

28ac373

Issue: junit-team#2816

Apply comment

4130c36

Issue: junit-team#2816

Adding a request-level store to Launcher

e6a8543

Issue: junit-team#2816

Apply comment

bdf400d

Issue: junit-team#2816

Revert "Apply comment"

5d3a14b

This reverts commit 4de6db8.

Apply comment

02ef45b

Issue: junit-team#2816

YongGoose force-pushed the feature/2816 branch from f6fe9f1 to 02ef45b Compare January 27, 2025 11:52

YongGoose mentioned this pull request Feb 1, 2025

Implement Parallel Method Execution in JUnit-Vintage engine #4242

Merged

6 tasks

marcphilipp self-requested a review February 1, 2025 10:38

marcphilipp reviewed Feb 9, 2025

View reviewed changes

YongGoose and others added 4 commits February 12, 2025 19:15

Update junit-platform-engine/src/main/java/org/junit/platform/engine/…

f7e5996

…Namespace.java Co-authored-by: Marc Philipp <mail@marcphilipp.de>

Update junit-platform-launcher/src/main/java/org/junit/platform/launc…

ae5a110

…her/core/DefaultLauncherSession.java Co-authored-by: Marc Philipp <mail@marcphilipp.de>

Add API annotation

33f5fb8

Issue: junit-team#2816

Move Namespace class

9fd6fd8

Issue: junit-team#2816

YongGoose commented Feb 12, 2025

View reviewed changes

YongGoose added 2 commits February 13, 2025 00:05

Apply comment

42a0089

Issue: junit-team#2816

Merge branch 'main' into feature/2816

5d2424f

YongGoose force-pushed the feature/2816 branch 2 times, most recently from e71438b to c9d5ac0 Compare February 13, 2025 10:40

Apply Namespace parameter

7d98e9a

Issue: junit-team#2816

YongGoose force-pushed the feature/2816 branch from c9d5ac0 to 7d98e9a Compare February 13, 2025 10:48

YongGoose commented Feb 13, 2025

View reviewed changes

YongGoose requested a review from marcphilipp February 13, 2025 12:06

YongGoose and others added 7 commits February 14, 2025 11:55

Merge branch 'main' into feature/2816

fd9023e

Outline management of session-/request-level stores

60f66f5

Use session-/request-level store in Jupiter engine

4624808

Use request-level store as parent of engine-level store

bac5250

fixup! Outline management of session-/request-level stores

9c24186

Fix test

972ad0a

Fix ExecutionRequest's NamespacedHierarchicalStore

6b4ee03

Issue: junit-team#2816

YongGoose commented Feb 18, 2025

View reviewed changes

...orm-launcher/src/main/java/org/junit/platform/launcher/core/EngineExecutionOrchestrator.java Outdated Show resolved Hide resolved

Merge branch 'main' into feature/2816

d72090c

YongGoose force-pushed the feature/2816 branch from 7b47567 to d72090c Compare February 21, 2025 10:43

YongGoose and others added 4 commits February 23, 2025 08:18

Merge branch 'main' into feature/2816

3f56d7d

Fix post-merge compile error

b2c95c4

Limit session-/request-level store availability to execution

e0a43ef

Remove broken Javadoc link

92abb5d

marcphilipp reviewed Feb 24, 2025

View reviewed changes

platform-tests/src/test/java/org/junit/platform/launcher/core/DefaultLauncherTests.java Outdated Show resolved Hide resolved

YongGoose commented Feb 25, 2025

View reviewed changes

YongGoose added 6 commits February 25, 2025 19:22

Merge branch 'main' into feature/2816

18148d5

Revert changes

62bcb8f

Add JupiterEngineTests

9fd3ba2

Merge branch 'main' into feature/2816

efcc719

Merge branch 'main' into feature/2816

b3877c7

Add NamespaceTests

1702fbf

Issue: junit-team#2816

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide mechanism for managing resources across engines and executions #4281

Provide mechanism for managing resources across engines and executions #4281

YongGoose commented Jan 27, 2025 •

edited by marcphilipp

Loading

YongGoose commented Jan 27, 2025 •

edited

Loading

YongGoose commented Jan 27, 2025

YongGoose commented Feb 6, 2025 •

edited

Loading

marcphilipp Feb 9, 2025

YongGoose Feb 12, 2025

marcphilipp Feb 9, 2025

YongGoose Feb 12, 2025

marcphilipp Feb 9, 2025

marcphilipp Feb 9, 2025

YongGoose Feb 12, 2025

marcphilipp Feb 12, 2025

marcphilipp Feb 12, 2025

YongGoose Feb 12, 2025

YongGoose Feb 12, 2025

marcphilipp Feb 12, 2025

YongGoose Feb 13, 2025

YongGoose Feb 13, 2025 •

edited

Loading

marcphilipp Feb 16, 2025

marcphilipp Feb 16, 2025 •

edited

Loading

YongGoose Feb 17, 2025

YongGoose Feb 18, 2025

YongGoose Feb 13, 2025 •

edited

Loading

marcphilipp Feb 16, 2025

YongGoose Feb 17, 2025

YongGoose left a comment •

edited

Loading

YongGoose Feb 25, 2025

marcphilipp Feb 25, 2025

YongGoose Feb 25, 2025

YongGoose Feb 25, 2025

marcphilipp Feb 25, 2025

YongGoose Feb 25, 2025

marcphilipp Feb 25, 2025

YongGoose commented Mar 3, 2025

	Supplier<Launcher> launcherSupplier) {
	Function<NamespacedHierarchicalStore<Namespace>, Launcher> launcherFactory) {

		private final NamespacedHierarchicalStore<Namespace> store = new NamespacedHierarchicalStore<>(null,
		closeAutoCloseables());

	NamespacedHierarchicalStore<Namespace> getStore();
	NamespacedHierarchicalStore<Namespace> getSessionLevelStore();

Provide mechanism for managing resources across engines and executions #4281

Are you sure you want to change the base?

Provide mechanism for managing resources across engines and executions #4281

Conversation

YongGoose commented Jan 27, 2025 • edited by marcphilipp Loading

Overview

Definition of Done

YongGoose commented Jan 27, 2025 • edited Loading

YongGoose commented Jan 27, 2025

YongGoose commented Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp Feb 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YongGoose commented Mar 3, 2025

YongGoose commented Jan 27, 2025 •

edited by marcphilipp

Loading

YongGoose commented Jan 27, 2025 •

edited

Loading

YongGoose commented Feb 6, 2025 •

edited

Loading

YongGoose Feb 13, 2025 •

edited

Loading

marcphilipp Feb 16, 2025 •

edited

Loading

YongGoose Feb 13, 2025 •

edited

Loading

YongGoose left a comment •

edited

Loading