-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(canary): Fix Flaky Tests #889
Conversation
…void races around the entry order and test verifications
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.
LGTM
I can still reproduce some flakes if I simulate a slow machine, unfortunately.
I got this:
|
@rfratto did you try it with these changes? I haven't merged it yet. |
@sandlis yeah, I checked out the PR locally |
Depending on how much you slowed things down @rfratto you may have just exceeded some of the timings hard coded into the test. Was it always the same test that failed? loki/pkg/canary/comparator/comparator_test.go Lines 176 to 177 in 9563033
Can you increase that wait and see if it fixes the failures? |
It's always the same test that fails; it always passed when I increased maxWait to I tried lower settings (200ms, 300ms) but 500 seems to be the acceptable minimum for my specific machine. |
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.
LGTM
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.
whoops, forgot to hit approve 🙂
…n a really slow machine
the canary comparator tests are flaky because they assert some exact ordering on messages written to the output. I believe because certain validations are done in a goroutine this leads to inconsistent timing on their responses and flakes out the tests. I added a parameter to the comparator to make these lookups synchronous and hopefully fix this issue.