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

Support some level of parallelization in junit-vintage-engine #4135

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

Conversation

YongGoose
Copy link
Contributor

Overview

Resolves #2229

This PR implements parallel execution support for the JUnit Vintage engine, addressing issue #2229.

  • Implemented concurrent execution of test descriptors when parallel execution is enabled.
  • Updated the JUnit Vintage engine to read parallel configuration parameters. (planned)

Open Question

I haven’t yet worked on the part that handles reading configuration parameters, such as whether parallel execution should be enabled or the number of threads to be used.
Is there a specific configuration format or approach you would recommend for this?


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


Definition of Done

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.

Thanks for submitting the draft! I left a bunch of comments. 🙂

Signed-off-by: yongjunhong <kevin0928@naver.com>
@YongGoose YongGoose force-pushed the feature/support-parallelization-in-junit-vintage-engine branch from c25db8c to e12b155 Compare November 19, 2024 17:11
@marcphilipp marcphilipp self-requested a review November 20, 2024 17:48
Issue: junit-team#3717
Signed-off-by: yongjunhong <kevin0928@naver.com>
@YongGoose
Copy link
Contributor Author

@marcphilipp

If the parallelization work is completed, I would appreciate it if you could take a look at the Open Question in the PR description 😀

@marcphilipp
Copy link
Member

@YongGoose The last two weeks were very busy. I should be able to get to your PRs early next week, though. Cheers!

@YongGoose YongGoose requested a review from marcphilipp December 4, 2024 06:28
@marcphilipp
Copy link
Member

I haven’t yet worked on the part that handles reading configuration parameters, such as whether parallel execution should be enabled or the number of threads to be used.
Is there a specific configuration format or approach you would recommend for this?

I think it should be similar to the configuration parameters in Jupiter. Thus, I'd suggest we use junit.vintage.execution.parallel.enabled and junit.vintage.execution.parallel.pool-size. WDYT?

Issue: junit-team#2229
Signed-off-by: yongjunhong <kevin0928@naver.com>
Issue: junit-team#2229
Signed-off-by: yongjunhong <kevin0928@naver.com>
@YongGoose
Copy link
Contributor Author

I think it should be similar to the configuration parameters in Jupiter. Thus, I'd suggest we use junit.vintage.execution.parallel.enabled and junit.vintage.execution.parallel.pool-size. WDYT?

That's a great idea! I made this change in e0312cc

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.

As a next step, could you please add some tests?

YongGoose and others added 2 commits December 6, 2024 22:06
…ntageTestEngine.java

Co-authored-by: Marc Philipp <mail@marcphilipp.de>
Issue: junit-team#2229
Signed-off-by: yongjunhong <kevin0928@naver.com>
@YongGoose
Copy link
Contributor Author

YongGoose commented Dec 14, 2024

@marcphilipp
While implementing the test, I came across a question and decided to leave a comment :)

In this test case, the return value of VintageEngineDescriptor.getModifiableChildren is the SuccessfulParallelTestCase class.

  • VintageTestEngine
private boolean executeInParallel(VintageEngineDescriptor engineDescriptor,
		EngineExecutionListener engineExecutionListener, ExecutionRequest request) {
	ExecutorService executorService = Executors.newFixedThreadPool(getThreadPoolSize(request));
	RunnerExecutor runnerExecutor = new RunnerExecutor(engineExecutionListener);

	List<CompletableFuture<Void>> futures = new ArrayList<>();
	for (Iterator<TestDescriptor> iterator = engineDescriptor.getModifiableChildren().iterator(); iterator.hasNext();) {
		TestDescriptor descriptor = iterator.next();
		CompletableFuture<Void> future = CompletableFuture.runAsync(() -> {
			runnerExecutor.execute((RunnerTestDescriptor) descriptor);
		}, executorService);

		futures.add(future);
		iterator.remove();
	}
  • TestCode
@Test
void successfulParallelTest(TestReporter reporter) {
	var events = executeInParallelSuccessfully(3, SuccessfulParallelTestCase.class).list();

	var startedTimestamps = getTimestampsFor(events, event(test(), started()));
	var finishedTimestamps = getTimestampsFor(events, event(test(), finishedSuccessfully()));

	reporter.publishEntry("startedTimestamps", startedTimestamps.toString());
	reporter.publishEntry("finishedTimestamps", finishedTimestamps.toString());

	assertThat(startedTimestamps).hasSize(3);
	assertThat(finishedTimestamps).hasSize(3);
}
  • SuccessfulParallelTestCase
@RunWith(Enclosed.class)
public class JUnit4ParallelTestCase {

	public static class SuccessfulParallelTestCase {
		static AtomicInteger sharedResource;
		static CountDownLatch countDownLatch;

		@BeforeClass
		public static void initialize() {
			sharedResource = new AtomicInteger();
			countDownLatch = new CountDownLatch(3);
		}

		@Test
		public void firstTest() throws Exception {
			incrementAndBlock(sharedResource, countDownLatch);
		}

		@Test
		public void secondTest() throws Exception {
			incrementAndBlock(sharedResource, countDownLatch);
		}

		@Test
		public void thirdTest() throws Exception {
			incrementAndBlock(sharedResource, countDownLatch);
		}
	}
    ...

image
image

Question

  • If the TestDescriptor has child TestDescriptors as in the case above, could it be improved more efficiently through recursion?

If a test class contains a large number of test methods, I expect that recursion could make the process more efficient.

I believe the commit will make it easier to understand. 1484347


While implementing recursion, I noticed something unusual.

When fetching child TestDescriptor objects using getChildren from a TestDescriptor, it returns instances of VintageTestDescriptor.
As a result, I found that casting them to RunnerTestDescriptor is not possible.

If that’s the case, it seems unavoidable to modify the internal code to implement parallel execution of child TestDescriptor objects through recursion.

Alternatively, we could proceed with the current approach instead of recursion, without modifying the internal code. WDYT?

@YongGoose YongGoose force-pushed the feature/support-parallelization-in-junit-vintage-engine branch from de8fbce to 1484347 Compare December 15, 2024 03:48
@marcphilipp
Copy link
Member

marcphilipp commented Dec 16, 2024

Question

  • If the TestDescriptor has child TestDescriptors as in the case above, could it be improved more efficiently through recursion?

If a test class contains a large number of test methods, I expect that recursion could make the process more efficient.

Do you mean that running the children of a RunnerTestDescriptor concurrently instead of just the top-level RunnerTestDescriptors could speed up the overall execution? If so, I agree with that. However, that's not generally possible as the Runner orchestrates the execution of its children, not the VintageTestEngine.

JUnit 4 actually has a feature that allows running methods in parallel. It works for all subclasses of ParentRunner by setting a RunnerScheduler. We could use that to inject our own scheduler that uses the same ExecutorService. However, I think we should do that in a separate PR and focus on running test classes in parallel in this PR. Thus, you'll have to adjust the tests you've added to not check that methods can be run in parallel but classes.

@YongGoose
Copy link
Contributor Author

Do you mean that running the children of a RunnerTestDescriptor concurrently instead of just the top-level RunnerTestDescriptors could speed up the overall execution?

Exactly!

JUnit 4 actually has a feature that allows running methods in parallel. It works for all subclasses of ParentRunner by setting a RunnerScheduler. We could use that to inject our own scheduler that uses the same ExecutorService. However, I think we should do that in a separate PR and focus on running test classes in parallel in this PR.

I'll work on it in the next PR along with the issue once this PR is complete!

Thus, you'll have to adjust the tests you've added to not check that methods can be run in parallel but classes.

I was actually considering this issue, so thank you for the detailed suggestion!👍🏻
I'll proceed with the approach you recommended.

Issue: junit-team#2229
Signed-off-by: yongjunhong <kevin0928@naver.com>
@YongGoose YongGoose force-pushed the feature/support-parallelization-in-junit-vintage-engine branch from 1484347 to b196388 Compare December 21, 2024 11:59
@YongGoose
Copy link
Contributor Author

Thus, you'll have to adjust the tests you've added to not check that methods can be run in parallel but classes.

I implemented it by extracting and comparing thread names when executing test classes. b196388

I understand that in JUnit-Jupiter, threads can be recorded and verified using ThreadReporter.
Therefore, I would like to modularize the code for recording threads.

  • If you have any suggestions on how to achieve this, please let me know :)

@marcphilipp
Copy link
Member

Thanks! I don't think we can reuse that here since we have to use JUnit 3/4 primitives and can't use Jupiter extensions. I'll be out for about a week for Christmas but will suggest something when I'm back.

@YongGoose
Copy link
Contributor Author

Thanks! I don't think we can reuse that here since we have to use JUnit 3/4 primitives and can't use Jupiter extensions. I'll be out for about a week for Christmas but will suggest something when I'm back.

Have a good Christmas :)

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.

Support some level of parallelization in junit-vintage-engine
2 participants