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

samples(test): flaky sync pull test #434

Merged
merged 2 commits into from
Jun 23, 2021
Merged

samples(test): flaky sync pull test #434

merged 2 commits into from
Jun 23, 2021

Conversation

anguillanneuf
Copy link
Contributor

@anguillanneuf anguillanneuf commented Jun 23, 2021

Fixes #341 🦕

The stream sometimes gets removed (UNKNOWN errors) before the subscriber can successfully pull messages. Pausing to give the subscriber some time to reconnect since this subscription (subscription_sync) is used in two different synchronous pull samples:

  • test_receive_synchronously_with_lease
  • test_receive_synchronously

If this doesn't fix the flaky test, I will consider using two different subscriptions for the two different sync pull samples tests.

The delete schema sample test failure is unrelated. It's known and is flaky due to backend issues.

@anguillanneuf anguillanneuf requested a review from a team as a code owner June 23, 2021 15:53
@anguillanneuf anguillanneuf requested review from dandhlee and removed request for a team June 23, 2021 15:53
@product-auto-label product-auto-label bot added api: pubsub Issues related to the googleapis/python-pubsub API. samples Issues that are directly related to samples. labels Jun 23, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 23, 2021
@@ -475,7 +476,8 @@ def test_receive_synchronously_with_lease(
):
@backoff.on_exception(backoff.expo, Unknown, max_time=300)
def run_sample():
_publish_messages(publisher_client, topic, message_num=3)
_publish_messages(publisher_client, topic, message_num=10)
time.sleep(10)
Copy link
Contributor

@plamut plamut Jun 23, 2021

Choose a reason for hiding this comment

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

Maybe add a comment why this is needed?

Waiting for a fixed amount of time is of course far from ideal, but if it resolves or at least significantly mitigates the flakiness, it's fine to have it, as flakiness has really been getting into the way recently.

Edit: Whoa, samples tests passed on the first attempt. Could be a coincidence, of course, but it's a good start nevertheless. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment. Please take a look now to see if it's sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks!

@anguillanneuf anguillanneuf requested a review from plamut June 23, 2021 16:33
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM and the samples tests seem to be passing without retries.

@plamut plamut merged commit 86f1f68 into master Jun 23, 2021
@plamut plamut deleted the samples-sync-pull branch June 23, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

samples.snippets.subscriber_test: test_receive_synchronously_with_lease failed
2 participants