From ace9b016a7549fc3e6ec15b03fc43b5cb8922850 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 13 Oct 2026 00:43:15 +0000 Subject: [PATCH 1/2] test: an unguarded pyarrow import is refused under pytest -n too (#1305), red OffgridwithJD measured on #1286 that the refusal is lost under xdist: the serial run said REFUSED and exited 1, the -n 2 run said 2 passed and exited 0, because pytest_collect_file runs in the workers. A pytester arm runs both as subprocesses over a stub pyarrow and a file holding 'import pyarrow.parquet as pq'. Red on main: the serial premise holds, and under -n 2 the controller names nothing (got 0 want 1). guard_tests 413 -> 414 by collection. Co-Authored-By: Claude Opus 5.5 --- test/pytest/TESTS.md | 1 + test/pytest/expected_tests.txt | 3 ++- test/pytest/test_capability_decline.py | 26 ++++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 14de60719..da7568552 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -6486,6 +6486,7 @@ Written red first: all seven failed before `pgc_vacuity` had the methods. The fo | `test_a_broken_install_fails_rather_than_declines` | a `pyarrow` that raises `ImportError` from inside fails with its own error | | `test_a_missing_dependency_of_pyarrow_fails_rather_than_declines` | a `pyarrow` that cannot import its own dependency raises `ModuleNotFoundError` naming that dependency, and fails rather than declines; shown red with `require_module`'s `.name` check deleted (OffgridwithJD, review of #1286) | | `test_every_unguarded_import_spelling_is_refused` | each of six spellings, `__import__` among them, is named and refused; a file using `require_module` is not | +| `test_an_unguarded_import_is_refused_under_xdist_too` | under `-n 2` the controller names the unguarded import once and the run is refused, as the serial run is: workers ship their sites back through `workeroutput` (#1305) | ## 92. test_tid_range_scan.py: a ctid range reads only the row groups that hold it diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index e629d47cf..424977aa7 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -267,7 +267,8 @@ # NO_CLUSTER. Re-derived by collection on 73d9663: `412 tests collected`. # guard_tests 412 -> 413 for test_a_missing_dependency_of_pyarrow_fails_rather_than_declines # (#1286 review), collected over the NO_CLUSTER files on this tree. -guard_tests 413 +# guard_tests 413 -> 414 for #1305's test_an_unguarded_import_is_refused_under_xdist_too, by collection. +guard_tests 414 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and diff --git a/test/pytest/test_capability_decline.py b/test/pytest/test_capability_decline.py index d7ba9aa1c..678b8ef50 100644 --- a/test/pytest/test_capability_decline.py +++ b/test/pytest/test_capability_decline.py @@ -174,3 +174,29 @@ def test_guarded(expect): expect.num(1 if result.ret != 0 else 0, 1, "and a run carrying them is refused") expect.num(out.count("test_guarded.py imports pyarrow"), 0, "while a file that goes through require_module is not named") + + +def test_an_unguarded_import_is_refused_under_xdist_too(pytester, monkeypatch, expect): + """The refusal reaches the controller when workers do the collecting (#1305). + + Under `-n`, pytest_collect_file runs in the workers, so the sites they found + never reached the controller, which prints the summary and sets the exit status: + measured by @OffgridwithJD, the serial run said REFUSED and exited 1, the `-n 2` + run said `2 passed` and exited 0. A subprocess, because xdist's workers are. + """ + import pathlib + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pkg = pytester.mkpydir("pyarrow") + (pkg / "parquet.py").write_text("") + here = pathlib.Path(__file__).resolve().parent + monkeypatch.setenv("PYTHONPATH", f"{pytester.path}:{here}") + pytester.makepyfile(test_bad="import pyarrow.parquet as pq\n\ndef test_x(expect):\n expect.num(1, 1, 'x')\n", + test_ok="def test_y(expect):\n expect.num(1, 1, 'y')\n") + serial = pytester.runpytest_subprocess("-p", "pgc_vacuity", "-p", "no:cacheprovider") + xdist = pytester.runpytest_subprocess("-p", "pgc_vacuity", "-p", "no:cacheprovider", "-n", "2") + expect.num(1 if serial.ret != 0 and "REFUSED test_bad.py" in serial.stdout.str() else 0, 1, + "premise: the serial run refuses the unguarded import") + out = xdist.stdout.str() + expect.num(out.count("REFUSED test_bad.py imports pyarrow outside require_module"), 1, + "under -n 2 the controller names the unguarded import, once") + expect.num(1 if xdist.ret != 0 else 0, 1, "and the -n 2 run is refused, not green") From d29ade6c96fa1997b23c0b3a830f8ff941ebe915 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 13 Oct 2026 00:43:15 +0000 Subject: [PATCH 2/2] fix: workers ship their unguarded-import sites to the xdist controller (#1305) A worker records the sites it collected in workeroutput at session end; the controller merges them in pytest_testnodedown, keeping the first of each (every worker collects every file), before its own sessionfinish sets the exit status and its terminal summary prints the REFUSED lines. The hook is optionalhook, so a run without xdist loads nothing new. test_capability_decline.py 9 passed; the guard half 413 passed, 1 failed (test_build_refusal, which fails identically on unmodified main here). Co-Authored-By: Claude Opus 5.5 --- test/pytest/pgc_vacuity.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index cd038a55a..91499d506 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1554,7 +1554,16 @@ def pytest_sessionfinish(session, exitstatus): # A test file that imports a declinable capability outside require_module is # refused (#1286 review): it would fail on a box without the module, or pass # having caught its own ImportError, and either way the decline is not named. - if getattr(session.config, "pgc_capability_imports", None) and session.exitstatus == 0: + # + # UNDER XDIST THE WORKERS COLLECT, so the sites they found are shipped back + # through workeroutput and merged on the controller in pytest_testnodedown + # (#1305). Measured before that: the serial run said REFUSED and exited 1, the + # -n 2 run said `2 passed` and exited 0. + if hasattr(session.config, "workerinput"): + sites = getattr(session.config, "pgc_capability_imports", None) + if sites: + session.config.workeroutput["pgc_capability_imports"] = list(sites) + elif getattr(session.config, "pgc_capability_imports", None) and session.exitstatus == 0: session.exitstatus = 1 # THE RECONCILIATION, FIRST, because it is a statement about whether the run @@ -2027,6 +2036,26 @@ def _capability_import_sites(path, capabilities=DECLINABLE_CAPABILITIES): return sites +@pytest.hookimpl(optionalhook=True) +def pytest_testnodedown(node, error): + """Merge a worker's unguarded-import sites into the controller's list (#1305). + + Every worker collects every file, so each site arrives once per worker; the + list keeps the first of each. Optional, because the hook exists only when + xdist is loaded. + """ + sites = getattr(node, "workeroutput", {}).get("pgc_capability_imports") or [] + if not sites: + return + store = getattr(node.config, "pgc_capability_imports", None) + if store is None: + store = [] + node.config.pgc_capability_imports = store + for site in sites: + if site not in store: + store.append(site) + + def pytest_collect_file(file_path, parent): """Scan each test file for direct imports of a declinable capability.