[IMP] core: improve test request blocker

Better tag selection
====================

The block was originally hooked onto the `external` tag under the
assumption that this was the tag used to allow external requests.

It's not, it's the tag for the "external" test suite, which is only
one of the test suites allowed to perform external calls. Hook again
onto `standard`, this might have some false positives (allow requests
which we'd rather block), but it should have a lot less false
negatives (block requests we want to allow).

Make Overrides Easier
=====================

Currently `_request_handler` raises a regular `ConnectionError`, this
is an issue because it passes some requests through, which could
themselves trigger genuine `ConnectionError`.

When overriding `_request_handler` to implement fallbacks for bespoke
URL mocks, these two cases need to be distinguishable as overrides
likely want to handle blocked requests, not actual failures.

Therefore `_request_handler` should a dedicated exception. This
exception should be a subclass of `ConnectionError`, so that blocked
requests are treated as regular connection failures by normal Odoo
code.

Class-scope
===========

Originally `_request_handler` was scoped on the instance with the idea
that it'd be a `mock` object, which individual tests could
`configure_mock`. This turned out not to work correctly, because
`Mock.side_effect` does not receive a `self`, hence the `Session` was
inaccessible and it was not possible to passthrough local requests.

While I moved to a regular `lambda` (because a direct method didn't
work either), I forgot to remove the `request_mock` attribute, and
didn't think that the block could now be lifted up to the class
scope. Doing this, requests performed during a "standard" test case's
`setUpClass` are now also blocked, which they very much should be.

closes odoo/odoo#128977

Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>

addons/mass_mailing/tests/test_mailing_controllers.py

@@ -35,7 +35,8 @@ class TestMailingControllers(MassMailCommon, HttpCase):
         # freeze time base value
         cls._reference_now = datetime.datetime(2022, 6, 14, 10, 0, 0)
 
-    def _request_handler(self, s: Session, r: PreparedRequest, /, **kw):
+    @classmethod
+    def _request_handler(cls, s: Session, r: PreparedRequest, /, **kw):
         if r.url.startswith('https://www.example.com/foo/bar'):
             r = Response()
             r.status_code = 200

odoo/tests/common.py

@@ -244,6 +244,8 @@ def _normalize_arch_for_assert(arch_string, parser_method="xml"):
     arch_string = etree.fromstring(arch_string, parser=parser)
     return etree.tostring(arch_string, pretty_print=True, encoding='unicode')
 
+class BlockedRequest(requests.exceptions.ConnectionError):
+    pass
 _super_send = requests.Session.send
 class BaseCase(case.TestCase, metaclass=MetaCase):
     """ Subclass of TestCase for Odoo-specific code. This class is abstract and
@@ -258,21 +260,9 @@ class BaseCase(case.TestCase, metaclass=MetaCase):
         super().__init__(methodName)
         self.addTypeEqualityFunc(etree._Element, self.assertTreesEqual)
         self.addTypeEqualityFunc(html.HtmlElement, self.assertTreesEqual)
-        self.request_mock = None
 
-    def setUp(self):
-        super().setUp()
-        if 'external' not in self.test_tags:
-            # if the method is passed directly `patch` discards the session
-            # object which we need
-            # pylint: disable=unnecessary-lambda
-            self.patch(
-                requests.sessions.Session,
-                'send',
-                lambda s, r, **kwargs: self._request_handler(s, r, **kwargs),
-            )
-
-    def _request_handler(self, s: Session, r: PreparedRequest, /, **kw):
+    @classmethod
+    def _request_handler(cls, s: Session, r: PreparedRequest, /, **kw):
         # allow localhost requests
         # TODO: also check port?
         url = werkzeug.urls.url_parse(r.url)
@@ -283,8 +273,7 @@ class BaseCase(case.TestCase, metaclass=MetaCase):
 
         _logger.getChild('requests').info(
             "Blocking un-mocked external HTTP request %s %s", r.method, r.url)
-        raise requests.exceptions.ConnectionError(
-            f"External requests verboten (was {r.method} {r.url})")
+        raise BlockedRequest(f"External requests verboten (was {r.method} {r.url})")
 
     def run(self, result):
         testMethod = getattr(self, self._testMethodName)
@@ -318,6 +307,17 @@ class BaseCase(case.TestCase, metaclass=MetaCase):
                 patcher.stop()
         cls.addClassCleanup(check_remaining_patchers)
         super().setUpClass()
+        if 'standard' in cls.test_tags:
+            # if the method is passed directly `patch` discards the session
+            # object which we need
+            # pylint: disable=unnecessary-lambda
+            patcher = patch.object(
+                requests.sessions.Session,
+                'send',
+                lambda s, r, **kwargs: cls._request_handler(s, r, **kwargs),
+            )
+            patcher.start()
+            cls.addClassCleanup(patcher.stop)
 
     def cursor(self):
         return self.registry.cursor()
@@ -792,8 +792,6 @@ class TransactionCase(BaseCase):
         self.cr.execute('SAVEPOINT test_%d' % self._savepoint_id)
         self.addCleanup(self.cr.execute, 'ROLLBACK TO SAVEPOINT test_%d' % self._savepoint_id)
 
-        self.patch(self.registry['res.partner'], '_get_gravatar_image', lambda *a: False)
-
 
 class SingleTransactionCase(BaseCase):
     """ TestCase in which all test methods are run in the same transaction,