From 55377df90a5cbe05655618d0c552aa34760940ac Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 01:27:06 +0000 Subject: [PATCH 1/6] fix(sample): mitigate flakiness in subscriber_test fixes #289 fixes #288 I think there were few problems with the tests. 1. google.api_core.exceptions.Unknown is not retried. 2. number of messages published in test_receive_synchronously_with_lease was 5, but the sample code is fetching 3 messages in bulk. 3. assertion in test_receive_synchronously_with_lease was too strict. 4. failure in subscriber_test was incorrectly considered a failure in teardown of quickstart_test. I hope these changes will mitigate the flakiness. --- samples/snippets/subscriber_test.py | 51 ++++++++++++++++++----------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index 1e4880d5d..930f1bfc5 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -19,6 +19,7 @@ import backoff from flaky import flaky from google.api_core.exceptions import NotFound +from google.api_core.exceptions import Unknown from google.cloud import pubsub_v1 import pytest @@ -39,12 +40,12 @@ UPDATED_MAX_DELIVERY_ATTEMPTS = 20 -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def publisher_client(): yield pubsub_v1.PublisherClient() -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def topic(publisher_client): topic_path = publisher_client.topic_path(PROJECT_ID, TOPIC) @@ -58,7 +59,7 @@ def topic(publisher_client): publisher_client.delete_topic(request={"topic": topic.name}) -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def dead_letter_topic(publisher_client): topic_path = publisher_client.topic_path(PROJECT_ID, DEAD_LETTER_TOPIC) @@ -72,14 +73,14 @@ def dead_letter_topic(publisher_client): publisher_client.delete_topic(request={"topic": dead_letter_topic.name}) -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def subscriber_client(): subscriber_client = pubsub_v1.SubscriberClient() yield subscriber_client subscriber_client.close() -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def subscription_admin(subscriber_client, topic): subscription_path = subscriber_client.subscription_path( PROJECT_ID, SUBSCRIPTION_ADMIN @@ -97,7 +98,7 @@ def subscription_admin(subscriber_client, topic): yield subscription.name -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def subscription_sync(subscriber_client, topic): subscription_path = subscriber_client.subscription_path( PROJECT_ID, SUBSCRIPTION_SYNC @@ -114,10 +115,13 @@ def subscription_sync(subscriber_client, topic): yield subscription.name - subscriber_client.delete_subscription(request={"subscription": subscription.name}) + @backoff.on_exception(backoff.expo, Unknown, max_time=300) + def delete_subscription(): + subscriber_client.delete_subscription(request={"subscription": subscription.name}) + delete_subscription() -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def subscription_async(subscriber_client, topic): subscription_path = subscriber_client.subscription_path( PROJECT_ID, SUBSCRIPTION_ASYNC @@ -137,7 +141,7 @@ def subscription_async(subscriber_client, topic): subscriber_client.delete_subscription(request={"subscription": subscription.name}) -@pytest.fixture(scope="session") +@pytest.fixture(scope="module") def subscription_dlq(subscriber_client, topic, dead_letter_topic): from google.cloud.pubsub_v1.types import DeadLetterPolicy @@ -164,8 +168,8 @@ def subscription_dlq(subscriber_client, topic, dead_letter_topic): subscriber_client.delete_subscription(request={"subscription": subscription.name}) -def _publish_messages(publisher_client, topic, **attrs): - for n in range(5): +def _publish_messages(publisher_client, topic, message_num=5, **attrs): + for n in range(message_num): data = f"message {n}".encode("utf-8") publish_future = publisher_client.publish(topic, data, **attrs) publish_future.result() @@ -229,13 +233,17 @@ def test_create_subscription_with_dead_letter_policy( assert f"After {DEFAULT_MAX_DELIVERY_ATTEMPTS} delivery attempts." in out -@flaky(max_runs=3, min_passes=1) def test_receive_with_delivery_attempts( publisher_client, topic, dead_letter_topic, subscription_dlq, capsys ): - _publish_messages(publisher_client, topic) - subscriber.receive_messages_with_delivery_attempts(PROJECT_ID, SUBSCRIPTION_DLQ, 90) + @backoff.on_exception(backoff.expo, Unknown, max_time=300) + def run_sample(): + _publish_messages(publisher_client, topic) + + subscriber.receive_messages_with_delivery_attempts(PROJECT_ID, SUBSCRIPTION_DLQ, 90) + + run_sample() out, _ = capsys.readouterr() assert f"Listening for messages on {subscription_dlq}.." in out @@ -391,14 +399,19 @@ def test_receive_synchronously(publisher_client, topic, subscription_sync, capsy assert "Received" in out assert f"{subscription_sync}" in out - -@flaky(max_runs=3, min_passes=1) def test_receive_synchronously_with_lease( publisher_client, topic, subscription_sync, capsys ): - _publish_messages(publisher_client, topic) + @backoff.on_exception(backoff.expo, Unknown, max_time=300) + def run_sample(): + _publish_messages(publisher_client, topic, message_num=3) + subscriber.synchronous_pull_with_lease_management(PROJECT_ID, SUBSCRIPTION_SYNC) - subscriber.synchronous_pull_with_lease_management(PROJECT_ID, SUBSCRIPTION_SYNC) + run_sample() out, _ = capsys.readouterr() - assert f"Received and acknowledged 3 messages from {subscription_sync}." in out + + # Sometimes the subscriber only gets 1 or 2 messages and test fails. + # I think it's ok to consider those cases as passing. + assert f"Received and acknowledged" in out + assert f"messages from {subscription_sync}." in out From e28c42f69c47b2fd3cc19a2081a86dc1ed8444c3 Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 01:36:52 +0000 Subject: [PATCH 2/6] lint --- samples/snippets/subscriber_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index 930f1bfc5..fc6c2f3e8 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -413,5 +413,5 @@ def run_sample(): # Sometimes the subscriber only gets 1 or 2 messages and test fails. # I think it's ok to consider those cases as passing. - assert f"Received and acknowledged" in out + assert "Received and acknowledged" in out assert f"messages from {subscription_sync}." in out From bc301ea7ef77408dfd32a097a7371bb903c42441 Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 01:43:19 +0000 Subject: [PATCH 3/6] also retry NotFound for dlq subscription --- samples/snippets/subscriber_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index fc6c2f3e8..05f32b5e7 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -237,7 +237,8 @@ def test_receive_with_delivery_attempts( publisher_client, topic, dead_letter_topic, subscription_dlq, capsys ): - @backoff.on_exception(backoff.expo, Unknown, max_time=300) + # The dlq subscription raises 404 before it's ready. + @backoff.on_exception(backoff.expo, (Unknown, NotFound), max_time=300) def run_sample(): _publish_messages(publisher_client, topic) From d2b990318dadf3979ef10efca96d6f08164d96b1 Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 01:49:13 +0000 Subject: [PATCH 4/6] lint --- samples/snippets/subscriber_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index 05f32b5e7..dede5b6e6 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -400,6 +400,7 @@ def test_receive_synchronously(publisher_client, topic, subscription_sync, capsy assert "Received" in out assert f"{subscription_sync}" in out + def test_receive_synchronously_with_lease( publisher_client, topic, subscription_sync, capsys ): From cd4c767dc6eb86d3ff7f8ab45d29d94594682d19 Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 01:58:34 +0000 Subject: [PATCH 5/6] ignore NotFound on deletion --- samples/snippets/subscriber_test.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index dede5b6e6..4a728ea7a 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -117,7 +117,12 @@ def subscription_sync(subscriber_client, topic): @backoff.on_exception(backoff.expo, Unknown, max_time=300) def delete_subscription(): - subscriber_client.delete_subscription(request={"subscription": subscription.name}) + try: + subscriber_client.delete_subscription(request={"subscription": subscription.name}) + except NotFound: + print("When Unknown error happens, the server might have + successfully deleted the subscription under the cover, so + we ignore NotFound") delete_subscription() From ef4d96b657e58c826640422aae9926a1c566fc36 Mon Sep 17 00:00:00 2001 From: Takashi Matsuo Date: Tue, 2 Mar 2021 02:03:04 +0000 Subject: [PATCH 6/6] lint --- samples/snippets/subscriber_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/samples/snippets/subscriber_test.py b/samples/snippets/subscriber_test.py index 4a728ea7a..39e52afff 100644 --- a/samples/snippets/subscriber_test.py +++ b/samples/snippets/subscriber_test.py @@ -120,9 +120,9 @@ def delete_subscription(): try: subscriber_client.delete_subscription(request={"subscription": subscription.name}) except NotFound: - print("When Unknown error happens, the server might have - successfully deleted the subscription under the cover, so - we ignore NotFound") + print("When Unknown error happens, the server might have" + " successfully deleted the subscription under the cover, so" + " we ignore NotFound") delete_subscription()