From 230ba6ad1057574c9f3d42a97f890788cd9ec6c3 Mon Sep 17 00:00:00 2001
From: Paul Ollis <paul@cleversheep.org>
Date: Sat, 11 Nov 2023 14:24:21 +0000
Subject: [PATCH] Properly wait for workers when test run terminates early
 (#963)

Currently, a reason to terminate early (e.g. test failure with --exitfail
option set) causes DSession to immediately raise an Interrupt exception.
Subsequent reports generated by the workers, during the shutdown phase, are
discarded.

One consequence is that, for the failing test, teardown and testfinish are
ignored, which prevents corresponding hooks pytest_runtest_logreport and
pytest_runtest_logfinish being executed for the failing test (this problem
covered in #54). The reporting of tests executing in other workers is also left in an
indeterminate state. This can affect other plugin code.

This is a relatively simple fix, which appears to have minimal and, I think,
acceptable impact on text execution behaviour.

The observable differences are differences in what is reported about a test
run. For example, when running, for example with the '-x/--exitfail' option, it
is possible that more than a single test failure is reported. This is because
more than one test did fail before the test run was completely stopped. The
reporting is absolutely correct; and complete. Prior to this change, only a
single failure would have been reported, but because of incomplete and arguably
incorrect reporting.
---
 changelog/963.improvement.rst |  5 +++++
 src/xdist/dsession.py         | 20 ++++++++++++++------
 testing/acceptance_test.py    | 34 ++++++++++++++++++++++++++++++----
 3 files changed, 49 insertions(+), 10 deletions(-)
 create mode 100644 changelog/963.improvement.rst

diff --git a/changelog/963.improvement.rst b/changelog/963.improvement.rst
new file mode 100644
index 0000000..6cf96e2
--- /dev/null
+++ b/changelog/963.improvement.rst
@@ -0,0 +1,5 @@
+Wait for workers to finish reporting when test run stops early.
+
+This makes sure that the results of in-progress tests are displayed.
+Previously these reports were being discarded, losing information about the
+test run.
diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py
index 39d1afa..9dcfc2a 100644
--- a/src/xdist/dsession.py
+++ b/src/xdist/dsession.py
@@ -118,11 +118,14 @@ class DSession:
         assert self.sched is not None
 
         self.shouldstop = False
+        pending_exception = None
         while not self.session_finished:
             self.loop_once()
             if self.shouldstop:
                 self.triggershutdown()
-                raise Interrupted(str(self.shouldstop))
+                pending_exception = Interrupted(str(self.shouldstop))
+        if pending_exception:
+            raise pending_exception
         return True
 
     def loop_once(self):
@@ -351,14 +354,19 @@ class DSession:
     def _handlefailures(self, rep):
         if rep.failed:
             self.countfailures += 1
-            if self.maxfail and self.countfailures >= self.maxfail:
+            if (
+                self.maxfail
+                and self.countfailures >= self.maxfail
+                and not self.shouldstop
+            ):
                 self.shouldstop = f"stopping after {self.countfailures} failures"
 
     def triggershutdown(self):
-        self.log("triggering shutdown")
-        self.shuttingdown = True
-        for node in self.sched.nodes:
-            node.shutdown()
+        if not self.shuttingdown:
+            self.log("triggering shutdown")
+            self.shuttingdown = True
+            for node in self.sched.nodes:
+                node.shutdown()
 
     def handle_crashitem(self, nodeid, worker):
         # XXX get more reporting info by recording pytest_runtest_logstart?
diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py
index 79fe2cd..60edd9c 100644
--- a/testing/acceptance_test.py
+++ b/testing/acceptance_test.py
@@ -109,18 +109,44 @@ class TestDistribution:
         )
         assert result.ret == 1
 
-    def test_n1_fail_minus_x(self, pytester: pytest.Pytester) -> None:
+    def test_exitfail_waits_for_workers_to_finish(
+        self, pytester: pytest.Pytester
+    ) -> None:
+        """The DSession waits for workers before exiting early on failure.
+
+        When -x/--exitfail is set, the DSession wait for the workers to finish
+        before raising an Interrupt exception. This prevents reports from the
+        faiing test and other tests from being discarded.
+        """
         p1 = pytester.makepyfile(
             """
+            import time
+
             def test_fail1():
+                time.sleep(0.1)
                 assert 0
             def test_fail2():
+                time.sleep(0.2)
+            def test_fail3():
+                time.sleep(0.3)
                 assert 0
+            def test_fail4():
+                time.sleep(0.3)
+            def test_fail5():
+                time.sleep(0.3)
+            def test_fail6():
+                time.sleep(0.3)
         """
         )
-        result = pytester.runpytest(p1, "-x", "-v", "-n1")
+        result = pytester.runpytest(p1, "-x", "-rA", "-v", "-n2")
         assert result.ret == 2
-        result.stdout.fnmatch_lines(["*Interrupted: stopping*1*", "*1 failed*"])
+        result.stdout.re_match_lines([".*Interrupted: stopping.*[12].*"])
+        m = re.search(r"== (\d+) failed, (\d+) passed in ", str(result.stdout))
+        assert m
+        n_failed, n_passed = (int(s) for s in m.groups())
+        assert 1 <= n_failed <= 2
+        assert 1 <= n_passed <= 3
+        assert (n_passed + n_failed) < 6
 
     def test_basetemp_in_subprocesses(self, pytester: pytest.Pytester) -> None:
         p1 = pytester.makepyfile(
@@ -1150,7 +1176,7 @@ def test_internal_error_with_maxfail(pytester: pytest.Pytester) -> None:
     """
     )
     result = pytester.runpytest_subprocess("--maxfail=1", "-n1")
-    result.stdout.fnmatch_lines(["* 1 error in *"])
+    result.stdout.re_match_lines([".* [12] errors? in .*"])
     assert "INTERNALERROR" not in result.stderr.str()