Configure tests to run concurrently #1913
Conversation
Generate changelog in
|
| @@ -138,9 +138,7 @@ private static void enableJunit5ForTestTask(Test task) { | |||
|
|
|||
| // https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parallel-execution | |||
| task.systemProperty("junit.jupiter.execution.parallel.enabled", "true"); | |||
|
|
|||
| // Computes the desired parallelism based on the number of available processors/cores | |||
| task.systemProperty("junit.jupiter.execution.parallel.config.strategy", "dynamic"); | |||
There was a problem hiding this comment.
This is the default value so there's no need to configure it explicitly.
https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parallel-execution-config
There was a problem hiding this comment.
I'm not sure we want to run junit5 tests in parallel by default, of the subset that we've manually opted into, I've had to revert several because they were not threadsafe. I would bet that flake rates would jump substantially, though I'd be happy to be proven wrong!
There was a problem hiding this comment.
I would bet that flake rates would jump substantially, though I'd be happy to be proven wrong!
I don't disagree, but that probably is an indication that your tests are poorly written. Seems valuable to surface that. It's also fairly easy to make a temporary fix - just add @Execution(SAME_THREAD).
What about running tests classes in parallel but individual methods sequentially?
junit.jupiter.execution.parallel.mode.default = same_thread
junit.jupiter.execution.parallel.mode.classes.default = concurrent
There was a problem hiding this comment.
I don't disagree, but that probably is an indication that your tests are poorly written. Seems valuable to surface that.
I wouldn't be comfortable making such a blanket statement, but I agree that in the general case it's accurate. Working under the assumption that well-written tests all pass when run in parallel, the flake rate of those which are poorly designed spans a broad range. When they fail it isn't obvious why, or that it's related to concurrency (especially when mockito is involved), so I'm not sure how we'd surface precisely what needs to change.
Perhaps we could start with some experimentation without rolling this out everywhere by default?
There was a problem hiding this comment.
I experimented with our large internal authentication repo.
Everything in parallel:
https://c.p.b/workflow-run/29d6a91c-b52b-429b-9b55-fd62224faa8e
- No unit tests failures.
- Quite a few integration tests failures. Mostly because we modify the config of the in-process server. This is expected and easy to fix - I wouldn't expect many repos to have this problem since few have integration tests with an in-process server.
- Small number of ETE test failures. At a glance, most of these appear to be related to our selenium tests and our more custom ETE test setup.
Just classes in parallel:
https://c.p.b/workflow-run/6e675a44-aaaf-40d0-9675-4aacd801dded
- No unit tests failures.
- 1 integration test failure. Something fairly specific to our setup.
- 1 ETE test failure. Also likely related to our custom setup.
There was a problem hiding this comment.
I also experimented with out large internal filesystem repo, which has a more standard test setup, and got no test failures.
https://c.p.b/workflow-run/e1ccbab0-0042-41dc-893a-4d7e54197ed5
There was a problem hiding this comment.
In both cases, the unit test speedups were fairly minimal - since our unit tests are fairly quick to begin with.
But we saw considerable speedup in ETE tests, which are typically one of the slowest steps in our CI builds.
There was a problem hiding this comment.
Chiming in that we intentionally did not make tests run in parallel by default as people's repos have tests which in general do not allow this, as they depend on state shared between tests, and our big fear was that people's tests would start flaking and they would not know why (as concurrency issues are hard to debug). People can opt in manually per test using @Execution(ExecutionMode.CONCURRENT) which also makes it obvious the tests are running in parallel.
There was a problem hiding this comment.
This is a fair point. The gains here are fairly minimal so I'm just going to close this.
Before this PR
Tests are run sequentially be default.
After this PR
Tests are run concurrently by default.
It appears that the intention in #666 was to run tests concurrently by default. However, that PR was not sufficient - you must also set
junit.jupiter.execution.parallel.mode.default.https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parallel-execution