fix: tests that utilize threading from failing with pypy (#383)

* fix pypy error with thread safe call counter

* fix polling thread running after test completion

* fix thread interfering with fetch_datafile

Author: andrewleap-optimizely

tests/test_config_manager.py

@@ -1,4 +1,4 @@
-# Copyright 2019-2021, Optimizely
+# Copyright 2019-2022, Optimizely
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
@@ -218,6 +218,38 @@ def test_get_config_blocks(self):
         self.assertEqual(1, round(end_time - start_time))
 
 
+class MockPollingConfigManager(config_manager.PollingConfigManager):
+    ''' Wrapper class to allow manual call of fetch_datafile in the polling thread by
+    overriding the _run method.'''
+    def __init__(self, *args, **kwargs):
+        self.run = False
+        self.stop = False
+        super().__init__(*args, **kwargs)
+
+    def _run(self):
+        '''Parent thread can use self.run to start fetch_datafile in polling thread and wait for it to complete.'''
+        while self.is_running and not self.stop:
+            if self.run:
+                self.fetch_datafile()
+                self.run = False
+
+
+class MockAuthDatafilePollingConfigManager(config_manager.AuthDatafilePollingConfigManager):
+    ''' Wrapper class to allow manual call of fetch_datafile in the polling thread by
+    overriding the _run method.'''
+    def __init__(self, *args, **kwargs):
+        self.run = False
+        self.stop = False
+        super().__init__(*args, **kwargs)
+
+    def _run(self):
+        '''Parent thread can use self.run to start fetch_datafile and wait for it to complete.'''
+        while self.is_running and not self.stop:
+            if self.run:
+                self.fetch_datafile()
+                self.run = False
+
+
 @mock.patch('requests.get')
 class PollingConfigManagerTest(base.BaseTest):
     def test_init__no_sdk_key_no_url__fails(self, _):
@@ -294,9 +326,13 @@ def test_get_datafile_url__sdk_key_and_url_and_template_provided(self, _):
 
     def test_set_update_interval(self, _):
         """ Test set_update_interval with different inputs. """
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
+
+        # prevent polling thread from starting in PollingConfigManager.__init__
+        # otherwise it can outlive this test and get out of sync with pytest
+        with mock.patch('threading.Thread.start') as mock_thread:
             project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')
 
+        mock_thread.assert_called_once()
         # Assert that if invalid update_interval is set, then exception is raised.
         with self.assertRaisesRegex(
             optimizely_exceptions.InvalidInputException, 'Invalid update_interval "invalid interval" provided.',
@@ -321,9 +357,13 @@ def test_set_update_interval(self, _):
 
     def test_set_blocking_timeout(self, _):
         """ Test set_blocking_timeout with different inputs. """
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
+
+        # prevent polling thread from starting in PollingConfigManager.__init__
+        # otherwise it can outlive this test and get out of sync with pytest
+        with mock.patch('threading.Thread.start') as mock_thread:
             project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')
 
+        mock_thread.assert_called_once()
         # Assert that if invalid blocking_timeout is set, then exception is raised.
         with self.assertRaisesRegex(
             optimizely_exceptions.InvalidInputException, 'Invalid blocking timeout "invalid timeout" provided.',
@@ -352,9 +392,13 @@ def test_set_blocking_timeout(self, _):
 
     def test_set_last_modified(self, _):
         """ Test that set_last_modified sets last_modified field based on header. """
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
+
+        # prevent polling thread from starting in PollingConfigManager.__init__
+        # otherwise it can outlive this test and get out of sync with pytest
+        with mock.patch('threading.Thread.start') as mock_thread:
             project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')
 
+        mock_thread.assert_called_once()
         last_modified_time = 'Test Last Modified Time'
         test_response_headers = {
             'Last-Modified': last_modified_time,
@@ -366,24 +410,40 @@ def test_set_last_modified(self, _):
     def test_fetch_datafile(self, _):
         """ Test that fetch_datafile sets config and last_modified based on response. """
         sdk_key = 'some_key'
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
-            project_config_manager = config_manager.PollingConfigManager(sdk_key=sdk_key)
+
+        # use wrapper class to control start and stop of fetch_datafile
+        # this prevents the polling thread from outliving the test
+        # and getting out of sync with pytest
+        project_config_manager = MockPollingConfigManager(sdk_key=sdk_key)
         expected_datafile_url = enums.ConfigManager.DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key)
         test_headers = {'Last-Modified': 'New Time'}
         test_datafile = json.dumps(self.config_dict_with_features)
         test_response = requests.Response()
         test_response.status_code = 200
         test_response.headers = test_headers
         test_response._content = test_datafile
-        with mock.patch('requests.get', return_value=test_response):
-            project_config_manager.fetch_datafile()
+        with mock.patch('requests.get', return_value=test_response) as mock_request:
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
+        mock_request.assert_called_once_with(
+            expected_datafile_url,
+            headers={},
+            timeout=enums.ConfigManager.REQUEST_TIMEOUT
+        )
         self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified)
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
 
         # Call fetch_datafile again and assert that request to URL is with If-Modified-Since header.
         with mock.patch('requests.get', return_value=test_response) as mock_requests:
-            project_config_manager.fetch_datafile()
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
         mock_requests.assert_called_once_with(
             expected_datafile_url,
@@ -394,6 +454,9 @@ def test_fetch_datafile(self, _):
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
         self.assertTrue(project_config_manager.is_running)
 
+        # Shut down the polling thread
+        project_config_manager.stop = True
+
     def test_fetch_datafile__status_exception_raised(self, _):
         """ Test that config_manager keeps running if status code exception is raised when fetching datafile. """
         class MockExceptionResponse(object):
@@ -402,24 +465,40 @@ def raise_for_status(self):
 
         sdk_key = 'some_key'
         mock_logger = mock.Mock()
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
-            project_config_manager = config_manager.PollingConfigManager(sdk_key=sdk_key, logger=mock_logger)
         expected_datafile_url = enums.ConfigManager.DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key)
         test_headers = {'Last-Modified': 'New Time'}
         test_datafile = json.dumps(self.config_dict_with_features)
         test_response = requests.Response()
         test_response.status_code = 200
         test_response.headers = test_headers
         test_response._content = test_datafile
-        with mock.patch('requests.get', return_value=test_response):
-            project_config_manager.fetch_datafile()
 
+        # use wrapper class to control start and stop of fetch_datafile
+        # this prevents the polling thread from outliving the test
+        # and getting out of sync with pytest
+        project_config_manager = MockPollingConfigManager(sdk_key=sdk_key, logger=mock_logger)
+        with mock.patch('requests.get', return_value=test_response) as mock_request:
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
+
+        mock_request.assert_called_once_with(
+            expected_datafile_url,
+            headers={},
+            timeout=enums.ConfigManager.REQUEST_TIMEOUT
+        )
         self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified)
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
 
         # Call fetch_datafile again, but raise exception this time
         with mock.patch('requests.get', return_value=MockExceptionResponse()) as mock_requests:
-            project_config_manager.fetch_datafile()
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
         mock_requests.assert_called_once_with(
             expected_datafile_url,
@@ -434,22 +513,37 @@ def raise_for_status(self):
         # Confirm that config manager keeps running
         self.assertTrue(project_config_manager.is_running)
 
+        # Shut down the polling thread
+        project_config_manager.stop = True
+
     def test_fetch_datafile__request_exception_raised(self, _):
         """ Test that config_manager keeps running if a request exception is raised when fetching datafile. """
         sdk_key = 'some_key'
         mock_logger = mock.Mock()
-        with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
-            project_config_manager = config_manager.PollingConfigManager(sdk_key=sdk_key, logger=mock_logger)
+
+        # use wrapper class to control start and stop of fetch_datafile
+        # this prevents the polling thread from outliving the test
+        # and getting out of sync with pytest
+        project_config_manager = MockPollingConfigManager(sdk_key=sdk_key, logger=mock_logger)
         expected_datafile_url = enums.ConfigManager.DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key)
         test_headers = {'Last-Modified': 'New Time'}
         test_datafile = json.dumps(self.config_dict_with_features)
         test_response = requests.Response()
         test_response.status_code = 200
         test_response.headers = test_headers
         test_response._content = test_datafile
-        with mock.patch('requests.get', return_value=test_response):
-            project_config_manager.fetch_datafile()
+        with mock.patch('requests.get', return_value=test_response) as mock_request:
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
+        mock_request.assert_called_once_with(
+            expected_datafile_url,
+            headers={},
+            timeout=enums.ConfigManager.REQUEST_TIMEOUT
+        )
         self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified)
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
 
@@ -458,7 +552,11 @@ def test_fetch_datafile__request_exception_raised(self, _):
             'requests.get',
             side_effect=requests.exceptions.RequestException('Error Error !!'),
         ) as mock_requests:
-            project_config_manager.fetch_datafile()
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
         mock_requests.assert_called_once_with(
             expected_datafile_url,
@@ -473,12 +571,18 @@ def test_fetch_datafile__request_exception_raised(self, _):
         # Confirm that config manager keeps running
         self.assertTrue(project_config_manager.is_running)
 
+        # Shut down the polling thread
+        project_config_manager.stop = True
+
     def test_is_running(self, _):
         """ Test that polling thread is running after instance of PollingConfigManager is created. """
         with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
             project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')
             self.assertTrue(project_config_manager.is_running)
 
+        # Prevent the polling thread from running fetch_datafile if it hasn't already
+        project_config_manager._polling_thread._is_stopped = True
+
 
 @mock.patch('requests.get')
 class AuthDatafilePollingConfigManagerTest(base.BaseTest):
@@ -495,10 +599,14 @@ def test_set_datafile_access_token(self, _):
         """ Test that datafile_access_token is properly set as instance variable. """
         datafile_access_token = 'some_token'
         sdk_key = 'some_key'
-        with mock.patch('optimizely.config_manager.AuthDatafilePollingConfigManager.fetch_datafile'):
+
+        # prevent polling thread from starting in PollingConfigManager.__init__
+        # otherwise it can outlive this test and get out of sync with pytest
+        with mock.patch('threading.Thread.start') as mock_thread:
             project_config_manager = config_manager.AuthDatafilePollingConfigManager(
                 datafile_access_token=datafile_access_token, sdk_key=sdk_key)
 
+        mock_thread.assert_called_once()
         self.assertEqual(datafile_access_token, project_config_manager.datafile_access_token)
 
     def test_fetch_datafile(self, _):
@@ -538,9 +646,11 @@ def test_fetch_datafile__request_exception_raised(self, _):
         sdk_key = 'some_key'
         mock_logger = mock.Mock()
 
-        with mock.patch('optimizely.config_manager.AuthDatafilePollingConfigManager.fetch_datafile'):
-            project_config_manager = config_manager.AuthDatafilePollingConfigManager(
-                datafile_access_token=datafile_access_token, sdk_key=sdk_key, logger=mock_logger)
+        # use wrapper class to control start and stop of fetch_datafile
+        # this prevents the polling thread from outliving the test
+        # and getting out of sync with pytest
+        project_config_manager = MockAuthDatafilePollingConfigManager(datafile_access_token=datafile_access_token,
+                                                                      sdk_key=sdk_key, logger=mock_logger)
         expected_datafile_url = enums.ConfigManager.AUTHENTICATED_DATAFILE_URL_TEMPLATE.format(sdk_key=sdk_key)
         test_headers = {'Last-Modified': 'New Time'}
         test_datafile = json.dumps(self.config_dict_with_features)
@@ -552,7 +662,11 @@ def test_fetch_datafile__request_exception_raised(self, _):
         # Call fetch_datafile and assert that request was sent with correct authorization header
         with mock.patch('requests.get',
                         return_value=test_response) as mock_request:
-            project_config_manager.fetch_datafile()
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
         mock_request.assert_called_once_with(
             expected_datafile_url,
@@ -568,7 +682,11 @@ def test_fetch_datafile__request_exception_raised(self, _):
             'requests.get',
             side_effect=requests.exceptions.RequestException('Error Error !!'),
         ) as mock_requests:
-            project_config_manager.fetch_datafile()
+            # manually trigger fetch_datafile in the polling thread
+            project_config_manager.run = True
+            # Wait for polling thread to finish
+            while project_config_manager.run:
+                pass
 
         mock_requests.assert_called_once_with(
             expected_datafile_url,
@@ -586,3 +704,6 @@ def test_fetch_datafile__request_exception_raised(self, _):
         self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
         # Confirm that config manager keeps running
         self.assertTrue(project_config_manager.is_running)
+
+        # Shut down the polling thread
+        project_config_manager.stop = True

tests/test_user_context.py

@@ -1,4 +1,4 @@
-# Copyright 2021, Optimizely
+# Copyright 2021-2022, Optimizely
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
@@ -1859,6 +1859,28 @@ def clone_loop(user_context):
                 for x in range(100):
                     user_context._clone()
 
+            # custom call counter because the mock call_count is not thread safe
+            class MockCounter:
+                def __init__(self):
+                    self.lock = threading.Lock()
+                    self.call_count = 0
+
+                def increment(self, *args):
+                    with self.lock:
+                        self.call_count += 1
+
+            set_forced_decision_counter = MockCounter()
+            get_forced_decision_counter = MockCounter()
+            remove_forced_decision_counter = MockCounter()
+            remove_all_forced_decisions_counter = MockCounter()
+            clone_counter = MockCounter()
+
+            set_forced_decision_mock.side_effect = set_forced_decision_counter.increment
+            get_forced_decision_mock.side_effect = get_forced_decision_counter.increment
+            remove_forced_decision_mock.side_effect = remove_forced_decision_counter.increment
+            remove_all_forced_decisions_mock.side_effect = remove_all_forced_decisions_counter.increment
+            clone_mock.side_effect = clone_counter.increment
+
             set_thread_1 = threading.Thread(target=set_forced_decision_loop, args=(user_context, context_1, decision_1))
             set_thread_2 = threading.Thread(target=set_forced_decision_loop, args=(user_context, context_2, decision_2))
             set_thread_3 = threading.Thread(target=get_forced_decision_loop, args=(user_context, context_1))
@@ -1888,8 +1910,8 @@ def clone_loop(user_context):
             set_thread_7.join()
             set_thread_8.join()
 
-        self.assertEqual(200, set_forced_decision_mock.call_count)
-        self.assertEqual(200, get_forced_decision_mock.call_count)
-        self.assertEqual(200, remove_forced_decision_mock.call_count)
-        self.assertEqual(100, remove_all_forced_decisions_mock.call_count)
-        self.assertEqual(100, clone_mock.call_count)
+        self.assertEqual(200, set_forced_decision_counter.call_count)
+        self.assertEqual(200, get_forced_decision_counter.call_count)
+        self.assertEqual(200, remove_forced_decision_counter.call_count)
+        self.assertEqual(100, remove_all_forced_decisions_counter.call_count)
+        self.assertEqual(100, clone_counter.call_count)