test: Parallelize package tests in high-cost packages. #7126
+1,103
−63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philipaconrad
changed the title
|
Looks like there's a data race occurring under |
philipaconrad
force-pushed
the
philip/parallel-tests
branch
from
15cabfd to
c2e0bcd
Compare
|
ℹ️ Found the culprits! In both cases, the offending tests with race conditions were cases where the tests were modifying package variables of some sort. The two sets of cases were:
|
philipaconrad
force-pushed
the
philip/parallel-tests
branch
3 times, most recently
from
dd0f3ac to
673cefc
Compare
|
ℹ️ Looks like I've got a few more intermittent failures in the EDIT: I think we're stable once more. There was a timeout value that I'd lowered, and the slow GH Actions runner was causing the test to occasionally time out unexpectedly. |
ashutosh-narkar
approved these changes
Nov 8, 2024
There was a problem hiding this comment.
Thanks @philipaconrad! The comments about why tests are excluded from parallel execution are helpful. The tests are green so this LGTM.
This commit adds `t.Parallel()` calls to the beginning of many tests across several Go packages in OPA. The slowest packages (taking ~10s or more) have been instrumented where possible as a proof-of-concept. On a machine with many cores, the tests now will complete as fast as the slowest test per package, instead of the sum of all the tests in a particular package. Background: Go runs tests across different packages in parallel, but will run all tests *within* a package sequentially. By adding `t.Parallel()` to tests that support it, we are telling the test-runner that it is safe to run that test in parallel with any other tests. Signed-off-by: Philip Conrad <[email protected]>
This commit fixes a data race that could occur in the `server` package tests, because 3x tests were modifying package variables under `internal/version`. These tests now run sequentially, and are not included in the parallel test set. Signed-off-by: Philip Conrad <[email protected]>
Two tests in this package modified a package variable directly, and as such cannot be safely run in parallel with each other or any other tests in the package. Signed-off-by: Philip Conrad <[email protected]>
This commit wraps up a large batch of fairly mechanical refactorings to add t.Parallel() annotations to almost every test under `topdown`. The tests that could not be safely parallelized now have explicit warning comments on them describing why they are not safe to run in parallel. Signed-off-by: Philip Conrad <[email protected]>
This commit bundles up test parallelization changes for the `storage/disk` package, dramatically reducing its execution time. Signed-off-by: Philip Conrad <[email protected]>
Signed-off-by: Philip Conrad <[email protected]>
This commit includes a bundle of t.Parallel refactoring changes for the `rego` package, including a timer-related bugfix, and a slight change on a cancellation test to reduce its overall cost during test runs (the logic is preserved, but the mandatory timeouts are lower now). Signed-off-by: Philip Conrad <[email protected]>
Signed-off-by: Philip Conrad <[email protected]>
philipaconrad
force-pushed
the
philip/parallel-tests
branch
from
1ce32da to
9a3ff47
Compare
|
Rebasing through the Github UI, and then will merge when green. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
This commit adds
t.Parallel()calls to the beginning of many tests across several Go packages in OPA. Most of the slowest packages (those taking ~10s or more) have been instrumented where possible as a proof-of-concept. On a machine with many cores, the tests now will complete as fast as the slowest test per package, instead of the sum of all the tests in a particular package.Motivation (Why these changes?)
make testlocally, especially on multicore systemsserver,topdown/*,plugins/*,download, etc.Background Notes on
t.ParallelGo runs tests across different packages in parallel, but will run all tests within a package sequentially. By adding
t.Parallel()to tests that support it, we are telling the test-runner that it is safe to run that test in parallel with any other tests.Adding
t.Parallelat the top of a test marks that test as safe to run in parallel with other tests, but if it has sub-tests, they are run sequentially by default. In some places, I've added thet.Parallelannotation to sub-tests as well, which allows table-driven tests to benefit from available parallelism as well.TODOs
Some remaining targets for parallelizing:
topdown/:: Original run time: ~1 minute. ➡️ New run time: ~30s on a multicore machine.tester/:: Run time: ~1 minute.storage/disk/:: Original run time: ~25-30s. ➡️ New run time: ~5-7s.rego/takes about 10 seconds.