From a63eae48cc39e61b6c49e9ec2d17f4052c0af0e3 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Sat, 15 Dec 2018 11:05:43 -0800 Subject: [PATCH] test_push_notifications: Fix leak that can leak to test flakes. While reviewing #11012, I discovered a nondeterministic result for test_signup, which I tracked down to specifically this triple of tests failing when run in this order: test-backend GCMSuccessTest \ zerver.tests.test_push_notifications.TestAPNs.test_get_apns_client \ zerver.tests.test_signup.LoginTest.test_register with a query count mismatch like this: expected length: 73 actual length: 79 Comparing the list of queries, it's clear that test_register was seeing `push_notifications_enabled()` returning True in this test order. It's not clear why GCMSuccessTest was required here (it was!), but further debugging determined the problem was that `test_get_apns_client` left the _apns_client initialization system in a state where get_apns_client would return a non-None value, resulting in push_notifications_enabled() returning True for future tests. The immediate fix is to just reset the `_apns_client` and `_apns_client_initializedstate` state properly after the test runs; but arguably we should do a larger refactor to make this less fragile. --- zerver/tests/test_push_notifications.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 255f57c36c..3cb30c5514 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -707,12 +707,20 @@ class TestAPNs(PushNotificationTest): self.user_profile.id, devices, payload_data) def test_get_apns_client(self) -> None: + """This test is pretty hacky, and needs to carefully reset the state + it modifies in order to avoid leaking state that can lead to + nondeterministic results for other tests. + """ import zerver.lib.push_notifications zerver.lib.push_notifications._apns_client_initialized = False with self.settings(APNS_CERT_FILE='/foo.pem'), \ mock.patch('apns2.client.APNsClient') as mock_client: client = get_apns_client() self.assertEqual(mock_client.return_value, client) + # Reset the values set by `get_apns_client` so that we don't + # leak changes to the rest of the world. + zerver.lib.push_notifications._apns_client_initialized = False + zerver.lib.push_notifications._apns_client = None def test_not_configured(self) -> None: with mock.patch('zerver.lib.push_notifications.get_apns_client') as mock_get, \