-
Notifications
You must be signed in to change notification settings - Fork 6.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
[kms] fix flaky test #3268
[kms] fix flaky test #3268
Conversation
kms/api-client/snippets_test.py
Outdated
self.symId) | ||
except Aborted: | ||
# aborted by backend. Try again | ||
try_number += 1 |
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.
Maybe you want to use @eventually_consistent.call
?
An example commit:
c6254e4
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.
+1 to using eventually consistent if you can
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 didn't know about @eventually_consistent
, that's much cleaner!
kms/api-client/snippets_test.py
Outdated
self.symId) | ||
except Aborted: | ||
# aborted by backend. Try again | ||
try_number += 1 |
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.
+1 to using eventually consistent if you can
if b.role == self.role and self.member in b.members: | ||
found = True | ||
assert found | ||
eventually_consistent.call(check_policy, |
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.
Unfortunately the decorator form doesn't work with the current 0.0.15 release.
See: GoogleCloudPlatform/python-repo-tools#25
I'm trying to make a new release of the above module.
I'm fine with the current form :)
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, just some minor nits.
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.
Please see my comment about exceptions
tuple.
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.
Thanks! Ship it!
@dansanche Sorry for "hot" repo issue ;) I needed to rebase multiple times, but it's good to see the tests are more stable :) |
fixes #2970
I added exponential backoff to the policy test, as recommended by the error message