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/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. 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")