Skip to content
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

Merged
merged 10 commits into from
Nov 7, 2022
Merged

Conversation

telpirion
Copy link
Contributor

Fixes #6939
Fixes #6938

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: datastore Issues related to the Datastore API. labels Nov 2, 2022
@telpirion telpirion marked this pull request as ready for review November 2, 2022 22:11
@telpirion telpirion requested review from enocom and a team as code owners November 2, 2022 22:11
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Nov 2, 2022
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Nov 4, 2022
// exposed in anyway in the response.
err = client.Get(ctx, k, &got)
if err != nil {
t.Errorf("client.Get: %v", err)
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Nice catch!

}

// Wait a little bit for eventual consistency to catch up.
time.Sleep(time.Duration(10 * math.Pow(10, 9)))
Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Contributor Author

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!

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Nov 7, 2022
@telpirion telpirion requested a review from quartzmo November 7, 2022 20:18
@telpirion telpirion added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2022
@telpirion telpirion enabled auto-merge (squash) November 7, 2022 22:12
@telpirion telpirion added the automerge Merge the pull request once unit tests and other checks pass. label Nov 7, 2022
@telpirion telpirion merged commit 57e8cf9 into main Nov 7, 2022
@telpirion telpirion deleted the datastore-more-fix branch November 7, 2022 22:23
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datastore: TestIntegration_GetMulti failed datastore: TestIntegration_GetWithReadTime failed
3 participants