From 2007c457d392067e9ac23e649ee909a48fe4a1b5 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 18:12:31 +0000 Subject: [PATCH 1/6] fix: a missing pyarrow declines a suite instead of failing it (#1215) The 25 suites that call pgc_skip pyarrow now record a named SKIP and exit 66 when pyarrow is absent, so a box without it no longer reddens the run. Owner's decision, 2026-10-12. Every other capability keeps the old rule: a missing btree_gist still fails unless it is waived. Test first: selftest part 600 drives pgc_skip in throwaway suites with every waiver cleared. On the old lib.sh its two pyarrow arms fail for the intended reason (got [1 FAILED] want [66 SKIPPED]; got [0/1] want [1/0]) while both btree_gist controls pass. After the change the selftest is 1218 passed + 0 failed on PG17. End to end, objstore_listing.sh with an ImportError shim first on PYTHONPATH: rc=66, one SKIP record, 0 failed. docs/testing.md said a skipped suite exits 2. PGC_EXIT_SKIPPED is 66 and both runs above measured 66, so the page now says 66. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 +++ docs/testing.md | 21 ++++--- test/lib.sh | 20 ++++-- .../600-a-missing-pyarrow-declines.sh | 61 +++++++++++++++++++ test/selftest/parts.manifest | 1 + 5 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 test/selftest/600-a-missing-pyarrow-declines.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 85a87d7a8..f919c73ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -164,6 +164,14 @@ true until the next version shipped. release notes' repair procedure, reading its query from the notes, and a docs check refuses any documented query that tests `pg_extension` while listing databases. +- Without pyarrow, the 25 Arrow and Parquet suites that call `pgc_skip pyarrow` + now report `SKIPPED` and exit 66, and the run does not fail (#1215). Before, + each reported one `FAIL` and reddened the whole run on a box without pyarrow. + The owner decided this on 2026-10-12. Every other missing dependency, such as + `btree_gist` for `temporal.sh`, still fails unless it is waived. The nightly + workflow installs pyarrow and stops if the suites cannot import it, so CI + does not lose these suites silently. `docs/testing.md` also said a skipped + suite exits 2; it exits 66, and the page now says so. - `test/pytest/README.md` now names all four registries a new pytest test has to be entered in, and which of them a local run can catch. Adding one test reddens diff --git a/docs/testing.md b/docs/testing.md index 32d9f1fea..ab9f5c0af 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -317,14 +317,14 @@ you got before deciding whether something is wrong. | outcome | exit | when | | --- | :-: | --- | | `PASSED` | 0 | the suite ran its checks and they held | -| `SKIPPED (ran no checks)` | 2 | the feature under test does not exist on this major | +| `SKIPPED (ran no checks)` | 66 | the feature under test does not exist on this major, or pyarrow is not installed | | `FAILED` | 1 | a check failed, **or** a tool the suite measures with is missing | The matrix prints `suites that ran: n of m (skipped: k)` for each major, and names the skipped suites. A skip counts in `k`, not in `n`. -**A missing dependency fails rather than skips, and that is deliberate.** The box -cannot gate that suite. Reporting a pass would claim coverage nobody has. Two +**A missing dependency fails rather than skips, except pyarrow.** The box cannot +gate that suite, and reporting a pass would claim coverage nobody has. Two dependencies come from outside this repository: | suite | needs | where it comes from | @@ -332,15 +332,22 @@ dependencies come from outside this repository: | the Arrow and Parquet suites | `pyarrow` | `pip install pyarrow`, as the user the suites run as | | `temporal.sh` | `btree_gist` | PostgreSQL **contrib**. A PGDG package ships it; a source build configured without contrib does not | +**Without pyarrow, the Arrow and Parquet suites report `SKIPPED` and the run does +not fail** (#1215). A box without pyarrow is a supported developer configuration. +The matrix still names each skipped suite. The nightly workflow installs pyarrow +and stops if the suites' interpreter cannot import it, so CI does not lose these +suites silently. + Build and install `contrib/btree_gist` against each source tree when the matrix -runs against source builds. To run knowingly without a dependency, waive it: +runs against source builds. To run knowingly without it, or without any other +dependency, waive it: ```sh PGC_ALLOW_MISSING_BTREE_GIST=1 test/temporal.sh /path/to/pg_config PGC_ALLOW_MISSING=1 test/run_all_versions.sh ``` -A waived suite reports `SKIPPED` and exits 2, so the coverage loss stays visible +A waived suite reports `SKIPPED` and exits 66, so the coverage loss stays visible in the skipped count rather than becoming a pass. Measured on PostgreSQL 18, one suite, four configurations: @@ -349,8 +356,8 @@ Measured on PostgreSQL 18, one suite, four configurations: | --- | :-: | --- | | `btree_gist` present | 0 | `PASSED`, 5 checks | | `btree_gist` absent | 1 | `FAILED` | -| `btree_gist` absent, waived | 2 | `SKIPPED` | -| PostgreSQL 17, feature absent | 2 | `SKIPPED (ran no checks)` | +| `btree_gist` absent, waived | 66 | `SKIPPED` | +| PostgreSQL 17, feature absent | 66 | `SKIPPED (ran no checks)` | ## Cross-major upgrade diff --git a/test/lib.sh b/test/lib.sh index 52c33324f..b0b613db5 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -1971,17 +1971,29 @@ chunk_group_count() { # deliberately not the same code path. PostgreSQL 15 has no WITHOUT OVERLAPS to # test and no amount of installing will give it one, so those gates call # pgc_summary directly and report SKIPPED. Nothing is broken there. Here it is. +# +# EXCEPT PYARROW, which declines by default (#1215, owner's decision 2026-10-12). +# A box without pyarrow is a supported developer configuration, and the 25 suites +# that need it were the reddest thing such a box produced. The nightly installs +# pyarrow and stops when the suites' interpreter cannot import it, so CI cannot +# lose them silently; the run summary still names every suite that skipped. +# Every other capability keeps the rule above. pgc_skip() { # pgc_skip - local cap allow_one + local cap allow_one why cap="$(printf '%s' "$1" | tr '[:lower:]-' '[:upper:]_')" allow_one="PGC_ALLOW_MISSING_$cap" - if [ "${PGC_ALLOW_MISSING:-0}" = 1 ] || [ "${!allow_one:-0}" = 1 ]; then + why="" + if [ "$cap" = PYARROW ]; then + why="pyarrow is optional on a developer box, see #1215" + elif [ "${PGC_ALLOW_MISSING:-0}" = 1 ] || [ "${!allow_one:-0}" = 1 ]; then + why="waived by $allow_one or PGC_ALLOW_MISSING" + fi + if [ -n "$why" ]; then # The unwaived branch below records a FAIL. This one printed and left # PGC_CHECKS at zero, so waiving a dependency also erased the outcome -- # the same asymmetry check_timing had, in the function whose whole # subject is "a missing dependency is not a pass". - check_skip "$2" "SKIP $2 (waived by $allow_one or PGC_ALLOW_MISSING)" \ - "waived by $allow_one or PGC_ALLOW_MISSING" + check_skip "$2" "SKIP $2 ($why)" "$why" pgc_summary fi pgc_record FAIL "$2" "FAIL $2" diff --git a/test/selftest/600-a-missing-pyarrow-declines.sh b/test/selftest/600-a-missing-pyarrow-declines.sh new file mode 100644 index 000000000..07e09cba1 --- /dev/null +++ b/test/selftest/600-a-missing-pyarrow-declines.sh @@ -0,0 +1,61 @@ +# ---- a missing pyarrow declines a suite; other dependencies still fail ----- +# +# THE OWNER'S DECISION, 2026-10-12 (#1215). When pyarrow is absent, the 25 suites +# that call `pgc_skip pyarrow` report SKIPPED and exit 66, and the run does not +# fail. Every other missing dependency still FAILS unless it is waived, because +# the decision was about pyarrow only: `temporal.sh` without btree_gist is still +# a box that cannot gate that suite. +# +# Why a skip is safe for pyarrow and was not before: the nightly installs pyarrow +# and refuses to continue when the suites' interpreter cannot import it, so CI +# cannot lose these suites silently. A developer box without pyarrow is a +# supported configuration, and the run summary still names every skipped suite. +# +# The suites here source lib.sh and call pgc_skip without pgc_setup, so no +# cluster starts. Each differs from the others in one respect. + +_mpd_lib="$PGC_TESTDIR/lib.sh" +_mpd_dir="$PGC_WORKDIR/missing_pyarrow"; mkdir -p "$_mpd_dir" + +check "premise: lib.sh defines pgc_skip" \ + "$(grep -c '^pgc_skip()' "$_mpd_lib")" "1" + +_mpd_make() { # _mpd_make NAME BODY + { + printf '#!/usr/bin/env bash\n' + printf '. "%s"\n' "$_mpd_lib" + printf 'PGC_SUITE=%s\n' "$1" + printf '%s\n' "$2" + } > "$_mpd_dir/$1.sh" + chmod 755 "$_mpd_dir/$1.sh" +} + +# Run with every waiver cleared, so the caller's environment cannot decide the +# answer. `env -u` on a name that is not set is a no-op. +_mpd_run() { # _mpd_run NAME [VAR=VALUE ...] -> " " + local name="$1" out rc; shift + out="$(env -u PGC_ALLOW_MISSING -u PGC_ALLOW_MISSING_PYARROW \ + -u PGC_ALLOW_MISSING_BTREE_GIST "$@" bash "$_mpd_dir/$name.sh" 2>&1)"; rc=$? + printf '%s %s' "$rc" "$(printf '%s' "$out" | grep -oE '(PASSED|FAILED|INCOMPLETE|SKIPPED)' | tail -1)" +} + +_mpd_records() { # _mpd_records NAME VERDICT -> how many RESULT records carry VERDICT + env -u PGC_ALLOW_MISSING -u PGC_ALLOW_MISSING_PYARROW \ + -u PGC_ALLOW_MISSING_BTREE_GIST bash "$_mpd_dir/$1.sh" 2>&1 | + awk -F'\t' -v v="$2" '$1 == "RESULT" && $5 == v' | wc -l | tr -d ' ' +} + +_mpd_make nopyarrow 'pgc_skip pyarrow "pyarrow is required to write the fixture"' +_mpd_make nogist 'pgc_skip btree_gist "btree_gist is required for WITHOUT OVERLAPS"' + +check "a missing pyarrow declines the suite instead of failing it" \ + "$(_mpd_run nopyarrow)" "66 SKIPPED" +check "and it records a skip under the check's own name, not a failure" \ + "$(_mpd_records nopyarrow SKIP)/$(_mpd_records nopyarrow FAIL)" "1/0" + +check "control: a missing btree_gist still fails the suite" \ + "$(_mpd_run nogist)" "1 FAILED" +check "control: and an explicit waiver still turns that failure into a skip" \ + "$(_mpd_run nogist PGC_ALLOW_MISSING_BTREE_GIST=1)" "66 SKIPPED" + +rm -rf "$_mpd_dir" diff --git a/test/selftest/parts.manifest b/test/selftest/parts.manifest index 5881cf592..906d942e2 100644 --- a/test/selftest/parts.manifest +++ b/test/selftest/parts.manifest @@ -56,3 +56,4 @@ 570-a-checkout-test-must-ask-git.sh 580-a-hand-rebuild-must-record.sh 590-a-declared-suite-must-account.sh +600-a-missing-pyarrow-declines.sh From 2de72d449b251217090e18567671d6444d23eab3 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 18:45:51 +0000 Subject: [PATCH 2/6] fix: the capability sweep counts a declined suite as declined, not silent (#1215) Review of #1286 (OffgridwithJD): at b1f6cad the nightly capability_sweep went red, preserved=5 refused=0 silent=25 of 30, because its verdict had no outcome for a suite that loses its names and says so with a SKIP. And lib.sh joined the sweep's wide population, because the decline reason named pyarrow in code. - pgc_capability_verdict takes a third input, the SKIP records whose reason names the capability, and returns declined for them. silent, names lost with no FAIL and no SKIP, must still be zero: the owner's rule is that a capability may be skipped but no test may be silent. - The sweep asserts that the suites that decline are exactly the suites whose code calls pgc_skip pyarrow, as sets derived from the source (25 today), not a literal count. - The verdict's four cases move to selftest part 610, which evals the sweep's own text, so they run in seconds on every major instead of needing every pyarrow suite twice. Red on the old function: (3 lost, 0 fails, 1 skip) -> silent, want declined. - lib.sh builds the reason from $1, so its code no longer names pyarrow: code lines naming it, main 0, b1f6cad 1, this head 0. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++ test/capability_sweep.sh | 50 ++++++++++++++++--- test/lib.sh | 2 +- ...610-a-declined-capability-is-not-silent.sh | 27 ++++++++++ test/selftest/parts.manifest | 1 + 5 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 test/selftest/610-a-declined-capability-is-not-silent.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index f919c73ca..eee5185c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -172,6 +172,11 @@ true until the next version shipped. workflow installs pyarrow and stops if the suites cannot import it, so CI does not lose these suites silently. `docs/testing.md` also said a skipped suite exits 2; it exits 66, and the page now says so. + The nightly `capability_sweep.sh` counts these suites as `declined`, a fourth + outcome beside `preserved`, `refused` and `silent`. A suite is `declined` only + if it records a SKIP naming the capability, and `silent` must still be zero. + The sweep now also asserts that the suites that decline are exactly the ones + whose code calls `pgc_skip pyarrow`. - `test/pytest/README.md` now names all four registries a new pytest test has to be entered in, and which of them a local run can catch. Adding one test reddens diff --git a/test/capability_sweep.sh b/test/capability_sweep.sh index 33e69f1d8..09eeed04c 100755 --- a/test/capability_sweep.sh +++ b/test/capability_sweep.sh @@ -160,22 +160,37 @@ check_num "premise: and pyarrow IS importable without the shim, so absence is th # ---- the verdict, a pure function so the selftest can drive it ------------- # -# Two inputs and three outcomes. It takes the numbers rather than the logs so it +# Three inputs and four outcomes. It takes the numbers rather than the logs so it # can be exercised without running a suite, the same reason pgc_freshness_verdict # and pgc_build_needs_clean take theirs. -pgc_capability_verdict() { # pgc_capability_verdict LOST_NAMES FAIL_RECORDS -> verdict - local lost="${1:-}" fails="${2:-}" +# +# DECLINED IS ITS OWN OUTCOME (#1215, the owner's rule of 2026-10-12): a missing +# capability may be bypassed as a SKIP, but no test may be silent. A suite that +# loses its names and records a SKIP naming the capability has said what it did +# not run; a suite that loses them and records nothing has not. Folding the first +# into `silent` reddened the sweep on the 25 suites the rule allows to skip, and +# folding it into `preserved` would hide them. So the two stay apart, and `silent` +# must still be zero. +pgc_capability_verdict() { # pgc_capability_verdict LOST FAILS NAMED_SKIPS -> verdict + local lost="${1:-}" fails="${2:-}" skips="${3:-}" case "$lost" in ''|*[!0-9]*) echo unknown; return ;; esac case "$fails" in ''|*[!0-9]*) echo unknown; return ;; esac + case "$skips" in ''|*[!0-9]*) echo unknown; return ;; esac if [ "$lost" -eq 0 ]; then echo preserved elif [ "$fails" -ge 1 ]; then echo refused + elif [ "$skips" -ge 1 ]; then + echo declined else echo silent fi } +# Its four cases are asserted by selftest part 610, which evals this text: they +# need no suite run, and a sweep built on a verdict that misfiled one case would +# count it wrongly thirty times over and still reconcile. + cap_names() { # cap_names LOGFILE -> the recorded check names, sorted grep -oP '^RESULT\t[^\t]*\t[^\t]*\t\K[^\t]*' "$1" 2>/dev/null | LC_ALL=C sort -u } @@ -195,8 +210,10 @@ check_num "premise: the priming run built and installed cleanly" "$prime_rc" "0" n_preserved=0 n_refused=0 +n_declined=0 n_silent=0 silent_names="" +declined_names="" mask_failures=0 for s in $NARROW; do @@ -211,22 +228,43 @@ for s in $NARROW; do [ "$(cap_rc "$SHIM" 'import pyarrow')" = 1 ] || mask_failures=$((mask_failures + 1)) lost="$(LC_ALL=C comm -23 <(cap_names "$w_log") <(cap_names "$o_log") | grep -c .)" fails="$(grep -cP '^RESULT\t.*\tFAIL\t' "$o_log")" - case "$(pgc_capability_verdict "$lost" "$fails")" in + # A SKIP COUNTS ONLY IF ITS REASON NAMES THE CAPABILITY, read from the record + # rather than from the exit code: exit 66 also means a major without the + # feature, which is not this. + skips="$(awk -F'\t' '$1 == "RESULT" && $5 == "SKIP" && $7 ~ /pyarrow/' "$o_log" | grep -c .)" + case "$(pgc_capability_verdict "$lost" "$fails" "$skips")" in preserved) n_preserved=$((n_preserved + 1)) ;; refused) n_refused=$((n_refused + 1)) ;; + declined) n_declined=$((n_declined + 1)); declined_names="$declined_names $s" ;; *) n_silent=$((n_silent + 1)); silent_names="$silent_names $stem" ;; esac rm -f "$w_log" "$o_log" done -echo "-- preserved=$n_preserved refused=$n_refused silent=$n_silent of $n_narrow" +echo "-- preserved=$n_preserved refused=$n_refused declined=$n_declined silent=$n_silent of $n_narrow" [ -n "$silent_names" ] && echo "-- silent:$silent_names" check_num "premise: the mask held for every run in the sweep" "$mask_failures" "0" check_num "premise: every suite in the population was classified" \ - "$((n_preserved + n_refused + n_silent))" "$n_narrow" + "$((n_preserved + n_refused + n_declined + n_silent))" "$n_narrow" check_num "no suite loses a check without recording a skip or a refusal" \ "$n_silent" "0" +# THE SUITES THAT DECLINE ARE THE SUITES THAT ASK TO, derived from the source and +# compared as SETS. A count would let a suite that stopped declining trade places +# with one that started; a literal 25 would go stale the day a suite is added. A +# suite that stops calling pgc_skip pyarrow leaves this side, and must then read +# preserved, or the silent arm above catches it. +DECLINERS="$(for _cap_b in $NARROW; do + sed 's/#.*//' "$TESTDIR/$_cap_b" | grep -qE 'pgc_skip[[:space:]]+"?pyarrow' && printf '%s\n' "$_cap_b" +done | LC_ALL=C sort)" +check_num "premise: some suites in the population call pgc_skip pyarrow" \ + "$([ -n "$DECLINERS" ] && echo 1 || echo 0)" "1" +_cap_diff="$(LC_ALL=C comm -3 <(printf '%s\n' "$DECLINERS") \ + <(printf '%s\n' $declined_names | LC_ALL=C sort) | tr -d '\t' | grep .)" +[ -n "$_cap_diff" ] && echo "-- decline mismatch: $(printf '%s ' $_cap_diff)" +check_num "the suites that decline are exactly the suites that call pgc_skip pyarrow" \ + "$(printf '%s\n' "$_cap_diff" | grep -c .)" "0" + rm -rf "$SHIM" "$prime_log" pgc_summary diff --git a/test/lib.sh b/test/lib.sh index b0b613db5..703a67c72 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -1984,7 +1984,7 @@ pgc_skip() { # pgc_skip allow_one="PGC_ALLOW_MISSING_$cap" why="" if [ "$cap" = PYARROW ]; then - why="pyarrow is optional on a developer box, see #1215" + why="$1 is optional on a developer box, see #1215" elif [ "${PGC_ALLOW_MISSING:-0}" = 1 ] || [ "${!allow_one:-0}" = 1 ]; then why="waived by $allow_one or PGC_ALLOW_MISSING" fi diff --git a/test/selftest/610-a-declined-capability-is-not-silent.sh b/test/selftest/610-a-declined-capability-is-not-silent.sh new file mode 100644 index 000000000..72ead5d4e --- /dev/null +++ b/test/selftest/610-a-declined-capability-is-not-silent.sh @@ -0,0 +1,27 @@ +# ---- a declined capability is not a silent one ------------------------------ +# +# capability_sweep.sh classifies each pyarrow suite by what it lost when pyarrow +# was masked. #1215 made 25 of them decline with a SKIP, and the owner's rule of +# 2026-10-12 is that a missing capability may be skipped but no test may be +# silent. So the verdict has four outcomes and the sweep must keep the last two +# apart. The sweep runs nightly and needs every pyarrow suite twice; the verdict +# is a pure function, so its cases are asserted here, from the sweep's own text. + +_dcs_src="$PGC_TESTDIR/capability_sweep.sh" +_dcs_fn="$(sed -n '/^pgc_capability_verdict() {/,/^}/p' "$_dcs_src")" + +check_num "premise: the sweep defines its verdict where this part reads it" \ + "$(printf '%s\n' "$_dcs_fn" | grep -c '^pgc_capability_verdict() {')" "1" + +eval "$_dcs_fn" + +check_text "the verdict: no name lost is preserved" \ + "$(pgc_capability_verdict 0 0 0)" "preserved" +check_text "the verdict: names lost beside a failure are refused" \ + "$(pgc_capability_verdict 3 1 0)" "refused" +check_text "the verdict: names lost beside a skip naming the capability are declined" \ + "$(pgc_capability_verdict 3 0 1)" "declined" +check_text "the verdict: names lost with neither a failure nor a skip are silent" \ + "$(pgc_capability_verdict 3 0 0)" "silent" + +unset -f pgc_capability_verdict diff --git a/test/selftest/parts.manifest b/test/selftest/parts.manifest index 906d942e2..e1964777a 100644 --- a/test/selftest/parts.manifest +++ b/test/selftest/parts.manifest @@ -57,3 +57,4 @@ 580-a-hand-rebuild-must-record.sh 590-a-declared-suite-must-account.sh 600-a-missing-pyarrow-declines.sh +610-a-declined-capability-is-not-silent.sh From da7f834821d6ebc59ece82d018fb5deab3d0ac29 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 18:58:28 +0000 Subject: [PATCH 3/6] fix: the decline set reads with grep -c, not grep -q (#1215) The first run of the new set arm on PG17 failed with three suites that DO call pgc_skip pyarrow missing from the derived set. sed | grep -q under pipefail: grep -q exits at its first match, sed dies of SIGPIPE, and the pipeline fails, at random. The header of capability_sweep.sh records the same race for the population itself. grep -c reads all its input. PG17 sweep at this head: preserved=5 refused=0 declined=25 silent=0 of 30, 15 passed + 0 failed, 299 s. Co-Authored-By: Claude Opus 5.5 --- test/capability_sweep.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/capability_sweep.sh b/test/capability_sweep.sh index 09eeed04c..acbca66ec 100755 --- a/test/capability_sweep.sh +++ b/test/capability_sweep.sh @@ -256,7 +256,11 @@ check_num "no suite loses a check without recording a skip or a refusal" \ # suite that stops calling pgc_skip pyarrow leaves this side, and must then read # preserved, or the silent arm above catches it. DECLINERS="$(for _cap_b in $NARROW; do - sed 's/#.*//' "$TESTDIR/$_cap_b" | grep -qE 'pgc_skip[[:space:]]+"?pyarrow' && printf '%s\n' "$_cap_b" + # grep -c, NOT grep -q: -q exits at the first match, sed dies of SIGPIPE, and + # under pipefail the pipeline fails, so a suite that DOES call pgc_skip drops + # out at random. Measured here: three of 25 dropped on one run. The header of + # this file records the same race for the population itself. + [ "$(sed 's/#.*//' "$TESTDIR/$_cap_b" | grep -cE 'pgc_skip[[:space:]]+"?pyarrow')" -gt 0 ] && printf '%s\n' "$_cap_b" done | LC_ALL=C sort)" check_num "premise: some suites in the population call pgc_skip pyarrow" \ "$([ -n "$DECLINERS" ] && echo 1 || echo 0)" "1" From da3f5951817b5ed863a9d774b6f31d475fa3929e Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 20:45:16 +0000 Subject: [PATCH 4/6] test: a pytest test declines a missing pyarrow by name, and nothing declines silently (#1215) The pytest half of selftest parts 600 and 610, held by the Python harness's own machinery. The first version drove lib.sh from Python; OffgridwithJD's review refused it as crossing the harness boundary, and the owner chose a pytest mechanism matching the shell decision: a named skip, the run green, never silent. pgc_vacuity gains a fourth record verdict, DECLINED, and two methods: expect.declines(capability, *, name, detail='') expect.require_module(module, *, name) Five holes closed, from the review: 1 PGC_REQUIRE naming the capability turns a decline into a FAIL; the cluster pytest CI job installs pyarrow (pinned), proves it imports, and exports PGC_REQUIRE=pyarrow, so CI cannot decline. 2 a decline must be the test's first outcome, and no check may follow. 3 require_module declines only on ModuleNotFoundError naming the module or its top package; a broken install fails with its own error. 4 a test file importing pyarrow outside require_module is refused, in each of five spellings (AST, at file collection, before import). 5 the accounting line carries '+ N declined' when there are any, and each decline is printed with its node id and capability. A run with none prints the line every reader already parses, byte for byte. Test first: test_capability_decline.py's seven tests all failed before the methods existed; the four failure tests pin their MESSAGE, because a missing method also fails an inner test. The layer's own guards caught three mistakes of mine on the way: cannot_run lost its @_resolving to the new method, two refusals followed a record, and a second pytest_terminal_summary would have replaced the existing one. PG17: guard half 410 passed + 1 failed (test_build_refusal, also red on unmodified main); cluster half with PGC_REQUIRE=pyarrow 490 passed + 1 failed (test_harness_deps' driver-absent arm, also red on unmodified main here); harness_selftest 1223 passed + 0 failed. The new file passes 7/7 with and without PGC_REQUIRE exported. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 6 + test/pytest/TESTS.md | 27 ++++ test/pytest/compare_to_bash.py | 3 + test/pytest/pgc_vacuity.py | 164 +++++++++++++++++++++- test/pytest/requirements-test.txt | 1 + test/pytest/test_capability_decline.py | 152 ++++++++++++++++++++ test/pytest/test_failed_query_sentinel.py | 2 + test/pytest/test_harness_deps.py | 3 + 8 files changed, 356 insertions(+), 2 deletions(-) create mode 100644 test/pytest/test_capability_decline.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c53a74975..607b1a622 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -259,6 +259,12 @@ jobs: test -n "$WANT" echo "running $(set -- $FILES; echo $#) cluster file(s): $FILES" echo "expecting $WANT collected test(s)" + # PYARROW IS REQUIRED HERE (#1215). A ported pyarrow test declines green on a + # developer box without it; in CI that would be green forever, so CI proves + # the module imports and sets PGC_REQUIRE, which turns a decline into a + # failure (OffgridwithJD, review of #1286). + /tmp/pgcvenv/bin/python -c 'import pyarrow, pyarrow.parquet; print("pyarrow", pyarrow.__version__)' + export PGC_REQUIRE=pyarrow PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q \ --pg-config /usr/lib/postgresql/${{ matrix.pg }}/bin/pg_config \ --pgc-expect-tests "$WANT" \ diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 5d9ebc594..e13d4f14f 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -136,6 +136,7 @@ behaviour, the source of that number is named. - [88. test_storage_row_repair.py: the alpha5 release notes' repair procedure works as written](#88-test_storage_row_repairpy-the-alpha5-release-notes-repair-procedure-works-as-written) - [89. test_docs_cross_database_query.py: no documented query asks another database for its extensions](#89-test_docs_cross_database_querypy-no-documented-query-asks-another-database-for-its-extensions) - [90. test_partman_columnar.py: pg_partman makes columnar partitions with one setting](#90-test_partman_columnarpy-pg_partman-makes-columnar-partitions-with-one-setting) +- [91. test_capability_decline.py: a missing pyarrow declines by name, and nothing declines silently](#91-test_capability_declinepy-a-missing-pyarrow-declines-by-name-and-nothing-declines-silently) ## 1. How to read a test in here @@ -6455,3 +6456,29 @@ Removal proof, one mutation per arm, every major on the shell twin: the drop hoo | test | what it holds | | --- | --- | | `test_pg_partman_makes_columnar_partitions_with_one_setting` | pg_partman installed; without the setting every partition is heap; with it, a `DEFAULT` partition exists and every partition is columnar; the rows land columnar and read back as heap does; maintenance drops partitions past retention, creates the ones ahead columnar, and leaves no orphaned storage or row-group row; and it holds `AccessExclusiveLock` on the parent | + +## 91. test_capability_decline.py: a missing pyarrow declines by name, and nothing declines silently + +The pytest half of selftest parts 600 and 610, held by the Python harness's own machinery. It does not source, call or read `lib.sh`, `capability_sweep.sh` or either part: the twins are independent (the first version drove `lib.sh` from Python, and the harness-independence inventory refused it). + +The owner's rule of 2026-10-12 (#1215): a missing capability may be bypassed as a named skip, the run stays green, and no test may be silent. On this side that is `expect.declines(capability, *, name)`, a fourth record verdict `DECLINED`, and `expect.require_module(module, *, name)`. Five holes are closed, from OffgridwithJD's review of #1286: + +1. where `PGC_REQUIRE` names the capability, as CI does, a decline FAILS; +2. a decline must be the test's first outcome, and no check may follow it; +3. only an ABSENT module declines: a broken install (an `ImportError` from inside) fails; +4. a test file that imports pyarrow outside `require_module` is refused, in each of five import spellings, and each one is proven seen; +5. the run's accounting line carries `+ N declined` when there are any, and each decline is printed by name with its capability. + +Written red first: all seven failed before `pgc_vacuity` had the methods. The four failure arms pin their failure's MESSAGE, because a missing method also fails an inner test, so outcomes alone could not tell "refused for the right reason" from "crashed". + +### Every test + +| test | what it holds | +| --- | --- | +| `test_a_missing_pyarrow_declines_by_name_and_the_run_stays_green` | the inner run passes and exits 0; the accounting line carries `+ 1 declined`; the decline is printed with its node id and capability | +| `test_pgc_require_turns_a_decline_into_a_failure` | with `PGC_REQUIRE=pyarrow` the same test fails, and says pyarrow is required | +| `test_a_decline_after_a_counted_check_is_refused` | a decline after a recorded check is refused as not the first outcome | +| `test_a_check_after_a_decline_is_refused` | a check recorded after a decline is refused | +| `test_only_a_declinable_capability_may_decline` | `btree_gist` cannot decline; it is not in the closed list | +| `test_a_broken_install_fails_rather_than_declines` | a `pyarrow` that raises `ImportError` from inside fails with its own error | +| `test_every_unguarded_import_spelling_is_refused` | each of five spellings is named and refused; a file using `require_module` is not | diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py index fcbfd2102..329b5e694 100755 --- a/test/pytest/compare_to_bash.py +++ b/test/pytest/compare_to_bash.py @@ -148,6 +148,9 @@ def test_a_role_with_only_schema_usage_is_refused(..., func, name): "cannot_run": None, "plan_marker": None, "plan_node": None, + # #1215: both take the check name as a required KEYWORD, like cannot_run's. + "declines": None, + "require_module": None, } diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 64152697f..cd038a55a 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -20,6 +20,7 @@ """ import ast +import os import numbers import pathlib import re @@ -232,7 +233,15 @@ def note_write(nodeid, cur): # no outcome recorded, which is the reconciliation failure itself. SKIP has no # counterpart here: a bare skip is refused at collection, and the honest form is # expect.cannot_run(), which records UNRUN. -RECORD_VERDICTS = ("PASS", "FAIL", "UNRUN") +RECORD_VERDICTS = ("PASS", "FAIL", "UNRUN", "DECLINED") + +# The capabilities a test may DECLINE on, rather than declare unrunnable (#1215, the +# owner's decision of 2026-10-12, mirrored from lib.sh's pgc_skip). A decline is a +# named outcome that leaves the run green; everything else that cannot run goes +# through cannot_run and ends the run INCOMPLETE. Closed, so "optional" cannot grow +# by one call site at a time. PGC_REQUIRE (comma-separated) names capabilities that +# must be present here, as CI does, and turns a decline of them into a failure. +DECLINABLE_CAPABILITIES = ("pyarrow",) class VacuityError(AssertionError): @@ -347,6 +356,7 @@ def __init__(self, nodeid): self.nodeid = nodeid self._records = [] self.unrunnable = None + self.declined = None @property def records(self): @@ -420,6 +430,11 @@ def _record(self, name, verdict="PASS", reason=""): operation; it is phase 2's work and is not claimed here. What IS claimed here is the count, which the probe above shows is 2 and correct. """ + if self.declined is not None and verdict != "DECLINED": + raise VacuityError( + f"{name}: a check recorded after this test declined on " + f"{self.declined!r}. A decline means the test measured nothing, so it " + f"must be the test's only outcome: return right after declines().") self._records.append(_Record(name, verdict, reason)) # -- numbers ----------------------------------------------------------- @@ -1023,6 +1038,64 @@ def plan_marker(self, plan, key, *, name=None, absent=False): ) # -- the third state --------------------------------------------------- + @_resolving + def declines(self, capability, *, name, detail=""): + """Decline this test because an OPTIONAL capability is absent (#1215). + + A named outcome, verdict DECLINED, that leaves the run green. The shell's + pgc_skip does the same for pyarrow; the two harnesses do not call each + other. Five things keep it from becoming a way to stop asserting quietly + (OffgridwithJD, review of #1286): + + - only a capability in DECLINABLE_CAPABILITIES may decline; + - where PGC_REQUIRE names the capability -- CI -- this FAILS instead; + - it must be the test's first outcome, and no check may follow it; + - require_module() declines only on the module being ABSENT; + - each decline is printed by name, and counted in the accounting line. + """ + if capability not in DECLINABLE_CAPABILITIES: + raise VacuityError( + f"{name}: {capability!r} is not a declinable capability " + f"{DECLINABLE_CAPABILITIES}. A missing dependency that is not optional " + f"is unrunnable: use cannot_run('MISSING_DEPENDENCY', ...).") + if self._records: + raise VacuityError( + f"{name}: a decline must be the test's first outcome, and this test " + f"already recorded {len(self._records)} check(s). A test that half ran " + f"and then declined would read as one that measured nothing.") + required = [c.strip() for c in os.environ.get("PGC_REQUIRE", "").split(",") + if c.strip()] + # ONE record, after every refusal: a VacuityError raised after the record + # would be recorded as a failed assertion (test_every_refusal_precedes_its_ + # record). Where the capability is required, the record is a FAIL and the + # AssertionError below is the ordinary failure of a comparison. + must_have = capability in required + self._record(name, verdict="FAIL" if must_have else "DECLINED", + reason=f"{capability}: {detail}".rstrip(": ")) + if must_have: + raise AssertionError( + f"{name}: PGC_REQUIRE names {capability}, so it must be present here, " + f"and it is not: {detail}") + self.declined = capability + + def require_module(self, module, *, name): + """-> the imported module, or None after declining because it is ABSENT. + + Declines only on ModuleNotFoundError naming the module or its top-level + package. A module that is installed but broken -- an ImportError raised from + inside it, or an OSError loading its library -- propagates and fails the test, + because a broken install is not an absent one. + """ + import importlib + top = module.split(".")[0] + try: + return importlib.import_module(module) + except ModuleNotFoundError as e: + if e.name not in (module, top): + raise + self.declines(top, name=name, detail=f"{module} is not installed") + return None + @_resolving def cannot_run(self, reason, detail="", *, name=None): """Declare this test unrunnable. Not a pass, and not a silent skip. @@ -1296,6 +1369,7 @@ class _RecordCollector: def __init__(self): self.records = [] # (nodeid, verdict, name) self.offences = [] + self.declined = [] # (nodeid, name, reason): the capability is in reason def pytest_runtest_logreport(self, report): if report.when != "call": @@ -1305,6 +1379,8 @@ def pytest_runtest_logreport(self, report): for key, value in getattr(report, "user_properties", ()): if key == "pgc_records_held": held = value + elif key == "pgc_declined": + self.declined.extend((report.nodeid, n, r) for n, r in value) elif key == "pgc_records": arrived = list(value) if held is None and arrived is None: @@ -1351,6 +1427,13 @@ def pytest_runtest_makereport(item, call): report.user_properties.append(("pgc_records_held", rec.count)) report.user_properties.append( ("pgc_records", [(r.verdict, r.name) for r in rec.records])) + # A decline's capability rides in its reason, which pgc_records does not + # carry. Its own property, like pgc_unrunnable, so the record pairs that + # the reconciliation compares keep their shape. + if rec.declined is not None: + report.user_properties.append( + ("pgc_declined", [(r.name, r.reason) for r in rec.records + if r.verdict == "DECLINED"])) return report @@ -1367,6 +1450,13 @@ def pytest_terminal_summary(terminalreporter): claims in one test and one in another, where a per-test count would say 2 and the records say 5. """ + # DIRECT IMPORTS OF A DECLINABLE CAPABILITY, named whatever collection did. A file + # whose top-level import fails yields no items, so no hook that walks items ever + # sees it -- measured: the scan found the file and nothing printed. + # pytest_sessionfinish turns the same list into a failure. + for site in getattr(terminalreporter.config, "pgc_capability_imports", None) or []: + terminalreporter.write_line(f"REFUSED {site}") + collector = getattr(terminalreporter.config, "pgc_unrunnable", None) if collector is not None and collector.items: terminalreporter.write_line("") @@ -1381,10 +1471,17 @@ def pytest_terminal_summary(terminalreporter): for _nodeid, verdict, _name in records.records: if verdict in tally: tally[verdict] += 1 + if records.declined: + terminalreporter.write_line("") + for nodeid, nm, why in records.declined: + terminalreporter.write_line(f"DECLINED {nodeid}: {why}: {nm}") terminalreporter.write_line(f"checks run: {len(records.records)}") + # DECLINED joins the line only when present, so a run with no declines prints the + # line every reader and guard already parses, byte for byte. + shown = [v for v in RECORD_VERDICTS if v != "DECLINED" or tally[v]] terminalreporter.write_line( "accounting: " - + " + ".join(f"{tally[v]} {v.lower()}" for v in RECORD_VERDICTS) + + " + ".join(f"{tally[v]} {v.lower()}" for v in shown) + f" = {sum(tally.values())}" ) @@ -1454,6 +1551,12 @@ def pytest_sessionfinish(session, exitstatus): an unrunnable test is a failure, because the failure is the more urgent fact. So this only ever moves a run OFF zero, and never off a non-zero status. """ + # 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: + session.exitstatus = 1 + # THE RECONCILIATION, FIRST, because it is a statement about whether the run # can be believed at all rather than about one test (#937 phase 3). # @@ -1885,6 +1988,63 @@ def _sorted_ordered_sites(path): _order_killed_names, _sorted_ordered_sites = _order_guard() +def _capability_import_sites(path, capabilities=DECLINABLE_CAPABILITIES): + """-> one line per place a test file imports a declinable capability directly. + + A test that imports pyarrow itself, instead of through expect.require_module, + fails at import when pyarrow is absent, or worse, wraps the import in its own + try and passes having measured nothing. Either way the decline is not named. + Read from the AST, so a comment or a string that mentions the module cannot + trip it, and every spelling is covered: `import m`, `import m.sub as x`, + `from m import x`, `from m.sub import x`, `importlib.import_module('m...')` + and `__import__('m...')` (OffgridwithJD, review of #1286). + """ + try: + tree = ast.parse(pathlib.Path(path).read_text(), filename=str(path)) + except (OSError, SyntaxError, UnicodeDecodeError): + return [] + base = pathlib.Path(path).name + sites = [] + for node in ast.walk(tree): + names = [] + if isinstance(node, ast.Import): + names = [a.name for a in node.names] + elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module: + names = [node.module] + elif isinstance(node, ast.Call) and node.args: + f = node.func + fname = f.attr if isinstance(f, ast.Attribute) else ( + f.id if isinstance(f, ast.Name) else "") + arg = node.args[0] + if fname in ("import_module", "__import__") and isinstance(arg, ast.Constant) \ + and isinstance(arg.value, str): + names = [arg.value] + for n in names: + top = n.split(".")[0] + if top in capabilities: + sites.append(f"{base} imports {top} outside require_module " + f"(line {node.lineno})") + return sites + + +def pytest_collect_file(file_path, parent): + """Scan each test file for direct imports of a declinable capability. + + At FILE collection, before the module is imported: a file whose top-level + `import pyarrow` fails produces no items at all, so a scan over items would + never see the one file it exists for. + """ + if file_path.suffix == ".py" and file_path.name.startswith("test_"): + found = _capability_import_sites(file_path) + if found: + store = getattr(parent.config, "pgc_capability_imports", None) + if store is None: + store = [] + parent.config.pgc_capability_imports = store + store.extend(found) + return None + + def _broad_except_sites(path): try: tree = ast.parse(pathlib.Path(path).read_text()) diff --git a/test/pytest/requirements-test.txt b/test/pytest/requirements-test.txt index 6e3c6672f..147de622f 100644 --- a/test/pytest/requirements-test.txt +++ b/test/pytest/requirements-test.txt @@ -7,3 +7,4 @@ pytest==9.1.1 pytest-xdist==3.8.0 psycopg[binary]==3.3.5 +pyarrow==23.0.1 diff --git a/test/pytest/test_capability_decline.py b/test/pytest/test_capability_decline.py new file mode 100644 index 000000000..f52282f73 --- /dev/null +++ b/test/pytest/test_capability_decline.py @@ -0,0 +1,152 @@ +"""A missing pyarrow declines a pytest test by name; nothing declines silently (#1215). + +The pytest half of selftest parts 600 and 610, held by the PYTHON harness's own +machinery. It does not source, call or read lib.sh, capability_sweep.sh or either +part: the twins are independent, and each harness answers for itself. + +The owner's rule of 2026-10-12: a missing capability may be bypassed as a named skip, +the run stays green, and no test may be silent or vacuous. On this side that is +`expect.declines` and `expect.require_module`, with five ways a decline could hide +something closed off (OffgridwithJD, review of #1286): + + 1 where pyarrow is supposed to be (CI, PGC_REQUIRE=pyarrow) a decline FAILS + 2 a decline is a test's only outcome: not after a check, and no check after it + 3 only an ABSENT module declines; a broken install fails + 4 a test file that imports pyarrow outside require_module is refused, in every + import spelling, each proven seen + 5 the run's accounting line carries the declines + +Runs pytest inside pytest; needs no cluster and no driver. +""" + +# Absent for ONE inner test, through monkeypatch so it is undone: pytester runs +# in-process, and a module-level sys.modules entry would leak into every later test. +ABSENT = " import sys\n" + + +def test_a_missing_pyarrow_declines_by_name_and_the_run_stays_green(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pytester.makepyfile(ABSENT + """ + def test_inner_declines_green(expect, monkeypatch): + monkeypatch.setitem(sys.modules, "pyarrow", None) + pa = expect.require_module("pyarrow", name="the parquet fixture can be written") + assert pa is None +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a declined test leaves the run green", passed=1, failed=0) + expect.num(result.ret, 0, "and the run exits 0") + out = result.stdout.str() + expect.contains(out, "+ 1 declined", + "the accounting line carries the decline (hole 5)") + expect.contains(out, "DECLINED test_a_missing_pyarrow_declines_by_name_and_the_run_stays_green.py::test_inner_declines_green: pyarrow", + "and the decline is named, with its capability") + + +def test_pgc_require_turns_a_decline_into_a_failure(pytester, monkeypatch, expect): + monkeypatch.setenv("PGC_REQUIRE", "pyarrow") + pytester.makepyfile(ABSENT + """ + def test_inner_required_fails(expect, monkeypatch): + monkeypatch.setitem(sys.modules, "pyarrow", None) + expect.require_module("pyarrow", name="the parquet fixture can be written") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "with PGC_REQUIRE=pyarrow a missing pyarrow fails (hole 1)", + passed=0, failed=1) + expect.contains(result.stdout.str(), "PGC_REQUIRE names pyarrow", + "and it fails BECAUSE pyarrow is required, not for another reason") + + +def test_a_decline_after_a_counted_check_is_refused(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pytester.makepyfile(""" + def test_half_ran(expect): + expect.num(1, 1, "a check that ran") + expect.declines("pyarrow", name="too late to decline") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a decline after a counted check is refused (hole 2)", + passed=0, failed=1) + expect.contains(result.stdout.str(), "a decline must be the test's first outcome", + "and it is refused for that reason") + + +def test_a_check_after_a_decline_is_refused(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pytester.makepyfile(""" + def test_kept_going(expect): + expect.declines("pyarrow", name="declined") + expect.num(1, 1, "a check after the decline") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a check after a decline is refused (hole 2)", + passed=0, failed=1) + expect.contains(result.stdout.str(), "after this test declined", + "and it is refused for that reason") + + +def test_only_a_declinable_capability_may_decline(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pytester.makepyfile(""" + def test_wrong_capability(expect): + expect.declines("btree_gist", name="not optional") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a capability outside the closed list cannot decline", + passed=0, failed=1) + expect.contains(result.stdout.str(), "is not a declinable capability", + "and it is refused for that reason") + + +def test_a_broken_install_fails_rather_than_declines(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pkg = pytester.mkpydir("pyarrow") + (pkg / "__init__.py").write_text("raise ImportError('pyarrow built against another ABI')\n") + pytester.syspathinsert() + pytester.makepyfile(""" + import sys + + def test_inner_broken_install_fails(expect, monkeypatch): + # Drop any pyarrow already imported in this process, so the broken stub is + # what the import finds. + for k in [k for k in sys.modules if k == "pyarrow" or k.startswith("pyarrow.")]: + monkeypatch.delitem(sys.modules, k) + expect.require_module("pyarrow", name="the parquet fixture can be written") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "an ImportError from inside an installed pyarrow fails (hole 3)", + passed=0, failed=1) + expect.contains(result.stdout.str(), "pyarrow built against another ABI", + "and the failure is the broken install's own error, not a decline") + + +SPELLINGS = { + "test_spell_import.py": "import pyarrow\n", + "test_spell_import_sub.py": "import pyarrow.parquet as pq\n", + "test_spell_from.py": "from pyarrow import table\n", + "test_spell_from_sub.py": "from pyarrow.parquet import write_table\n", + "test_spell_importlib.py": "import importlib\nm = importlib.import_module('pyarrow.parquet')\n", +} + + +def test_every_unguarded_import_spelling_is_refused(pytester, monkeypatch, expect): + # The caller's PGC_REQUIRE must not decide this test (CI exports it). + monkeypatch.delenv("PGC_REQUIRE", raising=False) + for fname, body in SPELLINGS.items(): + pytester.makepyfile(**{fname[:-3]: body + "\ndef test_x(expect):\n expect.num(1, 1, 'x')\n"}) + pytester.makepyfile(test_guarded=""" + def test_guarded(expect): + expect.require_module("pyarrow", name="guarded") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + out = result.stdout.str() + seen = [f for f in SPELLINGS if f"{f[:-3]}.py imports pyarrow outside require_module" in out] + expect.num(len(seen), len(SPELLINGS), + "premise: the scan finds each of the five import spellings (hole 4)") + 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") diff --git a/test/pytest/test_failed_query_sentinel.py b/test/pytest/test_failed_query_sentinel.py index d2bbfb241..f00da4f3c 100644 --- a/test/pytest/test_failed_query_sentinel.py +++ b/test/pytest/test_failed_query_sentinel.py @@ -67,6 +67,8 @@ def _is_comparison(fn): "plan": "EXPLAIN plan, not a query value", "exc": "exception object, not a query value", "reason": "unrunnable declaration, not a comparison", + "capability": "a decline on an optional capability (#1215), not a comparison", + "module": "an optional module imported or declined on (#1215), not a comparison", } diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index f9441a811..d5b16c956 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -73,6 +73,9 @@ "test_suite_accounting.py", # #1288: reads docs/*.md and README.md only. "test_docs_cross_database_query.py", + # #1215: inner pytest runs of expect.declines and require_module. Needs no + # cluster and no driver; it never reaches into the shell harness. + "test_capability_decline.py", # The classifier's own controls, split out of this file so the job whose file # list IS this list actually runs them. This file cannot be in the list: it # hands cluster-bound file names to pytest, so it needs what they need. From 187544d2fdef76585671ad3017be3f458880f90d Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 21:19:16 +0000 Subject: [PATCH 5/6] test: ledger rows for parts 600 and 610 and the sweep's set arm, on main after #1288 (#1215) Twelve new checks; nine dated by mutation runs on all five majors, replayed from the saved logs onto #1288's ledger, merged one log at a time with --mutation and --target. The census moves 1664 -> 1667, the three new premises. guard_tests 405 -> 412 by collection. Co-Authored-By: Claude Opus 5.5 --- test/check_ledger.tsv | 14 +++++++++++++- test/check_ledger_budget.txt | 22 +++++++++++++++++++++- test/pytest/expected_tests.txt | 4 +++- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 0e449de9b..7e1c6e20b 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -48,12 +48,13 @@ base_scan_io base_scan_io premise: the later plan is still a base columnar scan base_scan_io base_scan_io premise: the later plan still does not name a covering projection 15;16;17;18;19 never - base_scan_io base_scan_io premise: the plan is a base columnar scan 15;16;17;18;19 never - base_scan_io base_scan_io premise: the table holds every inserted row 15;16;17;18;19 never - -capability_sweep capability_sweep no suite loses a check without recording a skip or a refusal 15;16;17;18;19 2026-09-23 a check_skip block removed from a suite so its arms vanish with no skip and no fail +capability_sweep capability_sweep no suite loses a check without recording a skip or a refusal 15;16;17;18;19 2026-10-12 a check_skip block removed from a suite so its arms vanish with no skip and no fail;the pyarrow decline reason stops naming the capability, so no skip counts as declined capability_sweep capability_sweep premise: and no member of the population is this very file 15;16;17;18;19 2026-09-23 the self-exclusion removed from pgc_cap_population, so the sweep enters its own population capability_sweep capability_sweep premise: and pyarrow IS importable without the shim, so absence is the shim's doing 15;16;17;18;19 never - capability_sweep capability_sweep premise: and pyarrow.parquet with it 15;16;17;18;19 2026-09-23 the shim's pyarrow.py emptied, so both runs are the present run capability_sweep capability_sweep premise: and the standard library still imports under it 15;16;17;18;19 never - capability_sweep capability_sweep premise: every suite in the population was classified 15;16;17;18;19 never - +capability_sweep capability_sweep premise: some suites in the population call pgc_skip pyarrow 15;16;17;18;19 never - capability_sweep capability_sweep premise: the mask held for every run in the sweep 15;16;17;18;19 2026-09-23 the shim's pyarrow.py emptied, so both runs are the present run capability_sweep capability_sweep premise: the population is the size this tree holds 15;16;17;18;19 never - capability_sweep capability_sweep premise: the priming run built and installed cleanly 15;16;17;18;19 never - @@ -61,6 +62,7 @@ capability_sweep capability_sweep premise: the shim makes pyarrow unimportable 1 capability_sweep capability_sweep premise: the sweep is not in the population it sweeps 15;16;17;18;19 2026-09-23 the self-exclusion removed from pgc_cap_population, so the sweep enters its own population capability_sweep capability_sweep premise: the two derivations of the population name the same suites 15;16;17;18;19 never - capability_sweep capability_sweep premise: while python itself still runs under the shim 15;16;17;18;19 never - +capability_sweep capability_sweep the suites that decline are exactly the suites that call pgc_skip pyarrow 15;16;17;18;19 2026-10-12 the pyarrow decline reason stops naming the capability, so no skip counts as declined catalog_delete_index catalog_delete_index A1 with more catalog pages the default does less work than reading every one whole 15;16;17;18;19 2026-10-12 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid catalog_delete_index catalog_delete_index B1 with large catalogs the default does far less work than reading every one whole 15;16;17;18;19 2026-10-12 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid catalog_delete_index catalog_delete_index P1 with a few catalog pages the default does less work than probing every one 15;16;17;18;19 2026-10-12 pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid @@ -1599,6 +1601,16 @@ harness_selftest 590-a-declared-suite-must-account the EXIT trap runs the wrappe harness_selftest 590-a-declared-suite-must-account the line it emits is the line the reconciliation reads 15;16;17;18;19 2026-09-23 the EXIT trap reverted to the bare pgc_teardown, so the eight pgc_setup exits emit no accounting harness_selftest 590-a-declared-suite-must-account the suite's own counters are restored after every driving block 15;16;17;18;19 never - harness_selftest 590-a-declared-suite-must-account the wrapper captures the exit status before anything overwrites it 15;16;17;18;19 2026-09-23 the EXIT trap reverted to the bare pgc_teardown, so the eight pgc_setup exits emit no accounting +harness_selftest 600-a-missing-pyarrow-declines a missing pyarrow declines the suite instead of failing it 15;16;17;18;19 2026-10-12 pgc_skip treats pyarrow like any other capability again, so a missing pyarrow fails the suite +harness_selftest 600-a-missing-pyarrow-declines and it records a skip under the check's own name, not a failure 15;16;17;18;19 2026-10-12 pgc_skip treats pyarrow like any other capability again, so a missing pyarrow fails the suite +harness_selftest 600-a-missing-pyarrow-declines control: a missing btree_gist still fails the suite 15;16;17;18;19 2026-10-12 pgc_skip declines every missing capability, not only pyarrow +harness_selftest 600-a-missing-pyarrow-declines control: and an explicit waiver still turns that failure into a skip 15;16;17;18;19 2026-10-12 pgc_skip ignores PGC_ALLOW_MISSING_, so only the run-wide waiver works +harness_selftest 600-a-missing-pyarrow-declines premise: lib.sh defines pgc_skip 15;16;17;18;19 never - +harness_selftest 610-a-declined-capability-is-not-silent premise: the sweep defines its verdict where this part reads it 15;16;17;18;19 never - +harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost beside a failure are refused 15;16;17;18;19 2026-10-12 pgc_capability_verdict answers declined for every input +harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost beside a skip naming the capability are declined 15;16;17;18;19 2026-10-12 pgc_capability_verdict has no declined outcome, so a lost name beside a skip reads silent +harness_selftest 610-a-declined-capability-is-not-silent the verdict: names lost with neither a failure nor a skip are silent 15;16;17;18;19 2026-10-12 pgc_capability_verdict answers declined for every input +harness_selftest 610-a-declined-capability-is-not-silent the verdict: no name lost is preserved 15;16;17;18;19 2026-10-12 pgc_capability_verdict answers declined for every input index_am_support index_am_support btree answers its operator with the same rows the scan returns 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan drops two rows in three, so the index misses entries index_am_support index_am_support btree builds an index over a columnar table 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan errors, so CREATE INDEX fails index_am_support index_am_support gist answers its operator with the same rows the scan returns 15;16;17;18;19 2026-09-23 pgcolumnar_index_build_range_scan drops two rows in three, so the index misses entries diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index b28c42098..90d0a683b 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -1276,4 +1276,24 @@ suites_not_covered 247 # never 1664 + 7 = 1671 # dated 219 + 5 = 224 # 1671 + 224 == 1895 -checks_never_observed_red 1671 +# +# #1215 (PR #1286): 1671 -> 1674. Twelve new checks, nine dated, three premises: +# +# harness_selftest 600 5 checks: 4 dated, 1 premise +# harness_selftest 610 5 checks: 4 dated, 1 premise +# capability_sweep 2 checks: 1 dated, 1 premise; and the silent arm +# re-dated +# +# Every date comes from a mutation run on all five majors, merged one log at a +# time with --mutation and --target, replayed from the saved logs onto #1288's +# ledger. The part 600 mutation logs predate #1284's renames, so the three +# old-name rows they carried were removed again, as #1284 did. Carried onto +# #1302's ledger unchanged: #1291's rows are another suite, and the result is +# main's rows less the one re-dated row plus 13 (12 new, 1 re-dated), checked +# row by row. +# +# ledger rows 1895 + 12 = 1907 +# never 1671 + 3 = 1674 +# dated 224 + 9 = 233 +# 1674 + 233 == 1907 +checks_never_observed_red 1674 diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index fe2dabe79..b44bf228a 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -263,7 +263,9 @@ # answer for a change that touches only the layer and its own guard. # 404 -> 405 for #1288: test_docs_cross_database_query.py, cluster-free, in NO_CLUSTER. # Re-derived by collection on 038600e (after #1303): `405 tests collected`. -guard_tests 405 +# 405 -> 412 for #1215: test_capability_decline.py, seven tests, cluster-free and in +# NO_CLUSTER. Re-derived by collection on 73d9663: `412 tests collected`. +guard_tests 412 # 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 From ac5030dc61ea035b27441014bcf92479c2231822 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 22:01:24 +0000 Subject: [PATCH 6/6] test: a pyarrow missing its own dependency fails rather than declines (#1215) OffgridwithJD, reviewing 3941f05, deleted require_module's .name check and all seven tests still passed. The case that check exists for is an installed pyarrow that cannot import one of its own dependencies: a ModuleNotFoundError naming that dependency, not pyarrow. This arm stubs exactly that. With the two-line check deleted it goes red (the inner test declines and passes); with it, 8 passed, with and without PGC_REQUIRE=pyarrow. Also adds __import__ to SPELLINGS, so the scanner's sixth documented spelling is proven too. guard_tests 412 -> 413, by collection. Co-Authored-By: Claude Opus 5.5 --- test/pytest/TESTS.md | 5 +++-- test/pytest/expected_tests.txt | 4 +++- test/pytest/test_capability_decline.py | 26 +++++++++++++++++++++++++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index e13d4f14f..a4437f9c5 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -6466,7 +6466,7 @@ The owner's rule of 2026-10-12 (#1215): a missing capability may be bypassed as 1. where `PGC_REQUIRE` names the capability, as CI does, a decline FAILS; 2. a decline must be the test's first outcome, and no check may follow it; 3. only an ABSENT module declines: a broken install (an `ImportError` from inside) fails; -4. a test file that imports pyarrow outside `require_module` is refused, in each of five import spellings, and each one is proven seen; +4. a test file that imports pyarrow outside `require_module` is refused, in each of six import spellings, and each one is proven seen; 5. the run's accounting line carries `+ N declined` when there are any, and each decline is printed by name with its capability. Written red first: all seven failed before `pgc_vacuity` had the methods. The four failure arms pin their failure's MESSAGE, because a missing method also fails an inner test, so outcomes alone could not tell "refused for the right reason" from "crashed". @@ -6481,4 +6481,5 @@ Written red first: all seven failed before `pgc_vacuity` had the methods. The fo | `test_a_check_after_a_decline_is_refused` | a check recorded after a decline is refused | | `test_only_a_declinable_capability_may_decline` | `btree_gist` cannot decline; it is not in the closed list | | `test_a_broken_install_fails_rather_than_declines` | a `pyarrow` that raises `ImportError` from inside fails with its own error | -| `test_every_unguarded_import_spelling_is_refused` | each of five spellings is named and refused; a file using `require_module` is not | +| `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 | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index b44bf228a..019b63b3d 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -265,7 +265,9 @@ # Re-derived by collection on 038600e (after #1303): `405 tests collected`. # 405 -> 412 for #1215: test_capability_decline.py, seven tests, cluster-free and in # NO_CLUSTER. Re-derived by collection on 73d9663: `412 tests collected`. -guard_tests 412 +# 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 # 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 f52282f73..d7ba9aa1c 100644 --- a/test/pytest/test_capability_decline.py +++ b/test/pytest/test_capability_decline.py @@ -124,7 +124,31 @@ def test_inner_broken_install_fails(expect, monkeypatch): "and the failure is the broken install's own error, not a decline") +def test_a_missing_dependency_of_pyarrow_fails_rather_than_declines(pytester, monkeypatch, expect): + # An installed pyarrow that cannot import one of ITS dependencies raises + # ModuleNotFoundError too, but naming the dependency. Declining on that would + # green a broken install (hole 3); only a missing pyarrow itself declines. + monkeypatch.delenv("PGC_REQUIRE", raising=False) + pkg = pytester.mkpydir("pyarrow") + (pkg / "__init__.py").write_text("import numpy_is_not_here\n") + pytester.syspathinsert() + pytester.makepyfile(""" + import sys + + def test_inner_missing_dependency_fails(expect, monkeypatch): + for k in [k for k in sys.modules if k == "pyarrow" or k.startswith("pyarrow.")]: + monkeypatch.delitem(sys.modules, k) + expect.require_module("pyarrow", name="the parquet fixture can be written") +""") + result = pytester.runpytest("-p", "pgc_vacuity") + expect.outcomes(result, "a pyarrow missing its own dependency fails (hole 3)", + passed=0, failed=1) + expect.contains(result.stdout.str(), "No module named 'numpy_is_not_here'", + "and the failure names the missing dependency, not a decline") + + SPELLINGS = { + "test_spell_dunder.py": "m = __import__('pyarrow')\n", "test_spell_import.py": "import pyarrow\n", "test_spell_import_sub.py": "import pyarrow.parquet as pq\n", "test_spell_from.py": "from pyarrow import table\n", @@ -146,7 +170,7 @@ def test_guarded(expect): out = result.stdout.str() seen = [f for f in SPELLINGS if f"{f[:-3]}.py imports pyarrow outside require_module" in out] expect.num(len(seen), len(SPELLINGS), - "premise: the scan finds each of the five import spellings (hole 4)") + "premise: the scan finds each of the six import spellings (hole 4)") 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")