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

Provide mechanism for managing resources across engines and executions #4281

Draft
wants to merge 69 commits into
base: main
Choose a base branch
from

Conversation

YongGoose
Copy link
Contributor

@YongGoose YongGoose commented Jan 27, 2025

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

Sorry, something went wrong.

@YongGoose
Copy link
Contributor Author

YongGoose commented Jan 27, 2025

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

YongGoose commented Feb 6, 2025

@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.

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

Choose a reason for hiding this comment

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

These also need an @API annotation:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 33f5fb8

@YongGoose YongGoose force-pushed the feature/2816 branch 3 times, most recently from c9d5ac0 to 7d98e9a Compare February 13, 2025 10:48
Copy link
Contributor Author

@YongGoose YongGoose left a comment

Choose a reason for hiding this comment

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

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!

@YongGoose
Copy link
Contributor Author

@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! 🙂

@marcphilipp
Copy link
Member

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?

I was thinking about a very basic tests that verifies that SuiteEngine passes along the requestLevelStore to test engines. It's probably sufficient to mock SuiteLauncher and check that a non-null store is passed to it.

YongGoose and others added 10 commits March 8, 2025 15:37

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong
Issue: junit-team#2816
Signed-off-by: yongjunhong <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong
Issue: junit-team#2816
Signed-off-by: yongjunhong <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong
Issue: junit-team#2816
Signed-off-by: yongjunhong <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@YongGoose
Copy link
Contributor Author

@marcphilipp

I have a seminar presentation tomorrow, so it will be difficult for me to work on this.
Apologies for the delay.

I will complete the work by next week or this weekend and request a review then.

@marcphilipp
Copy link
Member

No worries! Good luck for your presentation!

polishing

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
polishing

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong
@YongGoose
Copy link
Contributor Author

YongGoose commented Mar 17, 2025

@marcphilipp

Currently, the dummyTest in JupiterTest is failing. This happens because JupiterTest runs in isolation, attempting to retrieve a value from session-level-store without first inserting it, resulting in a NullPointerException.

One possible solution is to modify JupiterExtensionExample to allow null values to pass, but I don’t think this is the best approach.

WDYT?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@marcphilipp
Copy link
Member

marcphilipp commented Mar 18, 2025

The convention we have adopted after LauncherFactoryTests was initially written is to name test classes that should not be run directly (for example, because they're used by a test via EngineTestKit or directly via the Launcher) with a TestCase suffix. This avoids them being picked up due to this config:

So I'd suggest renaming JupiterTest to JupiterTestCase accordingly. This also applies to the JUnit4Example and JUnit5Example classes.

YongGoose and others added 2 commits March 19, 2025 06:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…LauncherFactoryTests.java

Co-authored-by: Marc Philipp <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@YongGoose
Copy link
Contributor Author

The convention we have adopted after LauncherFactoryTests was initially written is to name test classes that should not be run directly (for example, because they're used by a test via EngineTestKit or directly via the Launcher) with a TestCase suffix. This avoids them being picked up due to this config:

So I'd suggest renaming JupiterTest to JupiterTestCase accordingly. This also applies to the JUnit4Example and JUnit5Example classes.

Done in e56946f !

@YongGoose
Copy link
Contributor Author

@marcphilipp

I'm planning to move on to the documentation work. 🚀
Would it be possible to check if there's anything else that should be added to the tests?

Also, if you could recommend which part of file should be updated or where a new document should be added, that would be really helpful.

Writing documentation is always a challenge! 😅

Verified

This commit was signed with the committer’s verified signature.
YongGoose Yongjun Hong

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

Would it be possible to check if there's anything else that should be added to the tests?

The main thing I'm missing is integration tests that verifies AutoCloseable objects put into the request-level store get closed after execution and those put into the session-level store get closed when the LauncherSession is closed.

Also, if you could recommend which part of file should be updated or where a new document should be added, that would be really helpful.

We should update the usage example to store HttpServer (or a wrapper, if necessary) in the session-level store, retrieve it from a new ParameterResolver extension and inject it into the test rather than using system properties.

Moreover, we should add a new "Managing State Across Test Engines" subsection to JUnit Platform Launcher API where we provide an example of two test engines lazily creating and sharing a resource (maybe the same HttpServer or a wrapper thereof as in the other example?) regardless of which is executed first.

*
* @see #ClassExtensionContext(ExtensionContext, EngineExecutionListener, ClassBasedTestDescriptor, Lifecycle, JupiterConfiguration, ExtensionRegistry, LauncherStoreFacade, ThrowableCollector)
*/
ClassExtensionContext(ExtensionContext parent, EngineExecutionListener engineExecutionListener,
Copy link
Member

Choose a reason for hiding this comment

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

This is unused, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be unused.
I'll remove it!

import org.apiguardian.api.API;
import org.junit.platform.commons.util.Preconditions;

@API(status = EXPERIMENTAL, since = "5.13")
Copy link
Member

Choose a reason for hiding this comment

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

This should be 1.13 since it's in a Platform module.

import org.junit.platform.commons.util.Preconditions;

@API(status = EXPERIMENTAL, since = "5.13")
public class Namespace {
Copy link
Member

Choose a reason for hiding this comment

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

Please add class-level Javadoc here.

* existing sequence of parts in this namespace.
*
* @return new namespace; never {@code null}
* @since 5.13
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to repeat this for a method if it's identical to the class' @since tag. Please remove it.

* @return new namespace; never {@code null}
* @since 5.13
*/
@API(status = STABLE, since = "5.13")
Copy link
Member

Choose a reason for hiding this comment

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

This should also be removed to "inherit" it from the class.

static class JupiterExtensionExample implements BeforeAllCallback {
@Override
public void beforeAll(ExtensionContext context) {
var value = context.getRoot().getStore(ExtensionContext.Namespace.GLOBAL).get("testKey");
Copy link
Member

Choose a reason for hiding this comment

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

There should be no need to call getRoot() here. Please also check that we can get the value from context.getSessionLevelStore(ExtensionContext.Namespace.GLOBAL) as well.

@@ -73,11 +83,17 @@ LauncherSessionListener getListener() {
public void close() {
if (launcher.delegate != ClosedLauncher.INSTANCE) {
launcher.delegate = ClosedLauncher.INSTANCE;
store.close();
listener.launcherSessionClosed(this);
Copy link
Member

Choose a reason for hiding this comment

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

To allow the listener to read things from the store, we should first call the listener and then close the store.

@@ -333,6 +338,29 @@ public void executionFinished(TestIdentifier testIdentifier, TestExecutionResult
}));
}

@Test
void extensionCanReadValueFromSessionStore() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also test that an extension can store a value in the session-level store that can be read by a LauncherSessionListener in launcherSessionClosed().

NamespacedHierarchicalStoreProviders.dummyNamespacedHierarchicalStore());

engineWriter.execute(request);
engineReader.execute(request);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should go through Launcher.execute here rather than calling TestEngine.execute directly. That way, we would also test the Launcher's handling of the store meets our expectations.


new SuiteTestEngine().execute(request);

verify(mockDescriptor).execute(listener, requestLevelStore);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make the assertion a bit stronger to avoid accidentally depending on any equals() implementation.

Suggested change
verify(mockDescriptor).execute(listener, requestLevelStore);
verify(mockDescriptor).execute(same(listener), same(requestLevelStore));

@YongGoose
Copy link
Contributor Author

The main thing I'm missing is integration tests that verifies AutoCloseable objects put into the request-level store get closed after execution and those put into the session-level store get closed when the LauncherSession is closed.

Thank you so much for your detailed feedback on my work! 🙇🏻‍♂️🙇🏻‍♂️
Since you've reviewed it thoroughly, I'll make sure to refine my work carefully before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide mechanism for managing resources across engines and executions
2 participants