fix(i): For loop foot guns in tests #1900
Conversation
nasdf
added
bug
Codecov ReportPatch and project coverage have no change.
@@ Coverage Diff @@
## develop #1900 +/- ##
========================================
Coverage 70.26% 70.26%
========================================
Files 232 232
Lines 24192 24192
========================================
Hits 16998 16998
Misses 6029 6029
Partials 1165 1165
Flags with carried forward coverage won't be shown. Click here to find out more. see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -161,7 +161,8 @@ func TestIsComplex(t *testing.T) { | |||
| } | |||
|
|
|||
| mapping := getDocMapping() | |||
| for _, test := range tests { | |||
| for i := range tests { | |||
| test := tests[i] | |||
There was a problem hiding this comment.
question: How sure on these are you? The original test variable is never really accessed concurrently - t.Run will block until completion of the func unless you explicitly mark it as Parralel.
There was a problem hiding this comment.
Good point. I had forgotten that it blocked which mean that the variable wouldn't change value until the Run has completed anyways.
There was a problem hiding this comment.
I misread the go docs for t.Run. The change detector does use t.Parallel, but I'll undo these changes.
| @@ -291,7 +291,8 @@ func TestNormalizeConditions(t *testing.T) { | |||
| } | |||
|
|
|||
| mapping := getDocMapping() | |||
| for _, tt := range tests { | |||
| for i := range tests { | |||
| tt := tests[i] | |||
There was a problem hiding this comment.
question: Building upon my other comment, I did spend some time looking at this, and if you trim this test down to just the last usecase, and run it a bunch of times logging the output you can see that the output does vary. I am pretty convinced that there is an issue with the production code, and I think it is flaky due to the randomised map iteration order causing the test to only fail (correctly) every now and then.
Relevant issue(s)
Resolves #1879
Description
For loop range variable assignment can cause race conditions when used with go routines such as
test.Run().Read more about it here https://go.dev/blog/loopvar-preview
Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: