-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
JUnit parallel executor running too many tests with a fixed policy. #3108
Comments
Some additional thoughts: Yet another solution could be to use a ForkJoinPool for scheduling of the tests and a standard executor for the execution of the tests. |
There is also a short snippet reproducing this behavior here. |
To add to this, it is not currently possible to have parallel tests without being tricked by JUnit current implementation. |
@OPeyrusse Do the features released as part of 5.9.2 help in your case? |
Hello @marcphilipp , nope it will not help. The feature advertised was already one we looked at, when it was at the stage of PR. I can give you an analogy with the following story: I hope this helps understand the logic triggering the issue 😃 |
Nice analogy. I wish the car factory wouldn't leak implementation details... 😉
I think we can consider that. Would you like to give it a shot? |
Sure, that would be interesting. If you have some guidance on how to write it - and mostly how to write the associated tests - I am ready to try it. |
@OPeyrusse In Generally, I think we should make this new behavior opt-in via a configuration parameter since it introduces additional overhead. |
Thanks for the tips. I will look at it in the coming days. |
Just a quick update: I have not abandoned nor forgotten this issue. It is simply that I am currently taking care of my children during the holiday season here. I already have ideas on how to proceed and will start on this next week 😄 |
If I may ask, do you think it would be possible to allow custom |
@asardaes That's an interesting idea. Could you please raise a new issue for it? |
@marcphilipp since you mentioned @OPeyrusse's implementation should be opt-in, I think it's worth discussing in the context of this issue, killing 2 birds with 1 stone. I gave it some more thought, let me explain what I have in mind. A new configuration parameter could be added, say To instantiate I'm not so sure about the instantiation/configuration convention. Given that both included executor services are marked as stable, I imagine they need to keep existing constructors, but adding a new one and having a default And for completeness, the logic in a test engine would have to be roughly:
|
@asardaes do you have a special use-case in mind, driving your question? I am asking because my idea for this issue was to have two different ways of running the tests: one inlining the execution withing the ForkJoinPool, and one submitting a task to an external Executor and waiting for the result. |
@OPeyrusse no, I don't think that would fit what I had in mind, but if your implementation doesn't need a completely new Personally I'm just experimenting with kotlin coroutines, so I don't need a custom test engine, I just want a custom executor. |
@asardaes If you say so. Not being a core developer of JUnit, I cannot decide what to do. I will let @marcphilipp decide what to do and will start going forward with my plan, that is not creating a new executor. |
Mm I wouldn't mind submitting a PR myself, but I also need some guidance from the maintainers with the questions I mentioned above. I suppose that could be discussed in the PR? |
@shans96 |
Please, write an example how I can set your sollution |
You just quoted the example? |
|
Wherever you need it. |
I use groovyVersion = '3.0.19', spockVersion = '2.4-M1-groovy-3.0', gebVersion = '6.0', seleniumVersion = '4.17.0'. In SpockConfig.groovy : |
Can you show how you Override start() method? |
class FixJunit3108Extension implements IGlobalExtension {
private static ExecutorService threadPool
private final RunnerConfiguration runnerConfiguration
FixJunit3108Extension(RunnerConfiguration runnerConfiguration) {
this.runnerConfiguration = runnerConfiguration
}
@Override
void start() {
threadPool = Executors.newFixedThreadPool(runnerConfiguration.parallel.parallelExecutionConfiguration.parallelism)
}
@Override
void visitSpec(SpecInfo spec) {
// work-around for https://github.com/junit-team/junit5/issues/3108
spec.allFeatures*.addIterationInterceptor {
threadPool.submit(it::proceed).get()
}
}
@Override
void stop() {
threadPool?.shutdown()
}
} |
Hi, could you tell me where I can get this interface 'IGlobalExtension'? Is it in junit 5 anther issue branch? |
This is for Spock. I described above how you probably can do the same for Jupiter. |
Thanks. I use your solution, but in Ui tests with Geb and Spock I have problem, that every feature (test in Spec)create new thread. How I can rewrite this method to create a new thread only for Spec ? @Override
void visitSpec(SpecInfo spec) {
spec.allFeatures*.addIterationInterceptor {
threadPool.submit(it::proceed).get()
}
} |
It does not create a new thread for every features. |
You are even more deviating from the topic here. |
Hi, I tried the description your wrote, but it didn't work. I would greatly appreciate it if you had a validated solution.. |
I provided a full-blown example above, what more do you need? |
I mean for Junit, not for Spock. I tried your description like this but it didn't work. What I need is a solution that has been validated. |
Works perfectly fine for me, except that in Jupiter |
JUnit only parallel executor service relies on a ForkJoinPool. Unfortunately, this does not play well with code also using ForkJoinPools.
The observed issue is that, when activating parallel tests, JUnit uses
ForkJoinPoolHierarchicalTestExecutorService
. However, our tests are also usingForkJoinPool
s andForkJoinTask
s. The orchestration of the test awaits for the completion of those tasks before moving on in the tests.But the issue is that
ForkJoinTask
andForkJoinWorkerThread
are capable of detecting the use of the FJP framework (here for example) and react to it. As JUnit tasks and the project tasks are in different ForkJoinPools, they cannot help each other. This only results in more tests being started by already running and incomplete tests.This can be an issue when tests are resource sensitive. For example, it may not be possible to open too many connections to a Database.
Tough not explicitly illustrated in this project, we also faced
StackOverflowError
because of recursive test executions in a single worker. In the following logs, produced by the reproducing project, we can see that two workers are recursively starting tests before finishing any:In actual code, given the location of the point of respawn, this can generate very large stacks.
An alternative implementation of the executor service, as shown in this PR, using a standard Thread Executor, would not show similar issues, at the expense of not ideally orchestrating multiple executions.
Let's note that this is a very sneaky issue. Even in this project, we can see that the call to
ForkJoinTask#get
is not visible in the JUnit method. And it can be as deep as we want in the stack, even hidden to some users as it is happening in a used framework or tool. It may not be always possible for a user to detect that pattern.Steps to reproduce
See this project https://github.com/OPeyrusse/junitforkjoinpool
After building the project, run the test class
TestCalculator
(or look at the README if changed since opening this issue).Context
Deliverables
The text was updated successfully, but these errors were encountered: