From 1b45d947706a039504eee90bc37d40ab76102358 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 26 Jul 2026 06:36:38 +0200 Subject: [PATCH 1/3] Re-raise what concurrency test workers throw run_in_threads only let a worker's exception print to stderr: join() still succeeded and the worker's slot stayed None, so a test could carry on with None results. Some assertions catch that by accident (len({id(x) for x in results}) == 1 is satisfied by four Nones), and the ones that do fail report an AttributeError on None rather than the real error. Workers now record what they raise and the helper re-raises it after joining, grouping them when more than one fails. Adds tests for the helper itself, since a fault in it weakens every concurrency test silently. Reported by CodeRabbit on #779 after that PR merged. --- tests/conftest.py | 15 +++++++++++--- tests/test_conftest.py | 47 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/test_conftest.py diff --git a/tests/conftest.py b/tests/conftest.py index edfcedba3..b1f1c5018 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -117,16 +117,21 @@ def run_in_threads(): A barrier releases every thread together, so concurrency tests do not need sleeps. The timeouts turn a deadlock into a failure rather than a - hung test run. + hung test run, and anything a worker raises is re-raised here rather + than being printed while the test carries on with a None result. """ def _run(func, count=4, timeout=60): barrier = threading.Barrier(count, timeout=timeout) results = [None] * count + errors = [] def worker(index): - barrier.wait() - results[index] = func(index) + try: + barrier.wait() + results[index] = func(index) + except BaseException as error: # re-raised in the calling thread + errors.append(error) threads = [threading.Thread(target=worker, args=(i,)) for i in range(count)] for thread in threads: @@ -134,6 +139,10 @@ def worker(index): for thread in threads: thread.join(timeout) assert not thread.is_alive(), "thread never finished; possible deadlock" + if len(errors) == 1: + raise errors[0] + if errors: + raise ExceptionGroup("workers raised", errors) return results return _run diff --git a/tests/test_conftest.py b/tests/test_conftest.py new file mode 100644 index 000000000..336867ea4 --- /dev/null +++ b/tests/test_conftest.py @@ -0,0 +1,47 @@ +"""Tests for the shared test helpers defined in conftest.""" + +from __future__ import annotations + +import threading + +import pytest + + +class TestRunInThreads: + """The concurrency helper must not hide what its workers do.""" + + def test_returns_each_worker_result(self, run_in_threads): + """Each worker's return value comes back in index order.""" + results = run_in_threads(lambda index: index * 2, count=3) + assert results == [0, 2, 4] + + def test_worker_exception_propagates(self, run_in_threads): + """A raising worker fails the test instead of leaving a None result.""" + + def _raise_on_first(index): + if index == 0: + raise ValueError("worker failed") + return index + + with pytest.raises(ValueError, match="worker failed"): + run_in_threads(_raise_on_first, count=2) + + def test_several_worker_exceptions_are_grouped(self, run_in_threads): + """When more than one worker fails, none of them is dropped.""" + + def _always_raise(index): + raise ValueError(f"worker {index} failed") + + with pytest.raises(ExceptionGroup) as exc_info: + run_in_threads(_always_raise, count=3) + assert len(exc_info.value.exceptions) == 3 + + def test_workers_run_concurrently(self, run_in_threads): + """The barrier releases every worker together. + + A worker waiting on a barrier the others cannot reach would time + out, so passing it means they really did overlap. + """ + barrier = threading.Barrier(3, timeout=30) + results = run_in_threads(lambda index: barrier.wait() is not None, count=3) + assert all(results) From 875f15d58271d15fc631e7544b718fbaf3506549 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 26 Jul 2026 19:30:48 +0200 Subject: [PATCH 2/3] Re-raise the first worker failure, not an ExceptionGroup Two problems with aggregating, both raised by codex: ExceptionGroup is 3.11+, and pyproject declares requires-python >=3.10, so the aggregating branch would have been a NameError on the oldest supported interpreter (CI only runs 3.11+, so nothing would have caught it). It also rejects BaseException members. The worker deliberately catches BaseException, so two workers using pytest.skip or pytest.fail would have produced "TypeError: Cannot nest BaseExceptions in an ExceptionGroup" in place of the outcomes it promised to propagate. Re-raising the first failure is 3.10-safe, handles BaseException, and is simpler. Later failures are dropped, which is the normal contract for a test helper. Adds a test for the BaseException case. --- tests/conftest.py | 7 ++++--- tests/test_conftest.py | 20 ++++++++++++++++---- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b1f1c5018..b755d3984 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -139,10 +139,11 @@ def worker(index): for thread in threads: thread.join(timeout) assert not thread.is_alive(), "thread never finished; possible deadlock" - if len(errors) == 1: - raise errors[0] if errors: - raise ExceptionGroup("workers raised", errors) + # The first failure is re-raised as-is rather than aggregated: + # ExceptionGroup is 3.11+ while the package supports 3.10, and it + # rejects BaseException members such as pytest's own skip/fail. + raise errors[0] return results return _run diff --git a/tests/test_conftest.py b/tests/test_conftest.py index 336867ea4..cfb4c6643 100644 --- a/tests/test_conftest.py +++ b/tests/test_conftest.py @@ -26,15 +26,27 @@ def _raise_on_first(index): with pytest.raises(ValueError, match="worker failed"): run_in_threads(_raise_on_first, count=2) - def test_several_worker_exceptions_are_grouped(self, run_in_threads): - """When more than one worker fails, none of them is dropped.""" + def test_several_worker_exceptions_surface_one(self, run_in_threads): + """When several workers fail, one real failure reaches the test.""" def _always_raise(index): raise ValueError(f"worker {index} failed") - with pytest.raises(ExceptionGroup) as exc_info: + with pytest.raises(ValueError, match="worker . failed"): run_in_threads(_always_raise, count=3) - assert len(exc_info.value.exceptions) == 3 + + def test_base_exception_from_worker_propagates(self, run_in_threads): + """A BaseException-derived outcome, such as pytest.skip, is not swallowed. + + These cannot be members of an ExceptionGroup, so aggregating them + would replace the outcome with a TypeError. + """ + + def _skip(index): + pytest.fail("worker used a BaseException-derived outcome") + + with pytest.raises(BaseException, match="BaseException-derived"): + run_in_threads(_skip, count=2) def test_workers_run_concurrently(self, run_in_threads): """The barrier releases every worker together. From 73048935115ec96d93a27015c6babf51c842ef37 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 26 Jul 2026 20:26:47 +0200 Subject: [PATCH 3/3] Drop the tests for the test helper Testing the test helper is more machinery than the helper warrants; the concurrency tests that use it already exercise it. --- tests/test_conftest.py | 59 ------------------------------------------ 1 file changed, 59 deletions(-) delete mode 100644 tests/test_conftest.py diff --git a/tests/test_conftest.py b/tests/test_conftest.py deleted file mode 100644 index cfb4c6643..000000000 --- a/tests/test_conftest.py +++ /dev/null @@ -1,59 +0,0 @@ -"""Tests for the shared test helpers defined in conftest.""" - -from __future__ import annotations - -import threading - -import pytest - - -class TestRunInThreads: - """The concurrency helper must not hide what its workers do.""" - - def test_returns_each_worker_result(self, run_in_threads): - """Each worker's return value comes back in index order.""" - results = run_in_threads(lambda index: index * 2, count=3) - assert results == [0, 2, 4] - - def test_worker_exception_propagates(self, run_in_threads): - """A raising worker fails the test instead of leaving a None result.""" - - def _raise_on_first(index): - if index == 0: - raise ValueError("worker failed") - return index - - with pytest.raises(ValueError, match="worker failed"): - run_in_threads(_raise_on_first, count=2) - - def test_several_worker_exceptions_surface_one(self, run_in_threads): - """When several workers fail, one real failure reaches the test.""" - - def _always_raise(index): - raise ValueError(f"worker {index} failed") - - with pytest.raises(ValueError, match="worker . failed"): - run_in_threads(_always_raise, count=3) - - def test_base_exception_from_worker_propagates(self, run_in_threads): - """A BaseException-derived outcome, such as pytest.skip, is not swallowed. - - These cannot be members of an ExceptionGroup, so aggregating them - would replace the outcome with a TypeError. - """ - - def _skip(index): - pytest.fail("worker used a BaseException-derived outcome") - - with pytest.raises(BaseException, match="BaseException-derived"): - run_in_threads(_skip, count=2) - - def test_workers_run_concurrently(self, run_in_threads): - """The barrier releases every worker together. - - A worker waiting on a barrier the others cannot reach would time - out, so passing it means they really did overlap. - """ - barrier = threading.Barrier(3, timeout=30) - results = run_in_threads(lambda index: barrier.wait() is not None, count=3) - assert all(results)