-
Notifications
You must be signed in to change notification settings - Fork 92
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
fix: flaky TestOriginalSampleRateIsNotedInMetaField #991
Conversation
0a636b9
to
60b2086
Compare
60b2086
to
995128b
Compare
collect/collect_test.go
Outdated
coll.AddSpan(span) | ||
// Find the Refinery-sampled-and-sent event that had no upstream sampling which | ||
// should be the last event on the transmission queue. | ||
var noUpstreamSampleRateEvent *types.Event | ||
assert.Eventually(t, func() bool { |
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.
I know you didn't change this part of the test in this PR. I'm just wondering if using require
instead of assert
here would be easier to identify failures here. If the assertion for "no-upstream-stampling
has failed, the following check won't succeed either.
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.
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.
Yingrong's comment is appropriate - we should be using require
more frequently, and this is a good place to start. But even without that, I'm a huge fan of deflakifying things. Thanks for the attention!
require makes it more clear that the test failed because the event needed for the later assertion never appeared.
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.
Thank you for fixing the flaky test and all the beautiful comments 💯
Which problem is this PR solving?
Short description of the changes