[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) <xmo@odoo.com>

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')

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):