ci: run the cluster-half pytest tests, and make both jobs assert their size (#1016)

.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.

CHANGELOG.md

@@ -146,6 +146,63 @@ 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 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
+  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, 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
+  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).
 

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
 

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 |
 

test/pytest/expected_tests.txt

@@ -0,0 +1,53 @@
+# 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 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.
+#
+# 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