-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore(datastore): fix for flaky integration tests #6979
Conversation
datastore/integration_test.go
Outdated
// exposed in anyway in the response. | ||
err = client.Get(ctx, k, &got) | ||
if err != nil { | ||
t.Errorf("client.Get: %v", err) |
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.
Should this be r.Errorf
to ensure the retry works correctly?
Btw, have you observed the retries working?
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.
Done. Nice catch!
datastore/integration_test.go
Outdated
} | ||
|
||
// Wait a little bit for eventual consistency to catch up. | ||
time.Sleep(time.Duration(10 * math.Pow(10, 9))) |
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.
Is it really worth waiting 10s every time? I think this is only worth it if it's guaranteed to take that long, otherwise might as well try right away, or after a shorter wait.
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 tried several different time lengths from 1-10 seconds to ensure consistent test success. 10s was the lowest value that seemed to work every time (locally).
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.
You know what, it looks like the issue was that I didn't hook into the retries correctly :). Nice catch!
Fixes #6939
Fixes #6938