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.

Sign up for GitHub

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

Jump to bottom

Configure tests to run concurrently #1913

Closed
wants to merge 2 commits into from
Closed

Configure tests to run concurrently #1913

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37442ae
Configure tests to run concurrently
pkoenig10 Sep 10, 2021
41284bd
Add generated changelog entries
svc-changelog Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineTesting.java
View file
Open in desktop
Expand Up @@ -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");
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

@pkoenig10 pkoenig10 Sep 10, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@pkoenig10 pkoenig10 Sep 11, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pkoenig10 pkoenig10 Sep 12, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a fair point. The gains here are fairly minimal so I'm just going to close this.

task.systemProperty("junit.jupiter.execution.parallel.mode.default", "concurrent");

// provide some stdout feedback when tests fail when running on CI and locally
task.getTestLogging().getEvents().add(TestLogEvent.FAILED);
Expand Down