From 0b804c77b2f4e78419d0eb8d0cde2fc307750842 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Fri, 25 Sep 2026 16:14:18 -0600 Subject: [PATCH] docs: name all four registries a new pytest test must reach Adding one test to test/pytest/ reddens CI from four hand-maintained places, none of which mentions the others, and CI finds them one at a time: expected_tests.txt, TESTS.md, check_ledger.tsv with its budget, and expected_unrunnable.txt. Measured over one day of two people adding tests: four round trips, each failure in a different registry from the one just fixed, so "I fixed the registry" never meant the change was finished. The section says which of the four a local guard run catches and which it cannot. Two caveats that needed measuring rather than assuming: expected_tests.txt splits guard_tests and cluster_tests by NO_CLUSTER, so the wrong key is a silent miss because the other job's count still matches. And the number must be COLLECTED rather than added to: parametrization means a new test function is not reliably +1, and a rebase where both sides moved the key can make the arithmetic agree with the truth by accident. expected_unrunnable.txt cannot be checked by running the major where the test declines. A prefix built without ICU makes a collation test decline where the runner's does not, so the same non-zero exit appears on a tree where nothing is wrong. CI named one entry under exit 67 and a local PG17 run named a different one under the same exit. The rule is to read the names in the "unrunnable and not expected to be" list, never the exit code. Reported by @OffgridwithJD, who measured the ICU case and corrected the weaker rule this section would otherwise have carried. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP --- CHANGELOG.md | 30 ++++++++++++++++++ test/pytest/README.md | 72 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d894dc2e..8c735893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,36 @@ true until the next version shipped. ## [Unreleased] +### Changed + +- `test/pytest/README.md` now names all four registries a new pytest test has to + be entered in, and which of them a local run can catch. Adding one test reddens + CI from `expected_tests.txt`, `TESTS.md`, the ledger plus its budget, and + `expected_unrunnable.txt` separately, and CI reports them one at a time. + Measured over one day of two people adding tests: four round trips, each + failure in a different registry from the one just fixed. The collected count + collided three times; on the third the arithmetic was wrong, because two + branches cut from one base both moved the key to 487 and the rebased tree + collects 488. + + The caveat that needed measuring rather than assuming is on + `expected_unrunnable.txt`. Running the major where a test declines is not + sufficient to check it: a prefix built without ICU makes a collation test + decline where the runner's does not, so the same non-zero exit appears on a + tree where nothing is wrong. CI named one entry under exit 67 and a local PG17 + run named a different one under the same exit, so the section says to read the + names in the `unrunnable and not expected to be` list rather than the exit code. + + The same section's example invocation is corrected while it is being added to. + `--pgc-expect-tests` takes the number of **collected tests**, and the example + passed 24, which is the number of files `NO_CLUSTER` lists. It now takes `N` + with a sentence saying which quantity it is, because a literal there names the + wrong thing and goes stale. Every `python` and `pytest` in the file now carries + the interpreter path the Prerequisites section builds; the first draft of the + new block used bare `python`, which is not on PATH in the container and would + have failed for the reader the section is written for. + Reported by @OffgridwithJD. + ### Fixed - Planning a query over a columnar relation no longer sequentially scans diff --git a/test/pytest/README.md b/test/pytest/README.md index 339473bb..472b953b 100644 --- a/test/pytest/README.md +++ b/test/pytest/README.md @@ -28,10 +28,16 @@ python3 -m venv /root/pyenv cd test/pytest PYTHONPATH=. /root/pyenv/bin/pytest # serial PYTHONPATH=. /root/pyenv/bin/pytest -n 4 # four workers -PYTHONPATH=. /root/pyenv/bin/pytest --pgc-expect-tests 24 # assert the run's shape +PYTHONPATH=. /root/pyenv/bin/pytest --pgc-expect-tests N # assert the run's shape PGC_PG_CONFIG=/usr/local/pg19a/bin/pg_config PYTHONPATH=. /root/pyenv/bin/pytest ``` +`--pgc-expect-tests` takes the number of **collected tests**, not files. `N` above +is whatever the set you ran collects; the two numbers the gate uses are in +`expected_tests.txt`. A literal here would name the wrong quantity and go stale, +which is the mistake the previous text made: it passed 24, the number of files +`NO_CLUSTER` lists, where a test count belongs. + Each worker builds its own throwaway cluster on a port derived from its worker id, and drops it at session end. Nothing survives a run. @@ -64,6 +70,70 @@ Both pass `--pgc-expect-tests` from `expected_tests.txt`, so a run that collects 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. +## Adding a test: four registries, and each one reddens alone + +A new test has to be entered in four hand-maintained places. None of them mentions +the others, and CI finds them one at a time, so "I fixed the registry" does not +mean the change is finished. Measured over one day of two people adding tests: +four round-trips, each failure in a different registry from the one just fixed. + +1. **`expected_tests.txt`** moves, as the section above says. Which key moves + depends on whether `test_harness_deps.py`'s `NO_CLUSTER` names the file: + `guard_tests` if it does, `cluster_tests` if it does not. Putting the count in + the wrong key is a silent miss, because the other job's count still matches. + + **Collect the number, do not add to it.** Parametrization expands one function + into several, so a new test function is not reliably `+1`. Two branches cut + from the same base both move the key, and the survivor's value is neither + branch's: + + ``` + both branches moved cluster_tests to 487, for different tests + the first merged, so 487 became main's value without the second test in it + collected on the rebased tree: 488 + ``` + + This collided three times in one day. The first time `486 + 1` happened to + equal the measured 487 and the agreement was luck; the third time the luck ran + out and the arithmetic was wrong. Run the collection. + +2. **`TESTS.md`** names every file and every test. + `test_docs_cover_the_corpus.py` fails until the new test appears there. Join + the table of the section the file already has rather than opening a new one. + +3. **`check_ledger.tsv` and `check_ledger_budget.txt`**, but only if the suite is + already covered: the gate cannot refuse a check in a suite it has never seen. + New rows arrive as `never`, so `checks_never_observed_red` MOVES and has to + match the census. Regenerate with one log per gated major in a single `merge`, + because a row covers only the majors it was merged from. + +4. **`expected_unrunnable.txt`**, per major. A test that declines for an entirely + correct reason still reddens until its reason is written down for each major + where it declines. The file exists so that an *unexpectedly* unrunnable test + cannot leave a run green. + +### Which of those a local run catches + +From `test/pytest`, the guard half exactly as CI runs it: + +```sh +G="$(/root/pyenv/bin/python -c 'import sys;sys.path.insert(0,".");from test_harness_deps import NO_CLUSTER;print(" ".join(NO_CLUSTER))')" +PYTHONPATH=. /root/pyenv/bin/pytest -q \ + --pgc-expect-tests "$(awk '$1=="guard_tests"{print $2}' expected_tests.txt)" $G +``` + +That catches 1, 2 and the port-parity arm. It cannot catch 3, which needs a suite +run, or 4, which needs the major where the test declines. + +**For 4, read the names and never the exit code.** Running the right major locally +is not sufficient. A prefix built without ICU makes a collation test decline where +the runner's does not, so the same non-zero exit appears on a tree where nothing +is wrong. Measured: CI named one entry under exit 67 and a local PG17 run named a +different one under the same exit. Read the list the layer prints under +`unrunnable on PG, and not expected to be:` (`pgc_vacuity.py`, and the +exit is `EXIT_INCOMPLETE`, 67) and check whether your test is one of the names +in it. + ## Warnings The vacuity layer is loaded through `pytest.ini` and cannot be turned off by a test