-
-
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. |
@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 |
This begins the process of allowing for parallel test execution. A few issues were noticed that required some changes: * Some tests were taking up a bunch of time and benefitted from parallelized execution of tests within the fixture. Those have been updated to CONCURRENT execution mode * A known issue with JUnit (junit-team/junit5#3108) means that if one of the tests involves a future waiting, that can look to the `ForkJoinPool` like the thread is available for work stealing, so too many tests can end up being executed at once. A new test extension was added that has a semaphore, and that appears to be enough to stop extra tests from being executed * The server was running out of batch GRV transactions, which resulted in tests failing with "batch GRV transactions exhausted". This mainly affected indexing tests, and I was able to resolve this by upping the transaction priority, for better or worse There are still some issues: * A number of tests were hitting deadline exceeded exceptions. It looked like some kind of weird concurrency stuff may be going on, because there were things happening like key space path resolution while creating a record store would include stack traces from closing the test key space path manager, which seems like things were sharing objects that shouldn't have been. This affected both the `:fdb-record-layer-core:test` and `:fdb-lucene:test` tasks * The relational layer tests are not structured to allocate unique key spaces for each test, so they immediatetly hit concurrency problems when run in a parallelized manner
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: