Repository navigation
docs: name all four registries a new pytest test must reach - #1279
Conversation
8287ee3 to
e841ce1
Compare
OffgridwithJD
left a comment
There was a problem hiding this comment.
Reviewed at e841ce1f. I ran the claims rather than reading them, and there is
one thing to fix before this merges: the command does not work as written.
Blocking: python is not on PATH
G="$(python -c 'import sys;sys.path.insert(0,".");from test_harness_deps import NO_CLUSTER;...')"In pgcolumnar-dev:
bash: line 5: python: command not found
There is no python, only python3. In a section whose entire value is "here
is the command to run", a reader's first action produces a shell error rather
than a result — and it is the reader least likely to know what to substitute,
since they are here because CI told them to be.
The same command with python3 works, and I ran it end to end:
files: 24
guard_tests wanted: 404
checks run: 1131
accounting: 1131 pass + 0 fail + 0 unrun = 1131
404 passed in 14.31s
Worth also saying which pytest — a bare pytest is not on PATH here either;
CI and I both use /tmp/pgcvenv/bin/pytest. If you would rather keep it
executable-agnostic, say python3 -m pytest and the reader's virtualenv decides.
Everything else checked out, including the part I expected to push back on
Parametrization really is a reason here, not a borrowed one. I went looking
for it because "a new test function is not reliably +1" reads like general
pytest advice, and general advice in a repo-specific doc is usually someone
else's experience. It is ours:
files using @pytest.mark.parametrize: 13
test_hilbert_locality.py:742 @parametrize("box", sorted(PINS))
test_layer.py:452 @parametrize("mode", ["serial", "xdist"])
So a collected count can move by more than one from a single function, and the
argument stands on this corpus.
The quoted string is right, from source. pgc_vacuity.py:1528 is
f"unrunnable on PG{major}, and not expected to be:\n", and EXIT_INCOMPLETE = 67 at :226. You flagged that you had first quoted my paraphrase of the output
rather than the source — catching that in a section that says "read the names,
not the exit code" is the right instinct, and the corrected quote is what a
reader can actually grep for.
test_docs_cover_the_corpus.py exists under that exact name.
The ledger clause matches what the gate does: covered suites only, new rows
arrive never so the budget moves, one log per gated major in a single merge.
The ICU paragraph is accurate to what I measured
One nuance, and it strengthens rather than weakens it. My first local run
exited 67 on a tree that did have something wrong — my unregistered arm.
The run your sentence describes is the second, after registering, where
nothing was wrong and it still exited 67 naming the ICU entry. Your wording says
"on a tree where nothing is wrong", which is that second run, so it is right as
written. I mention it only because the two runs are easy to conflate and the
second is the one that makes the point.
One thing I would add, and would not block on
The four registries are listed in the order CI happens to find them. The order a
reader needs is the order they can act in — 1 and 2 before pushing, 4 before
pushing if the test declines anywhere, 3 only after a suite run. That is exactly
what the "which of those a local run catches" section says, so the information
is present; it is placement, not content.
Approving once the python3 fix is in. Happy to re-review at whatever head that
lands on.
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
e841ce1 to
0b804c7
Compare
OffgridwithJD
left a comment
There was a problem hiding this comment.
Re-reviewed at 0b804c77. The blocker is gone and I ran the block verbatim from
this head rather than checking that the string changed.
NO_CLUSTER files: 24
checks run: 1131
accounting: 1131 pass + 0 fail + 0 unrun = 1131
404 passed in 14.21s
Your fix is better than the one I suggested, and for a reason I should have
given myself. I proposed python3, which is portable. You used
/root/pyenv/bin/python, which is what this document's own Prerequisites builds
at line 21 and what every other invocation in it already uses. A reader
following the file has that path; a reader who has python3 also has the venv.
Consistency with the document beats portability inside the document.
Verified it exists rather than assuming the Prerequisites ran:
/root/pyenv/bin/python
/root/pyenv/bin/pytest
Every invocation in the file now carries the venv path. Swept it:
29, 30, 31, 32 /root/pyenv/bin/pytest
47, 120, 121 /root/pyenv/bin/python, /root/pyenv/bin/pytest
remaining bare matches: 3, all PROSE
"Running the pytest harness"
"ways a pytest harness can report a false ..."
"the shell runner invoking pytest is the cross-harness call"
Which is the distinction your own guard had to learn: in a documentation file a
mention is not an invocation, and a guard that cannot tell them apart gets
switched off.
The --pgc-expect-tests 24 defect is the better find and I would not have got there
before: PYTHONPATH=. /root/pyenv/bin/pytest --pgc-expect-tests 24
after: PYTHONPATH=. /root/pyenv/bin/pytest --pgc-expect-tests N
24 is len(NO_CLUSTER), the file count, in a flag that compares
len(session.items), the collected test count — 404 for that set. A literal
wrong quantity, in the one flag whose entire purpose is that a number means what
it says, in the block a reader copies first. It had nothing to do with my
blocker; my blocker only sent you to read the whole file.
Replacing it with N plus a sentence naming the quantity is right: any literal
there goes stale the next time anyone adds a guard test, and the section
directly below now says to collect rather than add up.
Approving
Everything I checked last round still holds at this head — the parametrize
justification is this corpus's (13 files), the quoted refusal string matches
pgc_vacuity.py:1528, EXIT_INCOMPLETE = 67 at :226, and
test_docs_cover_the_corpus.py exists under that name.
My one non-blocking note stands and I am not asking for it: the four registries
are listed in the order CI finds them rather than the order a reader can act in,
and the "which of those a local run catches" section already supplies what that
ordering would have.
Names the four hand-maintained registries a new pytest test has to be entered in,
and which of them a local run can catch.
Why it is worth a section
Adding one test reddens CI from four separate places, none of which mentions the
others, 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. So "I fixed the registry" never meant the change was finished.
The two caveats that needed measuring
expected_tests.txtsplits byNO_CLUSTER, so putting the count in thewrong key is a silent miss: the other job's count still matches. And the number
has to be collected, not added to — parametrization means a new test function
is not reliably
+1, and a rebase where both sides moved the key can make thearithmetic agree with the truth by accident. Both of those happened.
expected_unrunnable.txtcannot be checked by running the major where the testdeclines. 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:
So the section says to read the names the layer prints under
unrunnable on PG<major>, and not expected to be:, never the exit code.@OffgridwithJD measured that and corrected the weaker rule this section would
otherwise have carried, which was "run the major where it declines".
Scope
Documentation only.
test/pytest/README.mdis outsidedocs_style's scope —that checks
docs/*.mdand the top-levelREADME.md— so the section is held tothe same plain-language rules by hand rather than by a gate, and carries no em or
en dashes.
I verified the quoted message against the source rather than against a transcript
of it:
pgc_vacuity.py:1528emitsunrunnable on PG{major}, and not expected to be:andEXIT_INCOMPLETEis 67.My first draft quoted a paraphrase that would not have been findable by grep.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP