From d8929ae177897ddf341ac14716f493bc4f8ba94d Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sat, 12 Sep 2026 11:34:18 -0600 Subject: [PATCH 1/5] ci: run the 93 cluster tests, and make both pytest jobs assert their size (#1016) ci.yml had one pytest job, pytest-guards, and it installs psycopg deliberately NOT -- that absence is what proves those files need no database. nightly.yml mentions pytest zero times. run_all_versions.sh mentions it zero times and must, because the two harnesses stay independent and the shell runner invoking pytest is the cross-harness call the project forbids. lib.sh names pytest 11 times and every one is a comment. So 8 files ran nowhere: green when somebody ran them by hand, silent when they stopped. They were never broken -- measured on pg18a with the driver present, 99 collected, 289 checks, 289 pass, 40 seconds. --pgc-expect-tests is now passed by BOTH jobs, from test/pytest/expected_tests.txt. The flag existed and nothing used it. What it closes is narrower than "pytest passed having run nothing" and worse: a nonexistent path already fails on its own, but a file list that resolves to REAL files and collects FEWER tests does not. Measured, dropping one file from the guard list: unarmed rc=0 "255 passed" 17 tests gone armed rc=4 "collected 255 test(s) but expected 272" and the flag refuses 0 as vacuous rather than accepting it. THE NUMBERS ARE IN A TRACKED FILE for the reason check_ledger_budget.txt gives about its own: a change to one is then a diff a reviewer sees, next to the test that moved it. PGC_SKIP_TIMING is the precedent for the other choice -- set in two workflow files, suppressing whole suites for months, with no diff ever showing it. The SPLIT stays derived from NO_CLUSTER in both jobs. test -n guards every derived value, because an empty read would omit the flag and fail OPEN. Two new arms hold the new job to what the old one is held to, and a third holds both to passing the flag from the tracked file. The existing arm caught my own work: it forbids a corpus count in a job comment, my comment said "8 files", and it failed before the job ever ran. The count is in the CHANGELOG instead, which is dated. README.md's recorded reason had gone stale twice over: it said CI would first have to install from requirements-test.txt, which pytest-guards already does, and it proposed registering the run in SUITES, which is the forbidden cross-harness call. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a --- .github/workflows/ci.yml | 130 ++++++++++++++++++++++++++++++- CHANGELOG.md | 40 ++++++++++ test/pytest/README.md | 22 ++++-- test/pytest/TESTS.md | 2 + test/pytest/expected_tests.txt | 46 +++++++++++ test/pytest/test_harness_deps.py | 81 +++++++++++++++++++ 6 files changed, 314 insertions(+), 7 deletions(-) create mode 100644 test/pytest/expected_tests.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aeb65f35..ac569683 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,10 +110,138 @@ jobs: cd test/pytest FILES="$(python3 -c 'import sys; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')" test -n "$FILES" + # HOW MANY TESTS THIS MUST COLLECT, from the tracked file (#1016). pytest + # exits 0 having collected nothing, so this job could pass with a bad file + # list, an import error, or a rename that emptied the glob -- and did assert + # nothing about its own size until now. `test -n` matters as much as the + # number: an empty read would omit the flag and fail OPEN. + WANT="$(awk '$1=="guard_tests"{print $2}' expected_tests.txt)" + test -n "$WANT" # Printed, not stated: the count is a fact about the tree at this # commit, so it belongs in the output rather than in a comment. echo "running $(set -- $FILES; echo $#) database-free file(s): $FILES" - PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q $FILES + echo "expecting $WANT collected test(s)" + PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q --pgc-expect-tests "$WANT" $FILES + + # THE OTHER HALF OF THE CORPUS (#1016). The files that need the driver and a cluster + # ran in NO job: ci.yml had one pytest job and it installs psycopg deliberately NOT, + # nightly.yml mentions pytest zero times, and run_all_versions.sh must mention it zero + # times because the two harnesses stay independent. They passed when somebody ran them + # by hand and nothing noticed when they stopped -- the shape of the nightly gate that + # sat red for 25 nights behind a green PR gate. + # + # NO COUNT HERE, and the arm in test_harness_deps.py enforces that over this comment + # as well as the job body: a number in a comment is the same hand-maintained derived + # value as a number in a list, and the last one went stale the day it was written. The + # job PRINTS what it ran, and the measured figures live in the CHANGELOG entry, which + # is dated. This comment was caught by that arm before the job ever ran. + # + # ONE MAJOR, not the matrix. conftest.py's default is an assert build that exists on + # the audit container and not here, so the pg_config is passed explicitly. Widening to + # a matrix is a separate decision: this job's purpose is that these tests RUN at all, + # and a matrix would multiply a cost nobody has measured yet before the first green. + pytest-cluster: + name: pytest (cluster tests, with the driver) + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + + - name: Add the PGDG repository and install PostgreSQL 18 with its headers + run: | + set -euo pipefail + sudo install -d /usr/share/postgresql-common/pgdg + # Bounded and retried, for the reason the suites job gives: a plain + # "curl -fsSL" waits indefinitely on a stalled connection, which hung a + # step for 40 minutes on an otherwise healthy runner (#351). + sudo curl -fsSL --connect-timeout 15 --max-time 120 \ + --retry 5 --retry-delay 5 --retry-all-errors \ + -o /usr/share/postgresql-common/pgdg/apt.postgresql.org.asc \ + https://www.postgresql.org/media/keys/ACCC4CF8.asc + echo "deb [signed-by=/usr/share/postgresql-common/pgdg/apt.postgresql.org.asc] \ + https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" \ + | sudo tee /etc/apt/sources.list.d/pgdg.list >/dev/null + sudo timeout 300 apt-get -o Acquire::Retries=5 update + sudo timeout 600 apt-get -o Acquire::Retries=5 install -y --no-install-recommends \ + postgresql-18 postgresql-server-dev-18 \ + liblz4-dev libzstd-dev zlib1g-dev + # NOT cached, deliberately. The suites job caches because it runs a matrix for + # an hour; a second copy of that cache-key derivation is a thing that goes + # stale silently, and this job is minutes long. + + # ASSERTED, not assumed. A missing pg_config would otherwise surface as a + # confusing failure inside conftest's build rather than here. + - name: check the pg_config the tests will be given + run: | + set -euo pipefail + PGC="/usr/lib/postgresql/18/bin/pg_config" + test -x "$PGC" + echo "pg_config: $PGC" + "$PGC" --version + # pgc_cluster.py runs initdb as THIS user; root cannot, and the runner is not + # root. Said out loud because it is the assumption that breaks on a + # container. + echo "running as: $(id -un) (uid $(id -u))" + test "$(id -u)" -ne 0 + + # MAKE THE INSTALL TARGETS WRITABLE, because this job must stay non-root and + # therefore cannot install. `make install` writes the .so to pkglibdir and the + # control/SQL files to sharedir/extension, both root-owned on a PGDG install, so the + # first run of this job failed inside conftest's build with + # + # RuntimeError: pgcolumnar failed to build or install from ...; + # refusing to report checks against whatever was installed before. + # + # Diagnosed by @jdatcmd, who also named why the suites job does not hit it: that one + # runs the whole matrix under `sudo -E` and lib.sh drops to `runuser -u postgres` for + # what must not be root. The pytest harness has no equivalent drop, so it is non-root + # throughout -- initdb refuses to run as root, so it has to be. + # + # Of the three ways out, this is the one that KEEPS the non-root assertion above, + # which is a decision rather than an accident. + - name: make the extension's install targets writable by this user + run: | + set -euo pipefail + PGC="/usr/lib/postgresql/18/bin/pg_config" + LIB="$("$PGC" --pkglibdir)" + SHARE="$("$PGC" --sharedir)" + sudo chown -R "$(id -un)" "$LIB" "$SHARE" + # ASSERTED, not assumed. A chown that changed nothing would surface as the same + # opaque build failure this step exists to remove, two steps later and in + # somebody else's traceback. + test -w "$LIB" + test -w "$SHARE/extension" + echo "writable by $(id -un): $LIB and $SHARE/extension" + + - name: install pytest and the driver, pinned from requirements-test.txt + run: | + set -euo pipefail + python3 -m venv /tmp/pgcvenv + # EVERY pin, including psycopg, which is what makes this job the other half of + # pytest-guards. Installing "whatever the runner carries" is what + # requirements-test.txt exists to prevent. + /tmp/pgcvenv/bin/pip install --quiet -r test/pytest/requirements-test.txt + # The control for pytest-guards' own assertion: that job proves the guard + # files need no driver by its ABSENCE, so this one states its presence. + /tmp/pgcvenv/bin/pip show psycopg >/dev/null + + - name: run the tests that need a cluster + run: | + set -euo pipefail + cd test/pytest + # THE COMPLEMENT of NO_CLUSTER, derived rather than written here. A second + # hand-maintained copy of which file needs a database goes stale silently, + # which is why pytest-guards derives its list from the same place. + FILES="$(python3 -c 'import sys, pathlib; sys.path.insert(0, "."); from test_harness_deps import NO_CLUSTER; print(" ".join(p.name for p in sorted(pathlib.Path(".").glob("test_*.py")) if p.name not in NO_CLUSTER))')" + test -n "$FILES" + WANT="$(awk '$1=="cluster_tests"{print $2}' expected_tests.txt)" + test -n "$WANT" + echo "running $(set -- $FILES; echo $#) cluster file(s): $FILES" + echo "expecting $WANT collected test(s)" + PYTHONPATH=. /tmp/pgcvenv/bin/pytest -q \ + --pg-config /usr/lib/postgresql/18/bin/pg_config \ + --pgc-expect-tests "$WANT" \ + $FILES # Build against every supported major. Fast, and it is what an API change # between majors trips first. diff --git a/CHANGELOG.md b/CHANGELOG.md index aaae43d6..8152cce9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,46 @@ true until the next version shipped. Field 5 because the entry above inserted the majors as field 4. At the moment this change landed on its own it was field 4; both ship in the same release, so the form here is the one that works on the shipped tree. +- The 93 pytest tests that needed a cluster now run in CI, and both pytest jobs assert + how many tests they collected (#1016). + + `ci.yml` had one pytest job, `pytest-guards`, and it installs psycopg deliberately NOT + -- that absence is what proves those files need no database. `nightly.yml` mentions + pytest zero times. `run_all_versions.sh` mentions it zero times and must, because the + two harnesses stay independent and the shell runner invoking pytest is the cross-harness + call the project forbids. So 8 files and 93 test functions, 26% of the corpus, ran + nowhere: green when somebody ran them by hand, silent when they stopped. + + They were never broken. Measured on `pg18a` with the driver present: 99 collected, + 289 checks, 289 pass, 40 seconds. + + `--pgc-expect-tests` is now passed by BOTH jobs, from `test/pytest/expected_tests.txt`. + The flag existed and nothing used it. What it closes is narrower than "pytest passed + with no tests" and worse: a file list that resolves to real files and collects FEWER + tests than it should. Measured, dropping one file from the guard list: + + unarmed rc=0 "255 passed" 17 tests gone, nothing said + armed rc=4 "collected 255 test(s) but expected 272" + + A nonexistent path already fails on its own, so that was not the hole. A valid-but-short + list was. + + THE NUMBERS ARE IN A TRACKED FILE, not in the workflow and not in an environment + variable, for the reason `check_ledger_budget.txt` gives about its own: a change to one + is then a diff a reviewer sees, sitting next to the test that moved it. `PGC_SKIP_TIMING` + is the precedent for the other choice -- set in two workflow files, suppressing whole + suites for months, with no diff ever showing it. + + The SPLIT is derived from `NO_CLUSTER` in `test_harness_deps.py`, in both jobs, rather + than written out again: two copies of which file needs a database is a thing that goes + stale silently. `test -n` guards every derived value, because an empty read would omit + the flag and fail OPEN. + + `test/pytest/README.md` recorded the old reason and it had gone stale twice over: it + said CI would have to install from `requirements-test.txt` first, which `pytest-guards` + already does, and it proposed registering the run in `SUITES`, which is the + cross-harness invocation the independence rule forbids. A second CI job was always the + mechanism. - The mutation ledger covers a third suite: `differential`, 204 checks (#752). diff --git a/test/pytest/README.md b/test/pytest/README.md index 27657f52..339473bb 100644 --- a/test/pytest/README.md +++ b/test/pytest/README.md @@ -46,13 +46,23 @@ It compares the two by assertion NAME and exits non-zero if the bash suite asser a property the port does not. A port keeps this working by passing each assertion the same name string the bash check uses. -## This is not in the gate yet +## Both halves are in the gate (#1016) -`test/run_all_versions.sh` does not run these tests, and neither does CI. That is a -decision with a price, recorded in section 1a of the design document: `pgc_skip` -treats a missing dependency as a failure rather than a skip, so registering this run -in `SUITES` would redden every CI job until `ci.yml` installs from -`requirements-test.txt`. Until someone takes that decision, run it by hand. +`test/run_all_versions.sh` does not run these tests and must not: the two harnesses stay +independent, and the shell runner invoking pytest is the cross-harness call the project +forbids. Registering the run in `SUITES` was the plan recorded in section 1a of the design +document, and it was the wrong mechanism for that reason. A second CI job is the right one. + +`ci.yml` runs two: + +- **`pytest-guards`** runs the files `NO_CLUSTER` names, in a venv where psycopg is + deliberately ABSENT. That absence is what proves those files need no database. +- **`pytest-cluster`** runs the complement, with every pin from + `requirements-test.txt` and a PGDG PostgreSQL 18 with its headers. + +Both pass `--pgc-expect-tests` from `expected_tests.txt`, so a run that collects fewer +tests than it should fails instead of reporting a green that means nothing. **Adding a test +moves a number in that file**, and the diff sits next to the test that moved it. ## Warnings diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index bb3a041f..c18afb96 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1576,6 +1576,8 @@ fixtures are read off `conftest.py` rather than named in the classifier. | `test_the_gate_runs_the_membership_decision_rather_than_only_this_file` | selftest 350 runs the decision, and the command line it uses works | | `test_ci_derives_the_file_list_rather_than_repeating_it` | the CI job asks this module for `NO_CLUSTER`, names no file literally, and states no count | | `test_the_job_installs_no_database_driver` | the job asserts psycopg is absent rather than assuming it | +| `test_the_cluster_job_runs_the_other_half_and_derives_it` | the complement of `NO_CLUSTER` is RUN, derived not listed, and asserts the driver IS present | +| `test_both_pytest_jobs_assert_how_many_tests_they_collected` | both jobs pass `--pgc-expect-tests` from the tracked file, and each guards the read | | `test_the_shell_reference_detector_sees_code_and_not_prose` | the premise: a docstring is prose, a string passed to bash is a reference, an f-string counts once | | `test_the_harness_independence_inventory_is_exactly_what_the_corpus_does` | CONTEXT.md's inventory, asserted in both directions | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt new file mode 100644 index 00000000..34f15583 --- /dev/null +++ b/test/pytest/expected_tests.txt @@ -0,0 +1,46 @@ +# How many tests each half of the pytest corpus must collect. +# +# IN A TRACKED FILE, not in the workflow and not in an environment variable, for the +# reason check_ledger_budget.txt gives about its own numbers: a change to this is then a +# diff a reviewer sees, and the diff sits next to the test that moved it. PGC_SKIP_TIMING +# is the precedent for what the other choice costs -- set in two workflow files, +# suppressing whole suites for months, with no diff ever showing it. +# +# WHY A NUMBER AT ALL. `pytest` exits 0 when it collects nothing. An import error in one +# file, a bad file list, a venv missing the driver, a rename that empties a glob: each +# produces a green job that ran no tests and said so only in a line nobody reads. This +# repository has paid for that shape repeatedly -- a pending count that could not see a +# job which never started, an `until` loop that exited instantly on zero, a `grep -q` that +# closed its pipe. `--pgc-expect-tests` turns "collected fewer than it should" into a +# failure, and it refuses 0 as vacuous rather than accepting it. +# +# THE NUMBERS ARE COLLECTED TESTS, NOT TEST FUNCTIONS. Parametrization expands one +# function into several, and `--pgc-expect-tests` compares against what pytest collected. +# Re-derive them with collection alone, which needs no cluster and no driver for the guard +# half: +# +# cd test/pytest +# G="$(python3 -c 'import sys; sys.path.insert(0,"."); from test_harness_deps import NO_CLUSTER; print(" ".join(NO_CLUSTER))')" +# PYTHONPATH=. pytest --collect-only -q $G | tail -1 +# +# and for the cluster half, the complement of NO_CLUSTER, with the driver installed. +# +# THE SPLIT ITSELF IS DERIVED from NO_CLUSTER in test_harness_deps.py and is not repeated +# here. Two copies of which file needs a database is a thing that goes stale silently. + +# The files NO_CLUSTER lists: the harness's own guards, which drive pytest inside pytest +# and must run with psycopg ABSENT. The job that runs them is what proves they need no +# database. +# +# This number moved 272 -> 274 when #1010's first step merged, which added two tests to +# test_mutation_ledger.py. That is the mechanism doing its job rather than a nuisance: had +# it not moved, the job would have failed with "collected 274 test(s) but expected 272" and +# named the drift instead of running a different suite than the one declared. +guard_tests 274 + +# 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 +# silent when they stopped. The number below moved from 99 to 101 in that same change, +# because it added two arms about the jobs, which is the mechanism working: a test added is +# a number moved, in a diff next to the test. +cluster_tests 101 diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 7f24030b..ec565405 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -883,6 +883,87 @@ def test_ci_derives_the_file_list_rather_than_repeating_it(expect): "the job and its comment state no corpus count") +def test_the_cluster_job_runs_the_other_half_and_derives_it(expect): + """The complement of NO_CLUSTER must be RUN, and derived rather than listed (#1016). + + 93 test functions in 8 files ran in no CI job at all: ci.yml had one pytest job and it + installs psycopg deliberately not, nightly.yml mentions pytest zero times, and + run_all_versions.sh must mention it zero times because the two harnesses stay + independent. A quarter of the corpus passed when somebody ran it by hand and nothing + noticed when it stopped. + + Derived, for the reason `pytest-guards` derives its half: two hand-maintained copies of + which file needs a database is a value whose correct content is a function of the tree, + and it goes stale silently because nothing compares them. + """ + ci = (HERE.parent.parent / ".github" / "workflows" / "ci.yml") + expect.text(repr(ci.is_file()), "True", "premise: ci.yml is where this expects") + text = ci.read_text() + + expect.at_least(text.count("pytest-cluster:"), 1, + "a job runs the half that needs a cluster") + job = text[text.index("pytest-cluster:"):] + job = job[:job.index("\n build:")] if "\n build:" in job else job + + expect.at_least(job.count("NO_CLUSTER"), 1, + "and it derives its file list from this module rather than listing it") + expect.at_least(job.count("requirements-test.txt"), 1, + "and installs the pins from requirements-test.txt, not whatever is there") + # The control for pytest-guards' own assertion. That job proves its files need no + # database by psycopg's ABSENCE, so this one has to state its presence or the pair + # proves nothing about the split. + expect.at_least(job.count("pip show psycopg"), 1, + "and asserts the driver IS present, which is what makes it the other half") + + # It must not list the corpus either way round: neither the files it runs nor the + # files it does not. + hardcoded = [n for n in NO_CLUSTER if n in job] + expect.text(", ".join(hardcoded) or "none", "none", + "the cluster job names no database-free file literally") + + +def test_both_pytest_jobs_assert_how_many_tests_they_collected(expect): + """A run that collects fewer tests than it should is a green that means nothing (#1016). + + `--pgc-expect-tests` existed and nothing passed it. What it closes is narrower than + "pytest passed having run nothing" and worse: a nonexistent path already fails on its + own, but a file list that resolves to REAL files and collects FEWER tests does not. + Measured by dropping one file from the guard list: + + unarmed rc=0 "255 passed" 17 tests gone, silently + armed rc=4 "collected 255 test(s) but expected 272" + + THE NUMBERS LIVE IN A TRACKED FILE, for the reason check_ledger_budget.txt gives about + its own: a change to one is then a diff a reviewer sees, next to the test that moved + it. PGC_SKIP_TIMING is the precedent for the alternative -- set in two workflow files, + suppressing whole suites for months, with no diff ever showing it. + """ + ci = (HERE.parent.parent / ".github" / "workflows" / "ci.yml") + text = ci.read_text() + counts = HERE / "expected_tests.txt" + expect.text(repr(counts.is_file()), "True", + "premise: the expected counts are in a tracked file") + + nums = {} + for line in counts.read_text().splitlines(): + f = line.split() + if len(f) == 2 and f[1].isdigit() and not line.startswith("#"): + nums[f[0]] = int(f[1]) + for key in ("guard_tests", "cluster_tests"): + expect.at_least(nums.get(key, 0), 1, + f"{key} is named and positive, or the flag it feeds asserts nothing") + + # BOTH jobs, not one. Arming half of them would leave the other able to collect + # nothing and pass, which is the state this closes. + expect.num(text.count("--pgc-expect-tests"), 2, + "both pytest jobs pass the flag") + expect.num(text.count("expected_tests.txt"), 2, + "and both read the number from the tracked file rather than stating it") + # An empty read would OMIT the flag and fail open, so the value is guarded in the job. + expect.num(text.count('test -n "$WANT"'), 2, + "and each guards the read, because an empty value would fail open") + + def test_the_job_installs_no_database_driver(expect): """The job's value is that it runs where there is no database. From 318cc8a393175ee557f3bd1c9d53df2c505e2f48 Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sat, 12 Sep 2026 12:21:35 -0600 Subject: [PATCH 2/5] ci+test: a pytest cluster that will not start now says why (#1016) The cluster job's real failure, once the privilege problem jdatcmd diagnosed was fixed: the cluster does not start, and the harness does not say why. RuntimeError: ['pg_ctl', '-D', '/tmp/pgc-pytest-0-.../data', ..., 'start'] failed rc=1: pg_ctl: could not start server Examine the log output. Fifty identical errors, every one naming the command, none naming the cause -- because `pg_ctl` told the harness to examine a log and nothing examined it. The other 50 tests in those files passed, which is what made it legible at all: they are the static arms that need no cluster. lib.sh has had pgc_start_log_report since #537 for exactly this. The pytest side had no equivalent. The two harnesses are parallel in FUNCTIONALITY and never in call, so this is that function's job written on this side: FATAL lines with their line numbers, then a tail, and it says so when it found neither, because silence reads as "there was nothing to say". The chown from the previous commit DID work -- the build and install succeeded and 50 tests ran. This is a second, different problem that the first one was hiding, and I would rather add the instrument than guess at it a third time: I have already disproved one hypothesis today (a non-assert build, which passes 101/101 locally on pg18n) and read the code wrongly twice. --- CHANGELOG.md | 12 ++++++++++ test/pytest/pgc_cluster.py | 48 +++++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8152cce9..865de483 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -146,6 +146,18 @@ true until the next version shipped. Field 5 because the entry above inserted the majors as field 4. At the moment this change landed on its own it was field 4; both ship in the same release, so the form here is the one that works on the shipped tree. +- A pytest cluster that will not start now says why (#1016). + + `pg_ctl` prints "Examine the log output." and nothing examined it, so a cluster that + failed to start produced fifty identical errors naming the COMMAND and not one naming + the cause -- measured on a GitHub runner, fifty `pg_ctl: could not start server` and the + reason sitting in a file nobody read. `lib.sh` has had `pgc_start_log_report` since #537 + for exactly this; the pytest harness had no equivalent, and the two are meant to be + parallel in functionality. It reports the FATAL lines with their line numbers, then a + tail, and says so explicitly when it found neither -- silence reads as "nothing to say", + which was #537's whole complaint. Written on this side rather than called across the + boundary, because the harnesses stay independent. + - The 93 pytest tests that needed a cluster now run in CI, and both pytest jobs assert how many tests they collected (#1016). diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index edf5aa56..182deabd 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -264,9 +264,22 @@ def initdb(self): ) def start(self): - _asroot(["pg_ctl", "-D", str(self.datadir), "-l", - str(self.datadir / "server.log"), "-w", "start"], - self.bindir, self.datadir) + log = self.datadir / "server.log" + try: + _asroot(["pg_ctl", "-D", str(self.datadir), "-l", str(log), "-w", "start"], + self.bindir, self.datadir) + except RuntimeError as e: + # `pg_ctl` says "Examine the log output." and then nothing examined it, so a + # cluster that would not start produced fifty identical errors naming the + # command and not one naming the cause. Measured on a GitHub runner: fifty + # errors, every one of them `pg_ctl: could not start server`, and the reason + # was in a file nobody read. + # + # lib.sh has had pgc_start_log_report since #537 for exactly this, and the + # two harnesses are meant to be parallel in FUNCTIONALITY. This is that + # function's job on this side, written here rather than called across the + # boundary. + raise RuntimeError(f"{e}\n{_start_log_report(log)}") from e self._started = True def stop(self): @@ -564,6 +577,35 @@ def _run(argv, check=True): return proc.stdout +# What the server log says about a cluster that would not start. +# +# FATAL lines first, with their line numbers, then a tail, and it SAYS SO when it found +# neither: silence here reads as "there was nothing to say", which was the whole complaint +# in #537. lib.sh's pgc_start_log_report is the same function on the other side; neither +# calls the other, because the harnesses stay independent. +_START_FATAL = re.compile(r"FATAL|PANIC|could not|No space|Permission denied", re.I) + + +def _start_log_report(log, fatal_lines=5, tail_lines=20): + try: + text = pathlib.Path(log).read_text(errors="replace") + except OSError as e: + return f"---- server log unreadable at {log}: {e} ----" + if not text.strip(): + return f"---- server log absent or empty at {log} ----" + lines = text.splitlines() + fatal = [f" {n}: {l}" for n, l in enumerate(lines, 1) if _START_FATAL.search(l)] + out = [] + if fatal: + out.append("---- why the cluster would not start ----") + out.extend(fatal[:fatal_lines]) + else: + out.append("---- no FATAL in the server log; its tail follows ----") + out.append(f"---- server log tail ({log}) ----") + out.extend(f" {l}" for l in lines[-tail_lines:]) + return "\n".join(out) + + def _asroot(argv, bindir, datadir, check=True): """Run a server binary, dropping to postgres when we are root. From d9a685afad0e01c0b712199a4a5d796b21fa7f5e Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sat, 12 Sep 2026 12:29:51 -0600 Subject: [PATCH 3/5] test: pin the cluster's socket directory, which a packaged postgres defaults elsewhere (#1016) The cluster job's actual cause, named by the diagnostic added in the previous commit rather than by another guess: FATAL: could not create lock file "/var/run/postgresql/.s.PGSQL.29768.lock": Permission denied Measured, same box, same major, two builds: /usr/lib/postgresql/18 (PGDG, configured --runstatedir=/run) #unix_socket_directories = '/var/run/postgresql' /usr/local/pg18a (source build, no such flag) #unix_socket_directories = '/tmp' and /var/run/postgresql is drwxrwsr-x postgres postgres. So on any packaged postgres the postmaster cannot create its lock file and FATALs, and the caller saw only `pg_ctl: could not start server`. lib.sh does not pin this and does not need to: the suites job runs under sudo -E and lib.sh drops to `runuser -u postgres`, which CAN write that directory. This harness must be non-root throughout -- initdb refuses root -- and is not postgres either, so the packaged default is wrong for it and always was. It only ever ran against source builds, where the default is /tmp. Pinned to the cluster's own datadir: it already exists, it is already this cluster's, it goes away with it, and two xdist workers cannot collide in it. Connections are TCP (host=127.0.0.1), so nothing that connects is affected. REPRODUCED THE RUNNER LOCALLY to find this, because running as root proves nothing: _asroot drops to postgres when euid is 0, and postgres can write that directory, so the whole problem disappears. A non-root, non-postgres user against the PACKAGED build gave CI's exact shape -- 1 failed, 50 passed, 50 errors. --- test/pytest/pgc_cluster.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 182deabd..8f53b112 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -249,6 +249,31 @@ def initdb(self): "", f"port={self.port}", "listen_addresses='127.0.0.1'", + # THE SOCKET DIRECTORY, pinned to this cluster's own datadir. + # + # A PACKAGED POSTGRES DEFAULTS IT SOMEWHERE THIS USER CANNOT WRITE, + # and that is why the CI job could not start a cluster at all. + # Measured, same box, same major: + # + # /usr/lib/postgresql/18 (PGDG, --runstatedir=/run) + # #unix_socket_directories = '/var/run/postgresql' + # /usr/local/pg18a (source build, no such flag) + # #unix_socket_directories = '/tmp' + # + # and /var/run/postgresql is drwxrwsr-x postgres postgres. So the + # postmaster cannot create its lock file and FATALs, which reaches + # the caller as nothing more than `pg_ctl: could not start server`. + # + # lib.sh does not pin this and does not need to: the suites job runs + # under sudo and lib.sh drops to `runuser -u postgres`, which CAN + # write that directory. This harness must be non-root throughout -- + # initdb refuses root -- and is not postgres either, so the default + # is wrong for it on any packaged build. + # + # The datadir rather than /tmp: it already exists, it is already + # this cluster's, it goes away with it, and two xdist workers cannot + # collide in it. + f"unix_socket_directories='{self.datadir}'", "shared_preload_libraries='pgcolumnar'", # Deterministic output so a hash oracle means the same thing # on every machine. lib.sh sets the same three. From 7328551bd70da17b21df5174ed6ef643b58d970e Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sat, 12 Sep 2026 12:32:49 -0600 Subject: [PATCH 4/5] test: the shim subprocess must inherit the pg_config under test (#1016) test_a_cluster_test_still_needs_the_driver runs pytest in a subprocess with psycopg shimmed out and asserts the failure NAMES the shim. The subprocess got no --pg-config, so it fell back to conftest's DEFAULT_PG_CONFIG -- /usr/local/pg18a/bin/pg_config -- which exists on the audit container and on no GitHub runner. The child then failed on the missing prefix BEFORE anything imported psycopg, so the output never named the shim and the arm reported the shim absent when the shim was fine. Its own premise arm passed throughout, because the shim really did work: the premise and the assertion were about different processes. A latent bug rather than a CI quirk. It passes against a source-built prefix and fails against a packaged one, and nothing in the test said which it needed. --- test/pytest/expected_tests.txt | 14 +++++++++----- test/pytest/test_harness_deps.py | 20 ++++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 34f15583..bcd2828a 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -36,11 +36,15 @@ # test_mutation_ledger.py. That is the mechanism doing its job rather than a nuisance: had # it not moved, the job would have failed with "collected 274 test(s) but expected 272" and # named the drift instead of running a different suite than the one declared. -guard_tests 274 +guard_tests 277 # 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 -# silent when they stopped. The number below moved from 99 to 101 in that same change, -# because it added two arms about the jobs, which is the mechanism working: a test added is -# a number moved, in a diff next to the test. -cluster_tests 101 +# silent when they stopped. +# +# This number has moved twice already while the change was in flight: 99 -> 101 when this +# PR added two arms about the jobs, and 101 -> 106 when #1012 merged test_join_vector_agg.py +# into this half. Both times the number moved BEFORE CI ran, because re-deriving it is part +# of rebasing; had it not, the job would have failed naming the drift rather than running a +# different suite than the one declared. That is the mechanism working, not a nuisance. +cluster_tests 106 diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index ec565405..f3e8b806 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -605,9 +605,16 @@ def _main(argv): return 0 -def _run_without_psycopg(args, expect): +def _run_without_psycopg(args, expect, pg_config=None): """Run pytest with `import psycopg` forced to fail, and return the result. + THE SUBPROCESS GETS THE SAME pg_config THIS RUN WAS GIVEN (#1016). Without it the + child falls back to conftest's DEFAULT_PG_CONFIG, `/usr/local/pg18a/bin/pg_config`, + which exists on the audit container and on no GitHub runner. The cluster fixture then + fails on the missing pg_config BEFORE anything imports psycopg, so the output never + names the shim and the arm below reports the shim absent when the shim was fine. + Measured: passes against a source-built prefix, fails against a packaged one. + A SHIM RATHER THAN AN UNINSTALL. Uninstalling psycopg would test the machine rather than the harness, cannot run concurrently with anything else, and leaves the environment broken if the test dies. A module that raises on @@ -621,8 +628,9 @@ def _run_without_psycopg(args, expect): ) env = dict(os.environ) env["PYTHONPATH"] = shim + os.pathsep + str(HERE) + extra = ["--pg-config", pg_config] if pg_config else [] proc = subprocess.run( - [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", *args], + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", *extra, *args], cwd=str(HERE), env=env, capture_output=True, text=True, ) # PREMISE: the shim must actually bite, or this arm proves nothing at all. @@ -651,7 +659,7 @@ def test_the_guard_half_of_the_corpus_runs_without_a_database_driver(expect): % (len(NO_CLUSTER), out.strip().splitlines()[-1])) -def test_a_cluster_test_still_needs_the_driver(expect): +def test_a_cluster_test_still_needs_the_driver(expect, pytestconfig): """THE CONTROL, and without it the arm above is satisfied by a corpus that connects to nothing at all. @@ -659,7 +667,11 @@ def test_a_cluster_test_still_needs_the_driver(expect): IMPORT lazy. A test that actually wants a connection must still fail when the driver is gone, and it must fail for that reason rather than by being skipped. """ - proc = _run_without_psycopg(["test_connection.py"], expect) + # The pg_config THIS run was given, not conftest's default: see + # _run_without_psycopg. Without it the child dies on a missing prefix before it can + # reach the import, and this arm then reports the shim absent. + proc = _run_without_psycopg(["test_connection.py"], expect, + pg_config=pytestconfig.getoption("--pg-config")) expect.at_least(proc.returncode, 1, "a cluster test cannot pass without the driver") expect.at_least( From 0c4a939c57351d49c04273fbb60ede17dd8080e6 Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Sat, 12 Sep 2026 13:07:51 -0600 Subject: [PATCH 5/5] test: the build lock is per-user, because a fixed /tmp path is a permanent denial (#1016) Not a collision -- a kernel-enforced lockout. `fs.protected_regular = 2`, the default on this kernel, forbids opening a regular file for write in a world-writable STICKY directory when the file's owner is neither the directory's owner nor the caller. So once one user creates /tmp/pgc-pytest-build.lock, every other user on the box cannot run the corpus again, and neither can root: running as: root uid=0 lock: -rw-r--r-- 1 ciuser ciuser 0 /tmp/pgc-pytest-build.lock PermissionError: [Errno 13] Permission denied: '/tmp/pgc-pytest-build.lock' CAP_DAC_OVERRIDE does not help, which is what makes it surprising. It cost two runs before I read the sysctl -- it bit while reproducing the CI environment as a second user, then again on the next root run. The DIRECTORY carries the uid, not the filename: a per-user directory is owned by that user and is not world-writable, so protected_regular does not apply inside it. A per-user FILENAME would still be a file in a sticky shared directory. WHAT THIS GIVES UP, said out loud: the lock no longer serialises two DIFFERENT users installing into one shared prefix. That is already covered, and better, by the marker key -- it includes installed_library(pg_config) from #956, so another user's install invalidates this user's marker and forces a rebuild rather than being silently accepted. The lock's job is the xdist-worker race within one run, and workers share a uid. --- CHANGELOG.md | 13 +++++++++---- test/pytest/expected_tests.txt | 15 +++++++++------ test/pytest/pgc_cluster.py | 31 +++++++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 865de483..3ec129fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -158,8 +158,13 @@ true until the next version shipped. which was #537's whole complaint. Written on this side rather than called across the boundary, because the harnesses stay independent. -- The 93 pytest tests that needed a cluster now run in CI, and both pytest jobs assert - how many tests they collected (#1016). +- The pytest tests that need a cluster now run in CI, and both pytest jobs assert how many + tests they collected (#1016). + + **166 collected tests in 9 files**, counted on this tree: 93 when the gap was filed, plus + #1012's `test_join_vector_agg.py` and #1020's `test_differential.py`, which landed into the + ungated half while this change was in review. That is the argument for the change rather + than a detail about it -- the half was growing faster than it was being gated. `ci.yml` had one pytest job, `pytest-guards`, and it installs psycopg deliberately NOT -- that absence is what proves those files need no database. `nightly.yml` mentions @@ -168,8 +173,8 @@ true until the next version shipped. call the project forbids. So 8 files and 93 test functions, 26% of the corpus, ran nowhere: green when somebody ran them by hand, silent when they stopped. - They were never broken. Measured on `pg18a` with the driver present: 99 collected, - 289 checks, 289 pass, 40 seconds. + They were never broken. Measured on `pg18a` with the driver present, at the time the gap + was filed: 99 collected, 289 checks, 289 pass, 40 seconds. Nothing ran them. `--pgc-expect-tests` is now passed by BOTH jobs, from `test/pytest/expected_tests.txt`. The flag existed and nothing used it. What it closes is narrower than "pytest passed diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index bcd2828a..f1da3f3e 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -42,9 +42,12 @@ guard_tests 277 # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and # silent when they stopped. # -# This number has moved twice already while the change was in flight: 99 -> 101 when this -# PR added two arms about the jobs, and 101 -> 106 when #1012 merged test_join_vector_agg.py -# into this half. Both times the number moved BEFORE CI ran, because re-deriving it is part -# of rebasing; had it not, the job would have failed naming the drift rather than running a -# different suite than the one declared. That is the mechanism working, not a nuisance. -cluster_tests 106 +# This number has moved FOUR times while the change was in review: 99 -> 101 when this PR +# added two arms about the jobs, 101 -> 106 when #1012 merged test_join_vector_agg.py into +# this half, and 106 -> 166 when #1020 merged test_differential.py into it. Every move was +# re-derived during a rebase; had one been missed, the job would have failed naming the drift +# rather than running a different suite than the one declared. +# +# The rate is the point. The ungated half grew by 67 tests in the time this took to review, +# which is the argument for gating it rather than a detail about it. +cluster_tests 166 diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index 8f53b112..a5d00a33 100644 --- a/test/pytest/pgc_cluster.py +++ b/test/pytest/pgc_cluster.py @@ -536,8 +536,35 @@ def build_once(srcdir, pg_config, major, lock_path=None, runner=None): reintroduce the defect for anyone who runs the corpus twice against two majors. """ - lock_path = lock_path or os.path.join( - tempfile.gettempdir(), "pgc-pytest-build.lock") + # A PER-USER DIRECTORY, because a fixed path in /tmp is not a collision, it is a + # permanent denial. `fs.protected_regular = 2` (default on this kernel) forbids opening + # a regular file for write in a world-writable STICKY directory when the file's owner is + # neither the directory's owner nor the caller -- so once one user creates + # /tmp/pgc-pytest-build.lock, every other user on the box is locked out of the corpus + # FOREVER, and so is root: + # + # running as: root uid=0 + # lock: -rw-r--r-- 1 ciuser ciuser 0 /tmp/pgc-pytest-build.lock + # PermissionError: [Errno 13] Permission denied + # + # Measured after running the corpus as one user and then as another; it cost two runs + # before I read the sysctl. CAP_DAC_OVERRIDE does not help, which is what makes it + # surprising. + # + # The DIRECTORY carries the uid, not the filename: a per-user directory is owned by that + # user and is not world-writable, so protected_regular does not apply inside it at all. + # A per-user FILENAME in /tmp would still be a file in a sticky shared directory. + # + # WHAT THIS GIVES UP, said out loud: the lock no longer serialises two DIFFERENT users + # installing into one shared prefix. That is already covered, and better, by the marker + # key -- it includes `installed_library(pg_config)` (#956), so another user's install + # invalidates this user's marker and forces a rebuild rather than being silently + # accepted. The lock's job is the xdist-worker race within one run, and workers share a + # uid. + if lock_path is None: + lock_dir = os.path.join(tempfile.gettempdir(), f"pgc-pytest-{os.getuid()}") + os.makedirs(lock_dir, mode=0o700, exist_ok=True) + lock_path = os.path.join(lock_dir, "build.lock") marker = lock_path + ".done" # THE FINGERPRINT IS PART OF THE KEY. Keying on pg_config and major alone # would skip the build after a source edit, which is the staleness this