Repository navigation
ci(toon): pin the toon reference to the last pre-4.1 commit and require it - #185
Conversation
a6dd8b8 to
f5ecd03
Compare
There was a problem hiding this comment.
Reviewed for the pin, which is what this PR turns on. The mechanics are good — dropping the || true, making the reference install a required step, the tomli/tomllib fix for 3.10, the per-extra fallback and -rs are all the right shape. Four things on the pin and its acceptance test.
1. The pin is pre-4.1 and goes red the moment B lands. bench-toon pins 3c5e623. Against B's suite, measured:
toon_format @ 3c5e623 -> 6 failed, 114 passed
The failures are precisely B's changed expectations (test_empty_array, test_empty_array_as_object_value, test_nested_tabular, test_mixed_nested_arrays). The ordering rationale in the plan is therefore correct, and this pin has to move with B.
2. Which commit. Both candidates are green against B:
| reference | B's suite |
|---|---|
v1.0.0 6034550 (fixtures 4.1.2) |
120 passed |
v1.1.0 4814d77 (adds 4.2.1) |
120 passed |
Neither is red, so "take the one that is green" needs a tiebreak. I would take 6034550: B declares 4.1, and pinning the newest green would let 4.2.1-era expectations in under a 4.1 claim. Pin the spec the module claims; raise it deliberately later.
3. The comment beside the pin is now wrong. It reads "toon/ implements the pre-4.0 encoding, so the pin has to name that state". After B, toon/ implements 4.1. Pin and comment have to move together, or the comment misleads whoever touches this next.
4. The second acceptance check cannot pass as written. "point the pin at main once, confirm the job goes red" — main (91413d1, spec 4.4) is green against B's suite (120 passed). Pinning to main will produce a green job, so that check proves nothing. Use a reference that genuinely fails instead: e475c82 or 3c5e623 both give 6 failed, which demonstrates the job can go red.
The hole itself is real, and worth showing in the PR that closes it. With toon_format absent:
$ pytest toon/test_toon_correctness.py -q
1 skipped in 0.02s (exit 0)
That is exactly how the job passed while comparing nothing. The required install step is the right fix; -rs (already added) is what makes a future skip visible in the log rather than silent.
Nit. The required step uses pip install -e ".[bench-toon]" in the test job, which until now installed no editable copy of the project — only the lint job does. Probably harmless, but it is a new element in that job's environment, so worth confirming collection is unaffected on all four interpreters.
Requesting changes only for (1)+(3) (the pin and its comment), and (4) because the acceptance test as specified cannot demonstrate what it is meant to.
f5ecd03 to
6af87da
Compare
6af87da to
2eb6632
Compare
|
All five were measured on the previous head ( 1. The pin was pre-4.1. Fixed, and you are right that it had to move with B. I measured the same thing at 2. Which commit — changed to 3. The comment beside the pin. Rewritten with the pin. It no longer claims the module is pre-4.0; it states the spec, why that level is the one pinned, and that the 4.4 gap is #184. 4. The acceptance check. Same finding, from the same measurement — main is green against B's suite, so pinning there proves nothing. I took your suggestion and used Nit — collection on the four interpreters. Confirmed, not by inspection: the previous head ran all four jobs green with One more number for the channel, since it comes from the same measurement and bears on #183 closing here: what B's suite proves and what the module conforms to are different things. Conformance is 357/564 on B's branch against 341/564 on master — B's 120 green comparisons sit nowhere near that gap. Updated on #184. |
|
Confirmed the amended pin, as promised, and there is one operational thing to know for the stacked workflow. The pin at the amended head. The stacked head gets no
Same counts as at 1.1.0 — the reference version does not move the suite. Once the base retargets to master after #186 lands, the normal Worth knowing for the rest of the plan: while A is stacked on B, anything you want CI-verified on A needs |
9faed67 to
2dfb5b7
Compare
There was a problem hiding this comment.
Re-reviewed at 2eb6632e. All four points from my earlier review are resolved, and I verified the fixes rather than taking the description's word for them.
Verified independently
- The pin resolves:
6034550ceec432adba37480c82839d8428a80361is exactlyv1.0.0^{commit}upstream — not a typo or a short SHA that happens to resolve locally. .github/workflows/lint-test.ymlparses as YAML, and the required reference step and the-rsflag are present in the test job.- The comment beside the pin now names the spec and the tiebreak instead of claiming
toon/implements the pre-4.0 encoding. That was the part that would have misled the next person. - The acceptance check is corrected in the description with the reason recorded, which is the outcome I asked for:
mainis green against #186's suite, so pointing the pin at it cannot produce red. Substituting the reference going missing (assertion → exit 1) plus a wrong pin going red (3c5e623→ 6 failed) closes both halves.
The tomli/tomllib fallback, the dropped || true, and the per-extra fallback all look right, and the 3.10 item count moving from 2658 to 7545 is the evidence that its extras now actually install.
Nit (non-blocking). The comment on the required step reads "so a local pip install -e ".[dev]" matches what runs here", but the step runs pip install -e ".[bench-toon]" and .[dev] does not pull the extra. Reading [bench-toon] there would make the comment say what the code does.
One thing to refresh on rebase (no action needed now). #186 was force-pushed to 2dfb5b7 while this branch sat on 9faed67, so this head is now behind B's head, and B's toon suite went from 120 tests to 129 (brace-in-key and triple-nesting regressions added). The "120 passed" rows in the description become 129 once this rebases onto B/master. The pin itself is unaffected — it is still green against all three references at 129.
Approving. The remaining pin/CI mechanics are sound, and the spec-level gap stays correctly scoped to #184.
…e it The test job installed `toon_format` unpinned from git main, behind `|| true`, with the comparisons skipping when the import failed. The reference could move or vanish without the job noticing, which is how master stayed red from 2026-09-28 while the suite read as skipped rather than failed (issue #183). The reference now installs in its own required step, asserted importable, and `bench-toon` pins it to a commit, so `pip install -e ".[bench-toon]"` resolves what CI resolves. The pin names the spec `toon/` declares it implements -- toon-format 1.0.0 (`6034550c`, fixtures 4.1.2) -- not the newest reference the suite happens to pass against. 120 passed on 1.0.0, on 1.1.0 (`4814d77f`, spec 4.2) and on main (`91413d14`, spec 4.4), so the green result does not pick between them and taking the newest would let 4.2-era semantics in under a 4.1 claim. The pin names the claimed level; the 4.4 conformance gap this suite does not measure is #184. A pin the suite does fail against goes red -- at the pre-4.1 commit `3c5e623` it is 6 failed, 114 passed. Also repairs the 3.10 job, which collected 2658 items against 7569 on 3.11. `tomllib` is 3.11+, so `import tomllib` raised there and BENCH_EXTRAS came out empty, leaving `pip install ".[]"` with nothing to install while `continue-on-error` hid it. The script falls back to `tomli`, and a failed combined install retries one extra per `pip install`, so a single extra that cannot resolve cannot take the rest of the reference set with it. A green 3.10 is not evidence the toon comparisons ran; the import assertion in the new step is.
2eb6632 to
6fb9a28
Compare
There was a problem hiding this comment.
Re-approved at 6fb9a28. Verified against master rather than the report.
The rebase is the only change. git diff fdf44c2 6fb9a28 is exactly .github/workflows/lint-test.yml and pyproject.toml; the head is a single linear commit on fdf44c2, base is master, and mergeable: MERGEABLE. Nothing from B rides along, which is the failure mode a plain rebase would have produced: this branch sat on 9faed67, B's pre-force-push head, so replaying it would have re-applied B's superseded toon commit and collided with its final form (400a50c + fdf44c2). --onto 9faed67 was the right call.
The pin works end-to-end, not just as a string. In a clean venv at this head:
$ pip install -e ".[bench-toon]"
Successfully installed toon-format-1.0.0 zerodep-2026.9.28
$ python -c "…direct_url.json…"
__version__: 1.0.0
requested_revision: 6034550ceec432adba37480c82839d8428a80361
6034550ceec4… is byte-exact v1.0.0^{commit}, so the extra installs the same commit CI does. Then pytest toon/test_toon_correctness.py → 129 passed, and zerodep.py version-check --strict → all modules up to date.
CI on this head: lint plus 3.10–3.13 all pass, and the evidence that the hole is closed is in the log rather than the colour:
- every one of the four jobs prints
toon_format 1.0.0, so the pinned reference resolved on each interpreter; - 3.10 goes from 2658 collected to 7554, which is the extras the
tomllibfix restored — the old 3.10 was green precisely because it compared nothing; - 3.11/3.12/3.13: 7513 passed, 34 skipped, 7 xfailed. 3.10: 7511 passed, 36 skipped, 7 xfailed.
Description numbers are stale (not blocking, but they are the PR's evidence record and a reader will take them at face value). It still says 120 passed in ten places and lists 7502/7504 for the jobs; it is now 129 toon tests and 7511/7513, with 7554 items collected on every job. The 120 | 7504 rows also predate B's final form. Worth a refresh before merge so the table matches the run it cites.
Previous nit, restated and deliberately waived for this merge so it is not silently dropped: the comment on the required step says "so a local pip install -e ".[dev]" matches what runs here", but the step runs .[bench-toon] and .[dev] does not pull the extra. One-word fix, and it is the one line in the diff that tells a reader to run the wrong command. Closing it as a follow-up rather than holding A for another approval cycle, since the mechanism itself is correct and proven above.
Approving. The pin is the right level, the silent-skip hole is closed on both halves, and the spec gap stays correctly scoped to #184.
What
Stacked on #186, which tracks the current reference API and encoding in
toon/. This is the CI half.The job installed the reference as
— unpinned, and silent when it failed, with the comparisons skipping when the import did not resolve. The reference could move or vanish without the job noticing, which is how master stayed red from 2026-09-28 while the suite read as skipped rather than failed. That is what keeps #182 and #171 red.
bench-toonpins the reference to a commit, sopip install -e ".[bench-toon]"and CI resolve the same thing — the workflow's private install line is what let them driftimport tomllibis 3.11+, so the extraction script raised there,BENCH_EXTRAScame out empty, andpip install ".[]"installed no reference library, all of it hidden bycontinue-on-error: truepytest -rs, so a skip shows up in the summary instead of being inferred from an item countThe pin
6034550c— toon-format 1.0.0, fixtures 4.1.2 — the spec #186 declarestoon/implements.pytest toon/test_toon_correctness.py6034550cv1.0.04814d77fv1.1.091413d14main3c5e623pre-4.1, this branch's first headThree candidates are green, so the green result does not pick between them. The pin names the level the module claims rather than the newest reference it happens to pass against: a pin is a statement about what the module implements, and taking the newest green would let 4.2-era semantics in under a 4.1 claim. Pinning a reference the suite does fail against does go red, which the last row shows. The spec 4.4 gap is #184 — with these 120 comparisons green, conformance is still 357/564 (ENCODE 123/160, DECODE 234/404).
The first head of this branch pinned
3c5e623, the last commit before the 4.1 rewrite, chosen so the stale module's old expectations kept passing. Freezing the reference to hide drift is the thing this PR exists to stop, so it is not the direction.The acceptance check
The plan asked for the pin to be pointed at main once, expecting the job to go red. That check cannot pass as written, and the reason is worth recording:
toon/is green against main too, so pointing the pin there is green rather than red. The drift between what #186 targets and what CI installs is real, but #186's suite does not see it — only the conformance fixtures do.What the hole was, and what closes it, is the reference going missing rather than the reference moving:
A wrong pin also fails loudly, which the
3c5e623row shows at 6 failed. Both halves of the hole are closed and neither is silent any more.The 3.10 job
3.10 was green while 3.11/3.12/3.13 were red, and green because it compared nothing: 2658 items collected against 7569 on 3.11.
With
tomlias a fallback, all four matrix jobs install the reference from thebench-toonpin and run all 120 toon comparisons:The item count on 3.10 goes from 2658 to 7545, which is the extras it used to install nothing of. The install stays in a single
pip install ".[...]"call, with a per-extra fallback that did not trigger in this run; it is there so a future pin that cannot resolve on an older interpreter cannot leave a job with no reference library again.A green 3.10 is not evidence the toon comparisons ran — the number that is evidence is the 120 above, and the
toon_format 1.0.0line the new step prints in every job.Why this PR shows no CI checks
ci.ymltriggers onpull_requestwithbranches: [master], and this PR's base is #186's branch, so the stacked head gets nopull_requestrun. The four rows above were produced withgh workflow run ci.yml --ref fix/toon-reference-pin-183on head2eb6632, which runs the same job set. CI will attach to the PR normally once the base is retargeted to master, after #186 lands.Scope
Lands with #186 to unblock master; #182 and #171 rebase once both are in. It does not make
toon/conform to the current spec — that is #184, and #188 covers two round-trip failures in the list form.Closes #183, refs #184