Tests: Fix t.Parallel() errors in netdeploy package #4993
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4993 +/- ##
==========================================
- Coverage 53.44% 53.43% -0.01%
==========================================
Files 431 431
Lines 54364 54364
==========================================
- Hits 29056 29052 -4
- Misses 23053 23059 +6
+ Partials 2255 2253 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -87,7 +88,7 @@ func TestCreateSignedTxBasic(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestCreateSignedTxAssets(t *testing.T) { | |||
| func TestCreateSignedTxAssets(t *testing.T) { //nolint:paralleltest // Not parallel because it modifies config.Consensus | |||
There was a problem hiding this comment.
it seems only to read config.Consensus, but I guess the fear is someone else may modify config.Consensus?
There was a problem hiding this comment.
Is it not mutated on line 108? Assuming it is mutated on line 108, is there supposed to be a cleanup step that's missing?
There was a problem hiding this comment.
but that's on a copy of the params made on line 95, so it doesn't affect the params in the global map
There was a problem hiding this comment.
Ah yes - you're correct. I had forgotten that the values of the map are structs.
There was a problem hiding this comment.
I tried using t.Parallel() on these tests and they all passed. Empirically, I've found that tests which read config.Consensus don't have any issues being parallelized when other tests containing Consensus mutations are not parallelized. I've tried looking for a more concrete answer to this online to no avail. @cce do you know any more about how parallel vs. sequential tests are sandboxed? Perhaps they are run one after another?
There was a problem hiding this comment.
I can confirm that I am able to add t.Parallel() to these tests locally and that the test cases pass even with -count=40 and above.
There was a problem hiding this comment.
Yes go test will ensure that if a test doesn't t.Parallel(), it will not be run at the same time as any other test
See #5046 for info on why these were not caught
There was a problem hiding this comment.
Changes look good! I went ahead and parallelized the two tests that appear to mutate config.Consensus but do not, as @cce pointed out. I additionally parallelized test files in the netdeploy package that were not caught by the golangci regex.
Enables https://github.com/kunwardeep/paralleltest on
netdeployby fixing linter warnings. Incrementally moves the ball towards greater unit test parallelization, which reduces local + CI test durations.I vetted for flakiness by running:
Notes: