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

Implement Parallel Method Execution in JUnit-Vintage engine #4242

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

Conversation

YongGoose
Copy link
Contributor

@YongGoose YongGoose commented Jan 11, 2025

Resolves: #4238

Overview

Implements parallel execution of test methods in the junit-vintage-engine, inspired by the parallelization support available in JUnit 4 through ParallelComputer.

  • This feature allows for enhanced test execution performance by leveraging modern multi-threaded environments.

Changes

  1. Implemented Custom RunnerScheduler
  • Created a RunnerScheduler using ExecutorService.
  • Ensures proper scheduling and execution of test methods in parallel.
  1. Updated Existing Tests
  • Adjusted tests to verify the parallel execution of methods.
  • Added new test cases to ensure backward compatibility and correctness.
  1. Documentation Updates (Not yet)
  • Added documentation on how to enable and configure parallel method execution.
  • Explained performance implications and scenarios where parallelization is beneficial.

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


Definition of Done

@YongGoose
Copy link
Contributor Author

@marcphilipp

I created this Draft PR to ask a few questions.
First, I’ve added two independent configuration parameters as you suggested.

However, I have a question regarding their behavior:

  • What happens if the user sets the junit.vintage.execution.parallel.enabled parameter to true, but both junit.vintage.execution.parallel.classes and junit.vintage.execution.parallel.methods are set to false?

  • Additionally, what occurs if junit.vintage.execution.parallel.enabled is set to false, while junit.vintage.execution.parallel.classes or junit.vintage.execution.parallel.methods set to true?

@marcphilipp
Copy link
Member

What happens if the user sets the junit.vintage.execution.parallel.enabled parameter to true, but both junit.vintage.execution.parallel.classes and junit.vintage.execution.parallel.methods are set to false?

I think we should check for that, log a warning (like we do for invalid configuration parameters in Jupiter), and continue as if junit.vintage.execution.parallel.enabled was false.

Additionally, what occurs if junit.vintage.execution.parallel.enabled is set to false, while junit.vintage.execution.parallel.classes or junit.vintage.execution.parallel.methods set to true?

I think turning off parallel execution (using the ...enabled=false parameter) while keeping the rest of the parallel configuration in place is a valid use case. Therefore, I think we should ignore the ...classes and ...methods parameters in this case without logging a warning.

@YongGoose
Copy link
Contributor Author

What happens if the user sets the junit.vintage.execution.parallel.enabled parameter to true, but both junit.vintage.execution.parallel.classes and junit.vintage.execution.parallel.methods are set to false?

I think we should check for that, log a warning (like we do for invalid configuration parameters in Jupiter), and continue as if junit.vintage.execution.parallel.enabled was false.

Additionally, what occurs if junit.vintage.execution.parallel.enabled is set to false, while junit.vintage.execution.parallel.classes or junit.vintage.execution.parallel.methods set to true?

I think turning off parallel execution (using the ...enabled=false parameter) while keeping the rest of the parallel configuration in place is a valid use case. Therefore, I think we should ignore the ...classes and ...methods parameters in this case without logging a warning.

Done in be5234f !
If you have a better approach, please let me know in the comments 🙂

@YongGoose
Copy link
Contributor Author

YongGoose commented Jan 14, 2025

@marcphilipp

I implemented this to get feedback on parallelization at the class and method levels. 0b70615

The code is intended to facilitate understanding, so it hasn't been fully polished yet❕

There are three scenarios when running parallel tests:

  1. Running methods in parallel.
  2. Running classes in parallel.
  3. Running both methods and classes in parallel.

Currently facing challenges with designing for these scenarios.

I want to design it similarly to JUnit 4's ParallelComputer, but I need help..🥹

Comment on lines 170 to 171
executorService.shutdown();
executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
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 shutting down the thread pool here is not correct because this will be called for each runner plus we're also using it if classes is true as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the comment you provided.

Based on that, I removed the shutdown and only used the awaitTermination method.
Then, I added a conditional statement to check if it completes within the given time. e92e834

Upon testing, I found that not all tests finished within the specified time.

As a result, I implemented the finished method to do nothing, but this caused the assertion comparing the start and end times to fail.

Could you provide a hint about what kind of work might need to be done inside the finished method?

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 need to wait for all futures scheduled for a particular runner. I have addressed that in the commit I've pushed to your branch just now.

* @since 5.13
*/
@API(status = INTERNAL, since = "5.13")
public interface RunnerScheduler {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use org.junit.runners.model.RunnerScheduler directly?

Copy link
Contributor Author

@YongGoose YongGoose Jan 15, 2025

Choose a reason for hiding this comment

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

Directly using org.junit.runners.model.RunnerScheduler seems like a good approach.

However, when running the junit-vintage-engine:testWithoutJUnit4 task, an exception occurs because it fails to find org.junit.runners.model.RunnerScheduler.

  • Exception message
* What went wrong:
Execution failed for task ':junit-vintage-engine:testWithoutJUnit4'.
> Failure during test discovery: Forked test JVM terminated unexpectedly with exit value 1
  Test Distribution has stopped executing any remaining tests and silently skips them!
  For potential reasons why the test JVM exited, check the troubleshooting section at https://gradle.com/help/test-distribution-troubleshooting.
  Standard error from JVM:
      Terminating due to fatal error
      java.lang.NoClassDefFoundError: org/junit/runners/model/RunnerScheduler
      	at java.base/java.lang.Class.getDeclaredConstructors0(Native Method)

So, I’m considering restoring it again. What are your thoughts on that? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a commit that fixes that. Instead of referencing RunnerScheduler in VintageTestEngine, it passes ExecutorService to RunnerTestDescriptor and creates the RunnerScheduler in there.

…scriptor/RunnerTestDescriptor.java

Co-authored-by: Marc Philipp <[email protected]>
@YongGoose YongGoose force-pushed the feature/4238 branch 4 times, most recently from 4da4954 to 29e5e11 Compare January 15, 2025 10:58
@YongGoose
Copy link
Contributor Author

YongGoose commented Jan 18, 2025

@marcphilipp

While writing test code, I discovered an issue caused by using the same ExecutorService.

I concluded that the test did not complete due to a deadlock.

Assumption: Lets assume the thread pool size of the ExecutorService is 2.

Scenario:

  1. Two tests, A and B, are initiated in parallel from an outer loop.
  2. Test A internally attempts to execute two subtasks (A1, A2).
  3. Similarly, Test B internally attempts to execute two subtasks (B1, B2).
  4. A starts A1, and B starts B1. At this point, all threads in the pool are occupied.
  5. A waits for A2 to complete, and B waits for B2 to complete.
  6. However, A2 and B2 cannot start because there are no available threads in the pool.
  7. As a result, both A and B enter a deadlock, waiting indefinitely.

To prevent this issue, we could either set a timeout on allOf.get() (as a way to resolve the deadlock even if the test doesn't execute successfully) or separate the ExecutorService instances.

WDYT?


I’m also curious whether this issue will be included in version 5.12M1 or 5.13.

@YongGoose YongGoose requested a review from marcphilipp January 18, 2025 05:44
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.

Implement Parallel Method Execution in JUnit-Vintage engine
2 participants