Merge lp:~maxiberta/canonical-identity-provider/improve-throttle-tests into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Some cleanup of API throttling tests.

- De-duplicated code in a number of helper test functions (particularly `throttle_by_*`).
- Dropped single-line helper function (`patch_piston_cache`).
- Replaced literal `3600` (mocked request counter in cache) by `settings.THROTTLE_MAX_REQUESTS`.
- Replaced literal `42` (mocked request counter in cache) by `settings.THROTTLE_MAX_REQUESTS - 1`, which also prevents confusion with `42` as cache expiration time.
- Increased mocked expiration time from `time.time() + 42.1` to `time.time() + 42.99`, which should lower the chances of time-sensitive tests failing under load (with "AssertionError: '41' != u'42'") by allowing an extra 0.89 sec margin; but kept vila's FIXME note in case tests fail again. In that case we can resort to mocking `time()` as vila suggested, but I'd rather try this simple cleanup first.

[Daniel Manrique (roadmr)]

nice, I guess 42 was overused as an universal constant.

Thanks!

review: Approve

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2018-08-17 18:38:29 +0000
+++ src/api/v20/tests/test_handlers.py	2018-09-13 20:30:42 +0000
@@ -2164,46 +2164,26 @@
 # entries and use them (instead of mocking them) -- vila 2015-11-13
 class ThrottledTestCase(BaseTestCase):
 
-    def patch_piston_cache(self):
-        return self.patch('piston.utils.cache')
-
     def throttle_all(self):
-        mock_cache = self.patch_piston_cache()
+        mock_cache = self.patch('piston.utils.cache')
         # hit max number of requests allowed
-        mock_cache.get.return_value = (3600, time.time() + 42.1)
+        mock_cache.get.return_value = (
+            settings.THROTTLE_MAX_REQUESTS, time.time() + 42.99)
 
     def throttle_by_openid(self, openid):
-        mock_cache = self.patch_piston_cache()
+        mock_cache = self.patch('piston.utils.cache')
 
         def mock_get(key, default=None, version=None):
             # only look for openid in extra part
             # the first part of the key is the requester username
             _, extra = key.split(':', 1)
             if openid in extra:
-                return (3600, time.time() + 42.1)
-            else:
-                return (42, time.time() + 42.1)
-        mock_cache.get = mock_get
-
-    def throttle_by_email(self, email):
-        mock_cache = self.patch_piston_cache()
-
-        def mock_get(key, default=None, version=None):
-            if email in key:
-                return (3600, time.time() + 42.1)
-            else:
-                return (42, time.time() + 42.1)
-        mock_cache.get = mock_get
-
-    def throttle_by_token(self, token):
-        mock_cache = self.patch_piston_cache()
-
-        def mock_get(key, default=None, version=None):
-            if token in key:
-                return (3600, time.time() + 42.1)
-            else:
-                return (42, time.time() + 42.1)
-        mock_cache.get = mock_get
+                return settings.THROTTLE_MAX_REQUESTS, time.time() + 42.99
+            else:
+                return settings.THROTTLE_MAX_REQUESTS - 1, time.time() + 42.99
+        mock_cache.get = mock_get
+
+    throttle_by_email = throttle_by_token = throttle_by_openid
 
     def assert_handler_throttled(
             self, request, token=None, throttle_func=None, throttled=True,
@@ -2480,7 +2460,7 @@
         with patch(
                 'api.v20.decorators.stats.increment', called_metrics.append):
             with patch('piston.utils.cache') as mock_cache:
-                mock_cache.get.return_value = (0, time.time() + 42.1)
+                mock_cache.get.return_value = (0, time.time() + 42.99)
                 self.get_response(request.path, method=request.method)
         self.assertEqual([], called_metrics)