test(parity): bidirectional snapshot ratchet for the gap suite (#797)#6755
test(parity): bidirectional snapshot ratchet for the gap suite (#797)#6755TheHypnoo wants to merge 4 commits into
Conversation
…napshot ratchet run_gap_tests.sh gated with `comm -23 <failures> known_failures.json` — fail on any failure not already triaged. That only catches regressions: a listed test that starts PASSING produces no signal, so its entry outlives the bug it tracked and the file decays into orphans nobody can revalidate (#797). - New scripts/gap_snapshot.py generates and checks test-parity/gap_snapshot.json, a committed list of every non-passing gap test. Checked BOTH ways — a new failure is red, and so is a listed test that now passes, which makes an orphaned entry impossible instead of something to audit by hand. Accept the current state with UPDATE_SNAPSHOT=1 ./scripts/run_gap_tests.sh. - Only non-passing tests are stored: "fails and is not listed" already means regression, so listing the ~370 passing tests would quadruple the file without adding a detection. - node_fail/skipped are recorded explicitly rather than dropped from the denominator, so an oracle-coverage change is a visible diff, not a silent hole (a Node 22 pin hid 14 tests that way, #6364). - Statuses only, never program output: the harness truncation marker embeds an exact line count that is not stable across runs (#796). - Shard-safe: check and update act only on tests present in the report, so a --shard N/8 run never judges or rewrites its siblings' entries. Entries are pruned only when the test file is gone, decidable from any shard. - Triage metadata carries across a status change (parity_fail -> crash is the same open bug); a fixed test drops out of the file entirely. - Add test-parity/README.md — the category reference known_failures.json's schema has always pointed at but which never existed (#797 finding). - known_failures.json keeps gating the tag-gated full parity job; its scope is now documented in-file. Migrating that job is a follow-up. - The checker's self-test runs in the lint job, so the logic that decides whether conformance-smoke goes red is verified on the cheap job.
📝 WalkthroughWalkthroughThe gap test gate now compares reports against a committed snapshot, detects regressions and improvements, supports explicit snapshot updates, and validates its comparison logic in CI. Documentation distinguishes the snapshot gate from the legacy parity allowlist. ChangesGap snapshot ratchet
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant RunGapTests
participant ParityRunner
participant GapSnapshot
Developer->>RunGapTests: invoke gap suite with optional shard arguments
RunGapTests->>ParityRunner: run filtered gap tests
ParityRunner-->>RunGapTests: produce test report
RunGapTests->>GapSnapshot: check report against committed snapshot
GapSnapshot-->>RunGapTests: return divergence status
Developer->>RunGapTests: set UPDATE_SNAPSHOT=1 when accepting changes
RunGapTests->>GapSnapshot: update committed snapshot
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/test.ymlTraceback (most recent call last): test-parity/gap_snapshot.jsonTraceback (most recent call last): test-parity/known_failures.jsonTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Folded from the 8 shard reports of this PR's first conformance-smoke run — Ubuntu, Node 26.5.0 per .node-version. Generating it on a macOS/Node 24 laptop would have committed statuses that are wrong for the gate's platform and oracle, so the run had to happen first (that is why the first run was red). Seeding it immediately surfaced 9 node_fail tests that NO existing skip-list entry covered, because the old gate dropped node_fail from the denominator entirely — the #6364 hole, still open: - 6 npm-import tests (exponential-backoff, cron, dayjs, moment, rate-limiter-flexible, slugify): node cannot resolve the import without node_modules, so the oracle never runs. Same class as test_ramda_sum; tracked to #1634 pending a CI-side fixture. - test_gap_4510_enum_forward_ref and test_gap_derived_param_props: node refuses the file outright with ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX — enums and parameter properties cannot run in strip-only mode. These tests have therefore never verified the very features they were written for. The fix is an expected-output fixture, not an edit to the test. - test_gap_prop_plan_cache_invalidation: node throws TypeError "Cannot assign to read only property 'q'" under ESM strict mode; needs triage. Triage for the 8 failures already in known_failures.json is carried over rather than discarded, so no provenance is lost. Every entry has a category and a reason; none are left untriaged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/run_gap_tests.sh`:
- Around line 46-52: Update the run_parity_tests.sh invocation in
run_gap_tests.sh to remove any existing latest.json before execution, capture
its exit status, and only continue for the documented aggregate-parity failure;
propagate every other non-zero status so setup or runtime failures cannot use
stale or partial results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8c4ebd5-068e-4ff9-9acb-64366b999301
📒 Files selected for processing (7)
.github/workflows/test.ymlchangelog.d/6755-gap-snapshot-ratchet.mdscripts/gap_snapshot.pyscripts/run_gap_tests.shtest-parity/README.mdtest-parity/gap_snapshot.jsontest-parity/known_failures.json
| # run_parity_tests.sh exits 1 when AGGREGATE parity < 80%. We gate on the | ||
| # snapshot diff instead (below), so don't let its aggregate exit abort us. | ||
| set +e | ||
| # Forward extra args (notably --shard N/M) so CI can fan the gap suite out | ||
| # across parallel runners; with no args this is the full serial gap suite. | ||
| ./run_parity_tests.sh --filter test_gap_ "$@" | ||
| set -e |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve unexpected parity-runner failures.
The runner exit status is discarded entirely. A setup/runtime failure can therefore proceed against a stale or partial latest.json, and UPDATE_SNAPSHOT=1 can commit that state. Capture the status, remove any prior report before execution, and only tolerate the documented aggregate-parity exit; fail for every other non-zero status.
As per coding guidelines, “When evaluating test or benchmark results, verify the harness exit code directly and do not use binaries from another worktree or stale build artifacts as bisect endpoints or performance baselines.”
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 47-47: set +e (or set +o errexit) disables the shell's errexit option, so the script keeps running after a command fails. This masks failures of security-critical operations (downloads, signature/checksum verification, permission changes, cleanup of secrets), letting the script proceed with a bad or insecure state. Leave errexit enabled (set -e / set -euo pipefail), or handle failures explicitly with if/|| and an explicit exit instead of globally turning off failure detection.
Context: set +e
Note: [CWE-754] Improper Check for Unusual or Exceptional Conditions.
(set-plus-e-error-masking-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run_gap_tests.sh` around lines 46 - 52, Update the
run_parity_tests.sh invocation in run_gap_tests.sh to remove any existing
latest.json before execution, capture its exit status, and only continue for the
documented aggregate-parity failure; propagate every other non-zero status so
setup or runtime failures cannot use stale or partial results.
Source: Coding guidelines
1 participant
Second of the base-hygiene changes drawn from oxc's workspace (after #6754). Adopts their conformance ratchet: a generated snapshot, committed, diffed by CI in both directions.
The problem
run_gap_tests.shgated withcomm -23 <failures> known_failures.json— "fail on any failure not already triaged". That is one-directional. A listed test that starts passing produces no signal at all, so its entry survives the bug it was tracking, and the file decays into orphans nobody can revalidate. That decay is exactly what #797 asks someone to clean up by hand.The change
scripts/gap_snapshot.pygenerates and checkstest-parity/gap_snapshot.json, listing every gap test that is not passing. A test absent from the file is expected to pass. CI fails on any divergence in either direction — a new failure, a listed test that now passes, or a status change. Accept the current state with:and commit the diff. Because an improvement is a red build until it is recorded, an orphaned entry becomes impossible rather than something to audit periodically.
Design points worth reviewing:
node_fail/skippedare recorded explicitly instead of being dropped from the denominator. A test the oracle stopped covering becomes a visible diff — that hole is how a Node 22 pin hid 14 tests for months (runtime: DisposableStack / Symbol.dispose surface incomplete (.disposed returns undefined) #6364).TRUNCATED … (total: N)) that is not stable across runs for a flapping test (CI: stop truncating gap-suite output, add per-test summary #796).--shard N/8run never judges or rewrites its siblings' entries. An entry is pruned only when its test file is gone from disk, which is decidable from any shard.parity_fail -> crashis the same open bug); a fixed test drops out of the file entirely. New failures land ascategory: untriagedwith a null issue — that placeholder is the gate asking for triage.Also adds
test-parity/README.md: the category reference thatknown_failures.json's schema has always pointed at and which never existed (a #797 finding).Scope
known_failures.jsonstill gates the tag-gatedparityjob (the fulltest-files/*.tssuite); its scope is now documented in-file. Migrating that job needs a full-suite baseline from a tag run and is a follow-up.Bootstrap — why this starts red
The snapshot is evidence about a specific platform and oracle, so it cannot honestly be generated on a laptop: CI runs Ubuntu with Node pinned by
.node-version(26.5.0), and several failure classes are Linux-only or oracle-version-dependent. Generating it on macOS/Node 24 would commit wrong statuses.So the first
conformance-smokerun here is expected to fail, listing the real failures as regressions against an absent snapshot. Its 8 shards still upload their reports (if: always()), and the snapshot is seeded by folding those 8 reports in — the merge-only update semantics exist precisely so 8 partial reports compose into one file. Once seeded and committed, the run goes green and this leaves draft.Validation
gap_snapshot.py --self-testcovers both diff directions, the sharded-update case, triage carry-over, and pruning; it runs in thelintjob so the logic gating conformance-smoke is checked on the cheap job rather than only 8 shards deep. Additionally exercised end-to-end against synthetic reports: seed → green, improvement → red, regression → red, status change → red, and a one-test sharded update leaving a sibling entry intact.Per contributor convention, no version bump — maintainer bumps at merge time.
Update: snapshot seeded — and it immediately found a hole
Seeded from this PR's first (intentionally red) run: 401 tests, 384 passing, 17 not. All 8 shards now check green against it.
The 8 real failures were already in
known_failures.json, and their triage is carried over rather than discarded. The other 9 were invisible:node_failtests that no skip-list entry covered, because the old gate droppednode_failfrom the denominator. They break down as:exponential-backoff,cron,dayjs,moment,rate-limiter-flexible,slugify)node_modulestest_ramda_sum; needs a CI fixture (#1634)test_gap_4510_enum_forward_ref,test_gap_derived_param_propsERR_UNSUPPORTED_TYPESCRIPT_SYNTAX— enums and parameter properties cannot run in strip-only modetest_gap_prop_plan_cache_invalidationTypeError: Cannot assign to read only property 'q'(ESM strict mode)The two
ERR_UNSUPPORTED_TYPESCRIPT_SYNTAXcases are the sharpest argument for the ratchet: both were written to catch real Perry bugs (#4510 enum forward-references, #1758/#321 derived parameter properties), and both have been silently no-ops against the oracle ever since. The harness already supports expected-output fixtures (test-parity/expected/<name>.txt), so the fix is mechanical — worth its own issue rather than bundling here.Summary by CodeRabbit
CI Improvements
Documentation