A suite that ran no checks must stop reporting PASSED (#447) - #448
Conversation
…rompt#447) pgc_summary decided the verdict from PGC_FAIL alone. PGC_CHECKS was printed and never read. So a suite that asserted nothing printed PASSED and exited 0, which is indistinguishable from one that ran four hundred checks, and run_all_versions reads only the exit code. Fifteen suites do exactly that whenever pyarrow is absent: arrow_export arrow_nested parquet_export parquet_import parquet_nested fuzz_arrow fuzz_parquet native_parquet_codecs native_parquet_hardening native_parquet_multifile native_parquet_partition native_parquet_projection native_parquet_pushdown native_parquet_streaming native_parquet_units That is the whole Arrow and Parquet surface including both fuzzers, and the run says PASSED beside every one of them. It is the same defect commandprompt#422 fixed one level up, where a matrix reported ALL VERSIONS PASSED having run none of them. Skipped is now its own state rather than a kind of pass. 0 passed, 1 failed, 2 ran no checks. Making it a failure instead would have been wrong and would have reddened the gate on the first run: PostgreSQL 15 has no WITHOUT OVERLAPS for temporal.sh to test, and a box without pg_isolation_regress is a supported configuration. A red everyone knows to ignore is a red nobody reads. Measured, on one box, same tree, same build: pyarrow present rc=0 PASSED 19 checks pyarrow absent rc=2 SKIPPED 0 checks pyarrow absent, PGC_REQUIRE_PYARROW=1 rc=1 FAILED native_roundtrip (control) rc=0 PASSED 8 checks harness_selftest rc=0 PASSED 33 checks The first two rows have to disagree. Before this they were both PASSED. PGC_REQUIRE_PYARROW follows PGC_REQUIRE_ISOLATION, which isolation.sh has used for this exact purpose since it was written, rather than inventing a second convention. Whether CI sets it is a policy call and is not made here. All four consumers had to learn the third state, because every one of them treated non-zero as failure: run_all_versions.sh, run_san.sh, run_coverage.sh and devloop.sh. run_all_versions now prints "suites that ran: n of m" per major and fails at zero. run_san fails if every suite it ran was a skip, because five of its default subset are pyarrow-gated and it would otherwise report a sanitizer gate passing having sanitized nothing. The PGC_SKIP_TIMING synthetic result now writes 2 rather than 0. Those suites do not run, and recording them as passes was the same claim, written by the driver. native_repack.sh and pg19_vacuum_options.sh printed their own SKIPPED line and exited 0 without calling pgc_summary, so no change inside pgc_summary could reach them. They now route through it and report the same state as everything else. Not fixed here, and worth naming: arrow_import, native_parquet_flba, native_parquet_schema and parquet_nested_import skip only a SECTION when pyarrow is missing and keep their other checks. They will still report a genuine pass having lost real coverage. This closes the zero-check gap, not the pyarrow gap. Running the suites this way immediately found a second instance: temporal.sh reports zero checks on PG18 on my bench, where the feature does exist, because btree_gist is not installed there. It has been reporting PASSED.
Owner call, and the right one. The previous commit made a zero-check run visible
but still let it pass the gate, which is the same instinct that produced the bug:
protect the run from red. A skip is a red nobody has to look at.
A box that cannot run a suite has not passed it. So a missing dependency now
FAILS, and the waiver is explicit and per-capability rather than implied by
silence:
PGC_ALLOW_MISSING_PYARROW=1 test/native_parquet_units.sh
PGC_ALLOW_MISSING=1 test/run_all_versions.sh
Missing DEPENDENCY and not-applicable-to-this-MAJOR stay different code paths,
because collapsing them would mean PG15 can never be green. PostgreSQL 15 has no
WITHOUT OVERLAPS and no amount of installing will give it one. Those gates call
pgc_summary directly and still report SKIPPED. pgc_skip is only for the case where
something is genuinely absent that ought to be there.
Ten arms, one box, same tree and build:
pyarrow present rc=0 PASSED
pyarrow absent rc=1 FAILED <- the change
absent, PGC_ALLOW_MISSING_PYARROW=1 rc=2 SKIPPED
absent, PGC_ALLOW_MISSING=1 rc=2 SKIPPED
temporal on PG17, feature absent rc=2 SKIPPED <- must not be red
native_repack on PG18 (PG19 only) rc=2 SKIPPED <- must not be red
pg19_vacuum_options on PG18 rc=2 SKIPPED <- must not be red
temporal on PG18, no btree_gist rc=1 FAILED <- real defect, found
native_roundtrip (control) rc=0 PASSED
harness_selftest (control) rc=0 PASSED
Rows 5 through 7 are the ones that decide whether this is shippable rather than
just satisfying. If any of them had gone red the matrix could never be green.
temporal.sh's btree_gist gate moves to pgc_skip because that IS a missing
dependency: the assert builds do not carry contrib, so temporal has been reporting
PASSED on PG18 and PG19 on my bench while asserting nothing. That is now a failure
telling someone to install btree_gist, which is what it always should have been.
CI already installs pyarrow (ci.yml:396, deliberately under sudo, with a comment
recording that installing it as the runner is how these suites came to report PASS
in CI while skipping themselves). So the pyarrow half of this is proven safe there.
Whether CI carries btree_gist I have NOT verified, and this PR's own checks are the
measurement. If temporal reds on this branch, that is the answer and the fix is a
package in ci.yml, not a softer verdict here.
Owner call: a missing dependency now FAILS, it does not skipPushed The waiver is explicit and per-capability, so a developer laptop still works and the waiver is Missing dependency and not-applicable-to-this-major stay separate paths on purpose. PG15
Rows 5 to 7 are what decide whether this is shippable rather than merely satisfying. If any of Correcting myself on CIIn the PR description I said I had not checked whether CI installs pyarrow. It does, at
Worth noting what the current CI summary already shows without any of this: |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. And I chased the question you left open: CI is fine, our own matrix is not.
This closes the gap I fell into repeatedly today. Every throwaway probe I wrote printed
checks run: 0 and PASSED, and I read past it every time because the word PASSED was
right there. Making skipped a third state rather than a failure is the right call, and
for the reason you give: a box without pg_isolation_regress is supported, and PG15 has no
WITHOUT OVERLAPS to test.
The table where rows one and two must disagree is the whole argument, and they used to be
identical.
Your temporal.sh finding: I can close the part you said you had not checked
temporal.shreports zero checks on PG18 on my bench, where the feature exists, because
btree_gistis not installed in the assert builds. I have not checked whether CI is in
the same position, and I am not claiming it is.
CI is not in the same position. Our local matrix is, on four majors out of five.
btree_gist in the SOURCE builds (what test/run_all_versions.sh uses):
pg15 ABSENT
pg16 ABSENT
pg17 ABSENT
pg18 present
pg19 ABSENT
PGDG packages (what CI apt-installs):
postgresql-18 ships btree_gist: yes
CI installs postgresql-N, which ships btree_gist on PGDG, so CI has really been running
this suite. Our source builds are configured without contrib, so the five-major matrix has
been reporting temporal coverage it never ran on 15, 16, 17 and 19.
Reproduced on main, PG17:
checks run: 0
temporal.sh: PASSED
That inverts the usual worry. The gate we treat as authoritative is the weaker one here, and
CI is the arm that was honest.
It also means I was wrong on #411. I approved the claim that temporal constraints are
"covered by test/temporal.sh" and checked only that the file exists and greps for the right
things. On the matrix I would have quoted, it asserted nothing on four of five majors. Your
change is what makes that visible, which is the argument for it.
On the two you asked to be read sceptically
1. The partial-skip gap is real and I agree it is separate. arrow_import,
native_parquet_flba, native_parquet_schema, parquet_nested_import lose a section and
keep a real pass, so the count stays non-zero and this change cannot see them. Worth its own
issue rather than scope creep here; the count is the wrong instrument for that one and it
needs a per-section notion of skipped.
2. Whether CI sets PGC_REQUIRE_PYARROW is a policy call and you were right to leave it.
My view, non-blocking: yes for CI, because CI installs pyarrow deliberately and a silent
skip there means the install broke, which is a real regression. Not for the local matrix,
where a developer without pyarrow should still get a useful run. That is the same split
PGC_REQUIRE_ISOLATION already makes, so following it rather than inventing a convention was
right.
One thing I would add before merge, not blocking
The matrix's new suites that ran: n of m line is the counterpart to #422's
versions run. Given what it just exposed, it is worth also printing which suites
skipped, not only how many. n of m tells you something is missing; the names tell you
whether it is temporal on four majors or isolation on a box without the harness, and
those need very different reactions.
@ChronicallyJD: every sentence I added was contradicted by line 40 of the file I added them to. Verified, and they are right. #448 merged at 03:17 and this branch sits on top of it. Since then pgc_skip FAILS on a missing dependency rather than skipping, which was the owner's call and the reason the three-state split exists. My prose described the pre-#448 world throughout: "the suite skips" where it fails, "read the SKIPPED line" where there is none, and "the run does not say so anywhere else" where the run says so loudly. It also never mentioned PGC_ALLOW_MISSING_BTREE_GIST, which is the thing a developer without contrib actually needs. Rewritten against measured behaviour rather than remembered behaviour. All four configurations run on PG18 and PG17: btree_gist present rc=0 PASSED, 5 checks btree_gist absent rc=1 FAILED btree_gist absent, waived rc=2 SKIPPED PG17, feature genuinely absent rc=2 SKIPPED (ran no checks) One correction to the review, which suggested the skipped case is "counted in the suites that ran: n of m line". It is not: run_all_versions.sh increments suites_skipped for rc=2 and suites_ran only for rc=0, and prints "suites that ran: n of m (skipped: k)". The docs now say a skip counts in k rather than n, because the point of the section is telling a reader what they will actually see. The distinction the review called worth keeping is kept, with the verdicts attached: a feature absent on this major is expected and skips, a dependency absent fails, and a waived dependency skips so the loss stays visible in the count instead of becoming a pass.
The per-major tally added up only while everything passed. #448 counts a suite as having run when it exits 0 and as skipped when it exits 2, and a failure lands in neither. The first five-major matrix after it merged reported PG18 suites that ran: 119 of 122 (skipped: 2) temporal=FAIL PG19 suites that ran: 121 of 122 (skipped: 0) temporal=FAIL 121 + 0 is not 122. The failing suite vanished from the count whose entire purpose is to say what ran, which is a milder version of the confusion #447 was about. Four majors hid it. A tally only contradicts itself once something fails, and 15, 16 and 17 were green.
2 participants
Closes #447. Full reasoning and the measurements are in the commit message; the short version:
pgc_summarytook the verdict fromPGC_FAILalone and never readPGC_CHECKS, so a suitethat asserted nothing printed PASSED and exited 0. Fifteen suites do that whenever pyarrow is
absent, which is the whole Arrow and Parquet surface including both fuzzers.
Skipped is now its own state: 0 passed, 1 failed, 2 ran no checks. Making it a failure
would have reddened the gate on the first run, because PG15 has no
WITHOUT OVERLAPSfortemporal.shto test and a box withoutpg_isolation_regressis supported.PGC_REQUIRE_PYARROW=1native_roundtrip(control)harness_selftestThe first two rows have to disagree. Before this they were both PASSED.
All four consumers needed teaching, since each treated non-zero as failure:
run_all_versions.sh,run_san.sh,run_coverage.sh,devloop.sh. The matrix now printssuites that ran: n of mper major and fails at zero, mirroring #422 one level down.
PGC_REQUIRE_PYARROWfollowsPGC_REQUIRE_ISOLATIONrather than inventing a secondconvention. Whether CI sets it is a policy call and I have not made it here.
Two things I want reviewed sceptically
arrow_import,native_parquet_flba,native_parquet_schemaandparquet_nested_importskip only asection without pyarrow and keep other checks, so they will still report a real pass having
lost real coverage. Separate problem, not addressed.
temporal.shreports zero checks on PG18 on mybench, where the feature exists, because
btree_gistis not installed in the assert builds.It has been reporting PASSED. I have not checked whether CI is in the same position, and I am
not claiming it is.