From f96988cbe55d44ae29ff562ed0ed3716ef9e1304 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 19 Jul 2023 08:37:16 +0000 Subject: [PATCH] [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) --- .../tests/test_mailing_controllers.py | 3 +- odoo/tests/common.py | 34 +++++++++---------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/addons/mass_mailing/tests/test_mailing_controllers.py b/addons/mass_mailing/tests/test_mailing_controllers.py index a7777616ab8..49ad26ef02b 100644 --- a/addons/mass_mailing/tests/test_mailing_controllers.py +++ b/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 diff --git a/odoo/tests/common.py b/odoo/tests/common.py index ab381bad1c3..d70fef9337c 100644 --- a/odoo/tests/common.py +++ b/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,