Testing: Lint for unparallelized tests without explicit reason #4746
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
# Conflicts: # data/transactions/logic/assembler_test.go # ledger/internal/eval_blackbox_test.go
Codecov Report
@@ Coverage Diff @@
## master #4746 +/- ##
==========================================
- Coverage 54.62% 54.57% -0.06%
==========================================
Files 414 414
Lines 53514 53514
==========================================
- Hits 29231 29203 -28
- Misses 21859 21881 +22
- Partials 2424 2430 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
If you want custom control of rules just for a single linter, you could theoretically set up a separate .golangci-paralleltest.yml configuration and add it to make lint as a second pass, and to reviewdog as an additional pass.. |
cce
approved these changes
Nov 3, 2022
jannotti
reviewed
Nov 3, 2022
| t.Run(fmt.Sprintf("unified=%t", unified), func(t *testing.T) { | ||
| // t.Parallel() NO! unified variable is actually shared | ||
|
|
||
| t.Run(fmt.Sprintf("unified=%t", unified), func(t *testing.T) { //nolint:paralleltest // NO t.Parallel(). unified variable is actually shared |
There was a problem hiding this comment.
Does this linter actually complain about every t.Run too? Or just top-level func Test*?
There was a problem hiding this comment.
Also t.Run(). For a full list of the errors it catches: https://github.com/kunwardeep/paralleltest#examples
jannotti
approved these changes
Nov 3, 2022
michaeldiamant
approved these changes
Nov 4, 2022
This was referenced Nov 21, 2022
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.
Summary
This PR starts the process of improving unit test parallelization by enabling the paralleltest linter within golang ci. Fixing missing
t.Parallel's will require careful consideration and so, in this initial PR, packages currently failing the paralleltest linter are excluded.Future PRs will incrementally add the
t.Parallel()flag to tests in existing packages and disable that package's exclusion within.golangci.yml. As a result, tests added to that package in the future will fail the linter if the parallel signal is not present and a reason is not given as to why it is missing. For more information regarding the process of finding and fixing incorrect tests within a particular package, refer to the additional information below.Additional Information
A large part of the work related to this PR involved brainstorming a path toward fixing unparallelized tests within existing packages. I've listed some additional information and answers to common questions below.
I want to start fixing missing instances of `t.Parallel` in a specific package. How can I find all the parlleltest errors associated with that package?
1. Adjust the
.golangci.ymlconfiguration file to not exclude the particular package.For example, to reveal errors within the
ledgerpackage, removeledgerfrom the exclusion filter:path: (data|ledger|logging).*_test.gobecomespath: (data|logging).*_test.go2. Temporarily disable the
new-from-revconfiguration within.golangci.ymlby commenting it out.A majority of the tests without
t.Parallel()were committed before the set revision so we must disable this configuration to let through every instance of the error within the particular package.3. Lint and filter by
paralleltesterrors specificallyWe must filter specifically for paralleltest errors here because many more errors will creep through once we ignore the
new-from-revargument.How can I mark tests to show that they shouldn't be run in parallel?
If you find yourself in a situation where using
t.Parallel()is impossible, put a//nolint:paralleltestcomment on the line where the error is caught and be sure to include an explanation for why the test could not be parallelized.For an example of how this looks in practice, please refer to examples in this commit.
Will new tests in excluded packages be caught by the linter?
Unfortunately, new tests without the parallel flag will not be caught by the linter as I could not find any way within the golangci lint configuration to demarcate that specific linters should be run against particular revisions. In other words, the
new-from-revflag applies to all linters and cannot be set individually.This is why future PRs that fix existing errors within packages will be necessary to make enabling this linter worthwhile. Once existing tests missing the parallel flag have been fixed, the exclusion rule for that package can be removed and new tests after that point will be caught.
Test Plan
These are primarily linter changes that do not benefit from tests.