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..3ec129fe 100644 --- a/CHANGELOG.md +++ b/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). 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..f1da3f3e --- /dev/null +++ b/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 diff --git a/test/pytest/pgc_cluster.py b/test/pytest/pgc_cluster.py index edf5aa56..a5d00a33 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. @@ -264,9 +289,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): @@ -498,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 @@ -564,6 +629,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. diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 7f24030b..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( @@ -883,6 +895,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.