leakcheck: Fix flaky test TestCheck #8309
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8309 +/- ##
==========================================
+ Coverage 82.15% 82.19% +0.04%
==========================================
Files 419 419
Lines 41904 41990 +86
==========================================
+ Hits 34426 34515 +89
+ Misses 6013 6008 -5
- Partials 1465 1467 +2 🚀 New features to boost your workflow:
|
internal/leakcheck/leakcheck_test.go
Outdated
| } | ||
| e := &testLogger{} | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
| CheckGoroutines(ctx, e) | ||
| if e.errorCount != leakCount { | ||
| if CheckGoroutines(ctx, e); e.errorCount == 0 { |
There was a problem hiding this comment.
That works, or you can do
| if CheckGoroutines(ctx, e); e.errorCount == 0 { | |
| if CheckGoroutines(ctx, e); ctx.Err() == nil { |
Or even:
| if CheckGoroutines(ctx, e); e.errorCount == 0 { | |
| if CheckGoroutines(ctx, e); e.errorCount < 3 { |
There was a problem hiding this comment.
This check expects that the goroutines spawned by time.Sleep() should still be alive.
I have not put e.errorcount == 3, because ctx can cause its goroutine live longer and it will cause a errorcount of 4 (Because of this the test was flaky)
There was a problem hiding this comment.
I have not put e.errorcount == 3, because ctx can cause its goroutine live longer and it will cause a errorcount of 4
Right, which is why I suggested changing to failing if it's less than three. We should have at least three. Or we can just confirm the check failed by checking ctx.Err() and then not have to worry about all the complexity to track errorCount at all. Either way is fine.
There was a problem hiding this comment.
Right, which is why I suggested changing to failing if it's less than three. We should have at least three. Or we can just confirm the check failed by checking
ctx.Err()and then not have to worry about all the complexity to trackerrorCountat all. Either way is fine.
Right, make sense. Both works fine. I'm going with e.errorcount < 3for now.
internal/leakcheck/leakcheck_test.go
Outdated
|
|
||
| // "sync" | ||
|
|
internal/leakcheck/leakcheck_test.go
Outdated
| } | ||
| e := &testLogger{} | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
| CheckGoroutines(ctx, e) | ||
| if e.errorCount != leakCount { | ||
| if CheckGoroutines(ctx, e); e.errorCount == 0 { |
There was a problem hiding this comment.
I have not put e.errorcount == 3, because ctx can cause its goroutine live longer and it will cause a errorcount of 4
Right, which is why I suggested changing to failing if it's less than three. We should have at least three. Or we can just confirm the check failed by checking ctx.Err() and then not have to worry about all the complexity to track errorCount at all. Either way is fine.
internal/leakcheck/leakcheck_test.go
Outdated
| @@ -47,16 +47,14 @@ func TestCheck(t *testing.T) { | |||
| for i := 0; i < leakCount; i++ { | |||
| go func() { time.Sleep(2 * time.Second) }() | |||
There was a problem hiding this comment.
Can you change this to not use Sleep? We should always avoid sleeps for correctness.
Instead do:
ch := make(chan struct{})
for i := 0; i < leakCount; i++ {
go func() { <-ch }()
}
// test interestingGoroutines() and CheckGoroutines()
close(ch) // make goroutines exit
internal/leakcheck/leakcheck_test.go
Outdated
| if e.errorCount != leakCount { | ||
| t.Errorf("CheckGoroutines found %v leaks, want %v leaks", e.errorCount, leakCount) | ||
| if CheckGoroutines(ctx, e); e.errorCount < leakCount { | ||
| t.Errorf("CheckGoroutines() = %v, want count %v", e.errorCount, leakCount) |
There was a problem hiding this comment.
"want count < %d" to more accurately describe the assertion.
There was a problem hiding this comment.
Checking for e.errorcount >= leakcount, not less than leakcount.
Want e.errorcount >= leakcount because ctx can cause its spawned goroutine to leak, which causes flakiness in the test.
arjan-bal
changed the title
Fixes #8276
leakcheck.TestCheck and leakcheck.TestCheckRegisterIgnore is flaky due to a gouroutine leak caused by context.WithTimeout().
Flakiness was filed in #8276.
RELEASE NOTES: N/A