From e11ad4dcc0ce6da53d5ede96a53072a7c1e6599f Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Sun, 25 Feb 2024 16:28:09 -0500 Subject: [PATCH 1/7] Terminate worker test loop if session sets shouldstop or shouldfail (#1024) --- src/xdist/dsession.py | 2 +- src/xdist/remote.py | 2 ++ testing/acceptance_test.py | 30 ++++++++++++++++++++++++------ 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index 9dcfc2a6..d8a95c92 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -182,7 +182,7 @@ def worker_workerfinished(self, node): self.shouldstop = f"{node} received keyboard-interrupt" self.worker_errordown(node, "keyboard-interrupt") return - if node in self.sched.nodes: + if not self.shouldstop and node in self.sched.nodes: crashitem = self.sched.remove_node(node) assert not crashitem, (crashitem, node) self._active_nodes.remove(node) diff --git a/src/xdist/remote.py b/src/xdist/remote.py index e035f772..eb3181e2 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -155,6 +155,8 @@ def pytest_runtestloop(self, session): self.nextitem_index = self._get_next_item_index() while self.nextitem_index is not self.SHUTDOWN_MARK: self.run_one_test() + if session.shouldfail or session.shouldstop: + break return True def run_one_test(self): diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 7b83f021..9ca33ffc 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -109,12 +109,12 @@ def test_skip(): ) assert result.ret == 1 - def test_exitfail_waits_for_workers_to_finish( + def test_exitfirst_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 + When -x/--exitfirst is set, the DSession wait for all workers to finish before raising an Interrupt exception. This prevents reports from the faiing test and other tests from being discarded. """ @@ -138,15 +138,16 @@ def test_fail6(): time.sleep(0.3) """ ) + # Two workers are used result = pytester.runpytest(p1, "-x", "-rA", "-v", "-n2") assert result.ret == 2 + # DSession should stop when the first failure is reached. Two failures + # may actually occur, due to timing. result.stdout.re_match_lines([".*Interrupted: stopping.*[12].*"]) - m = re.search(r"== (\d+) failed, (\d+) passed in ", str(result.stdout)) + m = re.search(r"== (\d+) failed", str(result.stdout)) assert m - n_failed, n_passed = (int(s) for s in m.groups()) + n_failed = int(m.groups()[0]) 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( @@ -1180,6 +1181,23 @@ def test_aaa1(crasher): assert "INTERNALERROR" not in result.stderr.str() +def test_maxfail_causes_early_termination(pytester: pytest.Pytester) -> None: + """ + Ensure subsequent tests on a worker aren't run when using --maxfail (#1024). + """ + pytester.makepyfile( + """ + def test1(): + assert False + + def test2(): + pass + """ + ) + result = pytester.runpytest_subprocess("--maxfail=1", "-n 1") + result.stdout.re_match_lines([".* 1 failed in .*"]) + + def test_internal_errors_propagate_to_controller(pytester: pytest.Pytester) -> None: pytester.makeconftest( """ From a022a1cec601b936dfd03cac67757bd9f0fa9acd Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Mon, 4 Mar 2024 17:22:45 -0500 Subject: [PATCH 2/7] PR feedback: Improve unit tests by assuming parseoutcomes and assert_outcomes --- testing/acceptance_test.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 9ca33ffc..acfa8d4b 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -143,11 +143,9 @@ def test_fail6(): assert result.ret == 2 # DSession should stop when the first failure is reached. Two failures # may actually occur, due to timing. - result.stdout.re_match_lines([".*Interrupted: stopping.*[12].*"]) - m = re.search(r"== (\d+) failed", str(result.stdout)) - assert m - n_failed = int(m.groups()[0]) - assert 1 <= n_failed <= 2 + outcomes = result.parseoutcomes() + assert "failed" in outcomes, "Expected at least one failure" + assert 1 <= outcomes["failed"] <= 2, "Expected no more than 2 failures" def test_basetemp_in_subprocesses(self, pytester: pytest.Pytester) -> None: p1 = pytester.makepyfile( @@ -1195,7 +1193,7 @@ def test2(): """ ) result = pytester.runpytest_subprocess("--maxfail=1", "-n 1") - result.stdout.re_match_lines([".* 1 failed in .*"]) + result.assert_outcomes(failed=1) def test_internal_errors_propagate_to_controller(pytester: pytest.Pytester) -> None: From c7a19c3d290e0a1eca0703c7d5d51fc7bef7cbbe Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Sat, 23 Mar 2024 13:32:21 -0400 Subject: [PATCH 3/7] New solution that does not require change to dsession --- src/xdist/dsession.py | 2 +- src/xdist/remote.py | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index d8a95c92..9dcfc2a6 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -182,7 +182,7 @@ def worker_workerfinished(self, node): self.shouldstop = f"{node} received keyboard-interrupt" self.worker_errordown(node, "keyboard-interrupt") return - if not self.shouldstop and node in self.sched.nodes: + if node in self.sched.nodes: crashitem = self.sched.remove_node(node) assert not crashitem, (crashitem, node) self._active_nodes.remove(node) diff --git a/src/xdist/remote.py b/src/xdist/remote.py index eb3181e2..214df065 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -154,12 +154,10 @@ def pytest_runtestloop(self, session): self.channel.setcallback(self.handle_command, endmarker=self.SHUTDOWN_MARK) self.nextitem_index = self._get_next_item_index() while self.nextitem_index is not self.SHUTDOWN_MARK: - self.run_one_test() - if session.shouldfail or session.shouldstop: - break + self.run_one_test(session.shouldfail or session.shouldstop) return True - def run_one_test(self): + def run_one_test(self, skip: bool): self.item_index = self.nextitem_index self.nextitem_index = self._get_next_item_index() @@ -173,7 +171,10 @@ def run_one_test(self): worker_title("[pytest-xdist running] %s" % item.nodeid) start = time.time() - self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem) + + if not skip: + self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem) + duration = time.time() - start worker_title("[pytest-xdist idle]") From 594ba7ad2311663167e1099d033cd0d7dfaa7bf5 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Sat, 23 Mar 2024 16:28:27 -0400 Subject: [PATCH 4/7] Revert "New solution that does not require change to dsession" This reverts commit c7a19c3d290e0a1eca0703c7d5d51fc7bef7cbbe. --- src/xdist/dsession.py | 2 +- src/xdist/remote.py | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index 9dcfc2a6..d8a95c92 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -182,7 +182,7 @@ def worker_workerfinished(self, node): self.shouldstop = f"{node} received keyboard-interrupt" self.worker_errordown(node, "keyboard-interrupt") return - if node in self.sched.nodes: + if not self.shouldstop and node in self.sched.nodes: crashitem = self.sched.remove_node(node) assert not crashitem, (crashitem, node) self._active_nodes.remove(node) diff --git a/src/xdist/remote.py b/src/xdist/remote.py index 214df065..eb3181e2 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -154,10 +154,12 @@ def pytest_runtestloop(self, session): self.channel.setcallback(self.handle_command, endmarker=self.SHUTDOWN_MARK) self.nextitem_index = self._get_next_item_index() while self.nextitem_index is not self.SHUTDOWN_MARK: - self.run_one_test(session.shouldfail or session.shouldstop) + self.run_one_test() + if session.shouldfail or session.shouldstop: + break return True - def run_one_test(self, skip: bool): + def run_one_test(self): self.item_index = self.nextitem_index self.nextitem_index = self._get_next_item_index() @@ -171,10 +173,7 @@ def run_one_test(self, skip: bool): worker_title("[pytest-xdist running] %s" % item.nodeid) start = time.time() - - if not skip: - self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem) - + self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem) duration = time.time() - start worker_title("[pytest-xdist idle]") From c927b1a3af5f040552861d03a1dac8a69940ca46 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Sat, 23 Mar 2024 16:52:05 -0400 Subject: [PATCH 5/7] PR feedback: Propagate worker shouldfail/shouldstop info to DSession --- src/xdist/dsession.py | 11 ++++++++++- src/xdist/remote.py | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index d8a95c92..0a49d696 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -182,7 +182,16 @@ def worker_workerfinished(self, node): self.shouldstop = f"{node} received keyboard-interrupt" self.worker_errordown(node, "keyboard-interrupt") return - if not self.shouldstop and node in self.sched.nodes: + + shouldfail = node.workeroutput["shouldfail"] + shouldstop = node.workeroutput["shouldstop"] + for shouldx in [shouldfail, shouldstop]: + if shouldx: + self.shouldstop = shouldx + self._active_nodes.remove(node) + return + + if node in self.sched.nodes: crashitem = self.sched.remove_node(node) assert not crashitem, (crashitem, node) self._active_nodes.remove(node) diff --git a/src/xdist/remote.py b/src/xdist/remote.py index eb3181e2..a5acf21e 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -103,6 +103,8 @@ def pytest_sessionstart(self, session): def pytest_sessionfinish(self, exitstatus): # in pytest 5.0+, exitstatus is an IntEnum object self.config.workeroutput["exitstatus"] = int(exitstatus) + self.config.workeroutput["shouldfail"] = self.session.shouldfail + self.config.workeroutput["shouldstop"] = self.session.shouldstop yield self.sendevent("workerfinished", workeroutput=self.config.workeroutput) From 7cb5c4908760d84d5a6115271f2a532f3d667074 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Mon, 25 Mar 2024 14:47:14 -0400 Subject: [PATCH 6/7] PR feedback: Do not overwrite self.shouldstop, remove duplicate node removal using for/else logic --- src/xdist/dsession.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index 0a49d696..bae92795 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -182,18 +182,17 @@ def worker_workerfinished(self, node): self.shouldstop = f"{node} received keyboard-interrupt" self.worker_errordown(node, "keyboard-interrupt") return - shouldfail = node.workeroutput["shouldfail"] shouldstop = node.workeroutput["shouldstop"] for shouldx in [shouldfail, shouldstop]: if shouldx: - self.shouldstop = shouldx - self._active_nodes.remove(node) - return - - if node in self.sched.nodes: - crashitem = self.sched.remove_node(node) - assert not crashitem, (crashitem, node) + if not self.shouldstop: + self.shouldstop = shouldx + break + else: + if node in self.sched.nodes: + crashitem = self.sched.remove_node(node) + assert not crashitem, (crashitem, node) self._active_nodes.remove(node) def worker_internal_error(self, node, formatted_error): From ae1553bf0571eb186fb9bc13f9e61387a499fad2 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Mon, 1 Apr 2024 10:46:34 -0400 Subject: [PATCH 7/7] Add changelog entry --- changelog/1024.bugfix | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog/1024.bugfix diff --git a/changelog/1024.bugfix b/changelog/1024.bugfix new file mode 100644 index 00000000..06ce77f4 --- /dev/null +++ b/changelog/1024.bugfix @@ -0,0 +1,2 @@ +Added proper handling of ``shouldstop`` (such as set by ``--max-fail``) and ``shouldfail`` conditions in workers. +Previously, a worker might have continued executing further tests before the controller could terminate the session.