From 6c3bbdbef8fa8e1caa412beb83dd9a023f2e972f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 29 Mar 2022 05:52:41 +0000 Subject: [PATCH] [FIX] core: fix "generator didn't yield" in assertRaises Because the `savepoint` and `clear` calls are nested inside the `assertRaises` context, if one of them happens to throw *the exception we're looking for* the interpreter will jump back to the `with`, the `assertRaises` will swallow the exception (and count it as a success) and the function will end having not gone through a `yield`. This is rather frustrating to debug as it's easy to forget that a `with` is a control flow structure, leading to a seemingly impossible error. We can fix this by initializing and `__enter__`-ing the savepoint first, but doing this by hand is a bit iffy and not really future-proof as the addition of more fallible steps during the initialization phase of `_assertRaises` could lead to the savepoint not being properly disposed of. Furthermore once in the scope of the "actual" assertRaises we want the savepoint to unwind first (otherwise the savepoint won't be rolled back when the exception *we are expecting* gets raised). As it turns out `ExitStack` offers the solution to our woes though it's a bit tricky at first glance: while modifying the cleanup queue in-place is haram, `pop_all` allows moving cleanup callbacks from one queue to the next. This means we can first add the savepoint to one stack and get its errors (if any) correctly reported, cover the rest of the initialization, then move the savepoint from one stack to an other, in order to correctly order the coverage of the `yield` (and the userland code). closes odoo/odoo#87733 X-original-commit: b1cd4e4e3c918b4b08d27b30b1017fc898d4b08a Signed-off-by: Xavier Morel (xmo) --- .../tests/test_methods.py | 59 +++++++++++++++++++ odoo/tests/common.py | 29 +++++---- 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/odoo/addons/test_testing_utilities/tests/test_methods.py b/odoo/addons/test_testing_utilities/tests/test_methods.py index 2a783aa2166..005914391d1 100644 --- a/odoo/addons/test_testing_utilities/tests/test_methods.py +++ b/odoo/addons/test_testing_utilities/tests/test_methods.py @@ -1,9 +1,19 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import itertools +from unittest import mock, TestCase +import psycopg2 + +from odoo.exceptions import AccessError +from odoo.sql_db import BaseCursor from odoo.tests import common +from odoo.tools import mute_logger +class CustomError(Exception): + ... + class TestBasic(common.TransactionCase): def test_assertRecordValues(self): X1 = {'f1': "X", 'f2': 1} @@ -28,3 +38,52 @@ class TestBasic(common.TransactionCase): self.assertRecordValues(records, [X1, Y3]) with self.assertRaises(AssertionError): self.assertRecordValues(records, [Y3, X1]) + + def test_assertRaises_rollbacks(self): + """Checks that a "correctly" executing assertRaises (where the expected + exception has been raised and caught) will properly rollback. + """ + self.env.cr.execute("SET LOCAL test_testing_utilities.a_flag = ''") + with self.assertRaises(CustomError): + self.env.cr.execute("SET LOCAL test_testing_utilities.a_flag = 'yes'") + raise CustomError + + self.env.cr.execute("SHOW test_testing_utilities.a_flag") + self.assertEqual(self.env.cr.fetchone(), ('',)) + + def test_assertRaises_error(self): + """Checks that an exception raised during the *setup* of assertRaises + bubbles up correctly. + + Raises an exception when `savepoint()` calls `flush()` during setup. + """ + # ensure we catch the error with the "base" method to avoid any interference + with mock.patch.object(BaseCursor, 'flush', side_effect=CustomError), \ + TestCase.assertRaises(self, CustomError): + with self.assertRaises(CustomError): + raise NotImplementedError + + @mute_logger('odoo.sql_db') + def test_assertRaises_clear_recovery(self): + """Checks that the savepoint is correctly rolled back if an error occurs + during the assertRaises setup + + Raises an exception during the first `clear()` calls which immediately + follows the initialisation of the savepoint iff we're expecting an + AccessError. + """ + # on the first `clear` call, break the current transaction with nonsense + # (on further calls do nothing as savepoint() needs to clear() for its + # own recovery) + def clear(call_count=itertools.count()): + if next(call_count) == 0: + self.env.cr.execute('select nonsense') + + with mock.patch.object(BaseCursor, 'clear', side_effect=clear),\ + TestCase.assertRaises(self, psycopg2.Error): + with self.assertRaises(AccessError): + raise NotImplementedError + + # check that the transaction has been rolled back and we can perform + # queries again + self.env.cr.execute('select 1') diff --git a/odoo/tests/common.py b/odoo/tests/common.py index a0b69d29bd9..62fd11ae26b 100644 --- a/odoo/tests/common.py +++ b/odoo/tests/common.py @@ -35,7 +35,7 @@ try: from concurrent.futures import InvalidStateError except ImportError: InvalidStateError = NotImplementedError -from contextlib import contextmanager +from contextlib import contextmanager, ExitStack from datetime import datetime, date from itertools import zip_longest as izip_longest from unittest.mock import patch @@ -439,17 +439,24 @@ class BaseCase(unittest.TestCase, metaclass=MetaCase): @contextmanager def _assertRaises(self, exception, *, msg=None): """ Context manager that clears the environment upon failure. """ - with super(BaseCase, self).assertRaises(exception, msg=msg) as cm: + with ExitStack() as init: if hasattr(self, 'env'): - with self.env.cr.savepoint(): - if issubclass(exception, AccessError): - # The savepoint() above calls flush(), which leaves the - # record cache with lots of data. This can prevent - # access errors to be detected. In order to avoid this - # issue, we clear the cache before proceeding. - self.env.cr.clear() - yield cm - else: + init.enter_context(self.env.cr.savepoint()) + if issubclass(exception, AccessError): + # The savepoint() above calls flush(), which leaves the + # record cache with lots of data. This can prevent + # access errors to be detected. In order to avoid this + # issue, we clear the cache before proceeding. + self.env.cr.clear() + + with ExitStack() as inner: + cm = inner.enter_context(super().assertRaises(exception, msg=msg)) + # *moves* the cleanups from init to inner, this ensures the + # savepoint gets rolled back when `yield` raises `exception`, + # but still allows the initialisation to be protected *and* not + # interfered with by `assertRaises`. + inner.push(init.pop_all()) + yield cm def assertRaises(self, exception, func=None, *args, **kwargs):