New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
resource: Fix flaky test due to missing Done call #25646
resource: Fix flaky test due to missing Done call #25646
Conversation
51ad940
to
4cd6d56
Compare
pkg/k8s/resource/resource_test.go
Outdated
@@ -40,7 +41,7 @@ func TestMain(m *testing.M) { | |||
goleak.VerifyTestMain(m, goleak.Cleanup(cleanup)) | |||
} | |||
|
|||
func testStore(t *testing.T, node *corev1.Node, store resource.Store[*corev1.Node]) { | |||
func testStore(t testing.TB, node *corev1.Node, store resource.Store[*corev1.Node]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the change is probably not required anymore? Conventionally this should be tb
now which would lead to a lot of churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted this hunk.
The workaround in 5fa2ac4 was faulty as it did not call ev.Done on the received event leading to further events not being received and thus eventually causing the context to time out. The problem was reproduced reliably locally by adding short time.Sleep calls to the pushUpdate and the initial listing to trigger the race causing the double updates. To remedy this, add the missing ev.Done call, handle timeout gracefully and make assertions FailNow immediately to not mask problems due to e.g. nil deref. FIxes: cilium#24696 Fixes: 5fa2ac4 ("resource: Work around a rare race in initial sync") Signed-off-by: Jussi Maki <jussi@isovalent.com>
4cd6d56
to
c8e75ad
Compare
Relevant test is the |
The workaround in 5fa2ac4 was faulty as it did not call ev.Done on the received event leading to further events not being received and thus eventually causing the context to time out. The problem was reproduced reliably locally by adding short time.Sleep calls to the pushUpdate and the initial listing to trigger the race causing the double updates.
To remedy this, add the missing ev.Done call, handle timeout gracefully and make assertions FailNow immediately to not mask problems due to e.g. nil deref.
FIxes: #24696
Fixes: 5fa2ac4 ("resource: Work around a rare race in initial sync")