From bc81230311ff9967526b1618b04acfd2b5398c6c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 6 Nov 2023 12:02:39 +0000
Subject: [PATCH] [FIX] core: skip marking completed tours as failed, restore
 had_failure

Succeeds #140464

Misunderstood `had_failure` and should not have reused it, its goal is
to avoid eagerly aborting some JS tests -- specifically the unit test
suites -- while still logging errors normally (useful when watching
interactively, or for the runbot's own reporting).

So the *checks* added on `had_failure` should in fact be checks on
`_result.exception()`, and as it turns out on `_result.done()`: if a
tour is already marked as successful we can't fail it either.

So we should not, we should log an error (to notify the caller /
runbot) and then bail. While #140464 did improve some things, we could
still lose legit errors and get pages of unhelpful `InvalidStateError`
if a tour would succeed *then* failures would occur, as the guard only
checked that the tour had already failed.

closes odoo/odoo#141815

X-original-commit: 87fcf66203dcbd281470285d648365d33d0e2fca
Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>
---
 odoo/tests/common.py | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/odoo/tests/common.py b/odoo/tests/common.py
index 7a3c9df2343..9cbb87ae686 100644
--- a/odoo/tests/common.py
+++ b/odoo/tests/common.py
@@ -917,6 +917,7 @@ class ChromeBrowser:
         self._request_id = itertools.count()
         self._result = Future()
         self.error_checker = None
+        self.had_failure = False
         # maps request_id to Futures
         self._responses = {}
         # maps frame ids to callbacks
@@ -942,12 +943,6 @@ class ChromeBrowser:
     def screencasts_frames_dir(self):
         return os.path.join(self.screencasts_dir, 'frames')
 
-    @property
-    def had_failure(self):
-        with contextlib.suppress(concurrent.futures.TimeoutError, CancelledError):
-            return self._result.exception(timeout=0) is not None
-        return False
-
     def signal_handler(self, sig, frame):
         if sig == signal.SIGXCPU:
             _logger.info('CPU time limit reached, stopping Chrome and shutting down')
@@ -1243,13 +1238,17 @@ class ChromeBrowser:
             message += '\n' + stack
 
         log_type = type
-        self._logger.getChild('browser').log(
+        _logger = self._logger.getChild('browser')
+        _logger.log(
             self._TO_LEVEL.get(log_type, logging.INFO),
-            "%s", message # might still have %<x> characters
+            "%s%s",
+            "Error received after termination: " if self._result.done() else "",
+            message # might still have %<x> characters
         )
 
         if log_type == 'error':
-            if self.had_failure:
+            self.had_failure = True
+            if self._result.done():
                 return
             if not self.error_checker or self.error_checker(message):
                 self.take_screenshot()
@@ -1283,23 +1282,23 @@ class ChromeBrowser:
 
                 if node_id:
                     self.take_screenshot("unsaved_form_")
-                    self._result.set_exception(ChromeBrowserException("""\
+                    msg = """\
 Tour finished with an open form view in edition mode.
 
 Form views in edition mode are automatically saved when the page is closed, \
-which leads to stray network requests and inconsistencies."""))
+which leads to stray network requests and inconsistencies."""
+                    if self._result.done():
+                        _logger.error("%s", msg)
+                    else:
+                        self._result.set_exception(ChromeBrowserException(msg))
                     return
 
-                try:
+                if not self._result.done():
                     self._result.set_result(True)
-                except Exception:
+                elif self._result.exception() is None:
                     # if the future was already failed, we're happy,
                     # otherwise swap for a new failed
-                    if self._result.exception() is None:
-                        self._result = Future()
-                        self._result.set_exception(ChromeBrowserException(
-                            "Tried to make the tour successful twice."
-                        ))
+                    _logger.error("Tried to make the tour successful twice.")
 
 
     def _handle_exception(self, exceptionDetails, timestamp):
@@ -1312,8 +1311,9 @@ which leads to stray network requests and inconsistencies."""))
         if stack:
             message += '\n' + stack
 
-        if self.had_failure:
-            self._logger.getChild('browser').error("%s", message)
+        if self._result.done():
+            self._logger.getChild('browser').error(
+                "Exception received after termination: %s", message)
             return
 
         self.take_screenshot()