You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit ff9379e
Browse filesBrowse the repository at this point in the historyBrowse files
ci(test): gate the goldens on PRs; keep run-ahead opt-in on measurement (#277)
* ci(test): run the goldens on PRs; measure run-ahead instead of assuming
Third and final PR of the `v1.26.0` hardening rung.
## The golden vectors ran in no pull-request job
Only `lint`, `test-light`, and `accuracysnes` run on a PR. `test-light`
never passes `--features test-roms`, and the `accuracysnes` job scoped
itself to `--test accuracysnes`. So the 53 rendered-scene goldens and
every framebuffer golden were reached only by `full-test`, which is
`mode == 'full'` -- `main`, tags, the weekly cron. A PR could shift PPU
behaviour, move a golden, and hear nothing until merge-to-main.
That is the same structure behind this project's own coprocessor-golden
staleness (goldens left stale by post-bless PPU accuracy fixes): a golden
vector nothing executes only accumulates drift. Here it was not that
nothing ran them ever, but that nothing ran them while it was still cheap
to fix.
The job now also runs the scene, undisbeliever, rainwarrior, coprocessor,
and save-state suites. Cost is bounded and known: ~5 minutes for the
three with a committed corpus (scenes ~84s, undisbeliever ~158s,
rainwarrior ~54s). The `*_oncart` suites need gitignored commercial dumps
and firmware, so they self-skip for free; they are listed anyway so the
intent is explicit rather than inferred from an omission, and so they
start gating the moment a runner ever has that corpus.
Found while looking for RustySNES's analogue of the sibling's
`expansion_level_tripwire`. That mechanism does NOT port, and saying so
matters more than inventing a stand-in: the only `commercial-roms`-gated
suite here (`commercial_screenshots`) is a screenshot generator that
asserts nothing, so it holds no golden that can go stale, and PPU
accuracy is not parameterised by a small constant set the way
expansion-audio levels are. Looking for it surfaced a larger real gap.
## Run-ahead stays opt-in -- measured, not assumed
`docs/frontend.md` recorded the per-frame save-state allocation as THE
blocker on run-ahead going default-on. `v1.25.0` removed that allocation,
so the question was re-measured rather than assumed resolved:
save_state ~119 us
load_state ~285 us
save/load round trip ~0.40 ms
one emulated frame 6.39 ms (headless_frame_steady_state)
NTSC frame budget 16.64 ms
The round trip is 2.4% of the budget. It was never the dominant cost, and
removing the allocation -- a real improvement -- did not move the
decision. What run-ahead costs is the extra frame of emulation, inherent
to the technique: `frames = 1` needs 79% of the budget, leaving ~3.5 ms
for present/UI/audio on a fast development machine; `frames = 2` needs
118% and cannot hold 60 fps at all.
So it stays opt-in. The plan for this rung said "default-on only if the
measurement supports it, and report the number either way" -- it does
not, and this is the number.
One thing did change as a result. `RunAheadConfig`'s `Default` is now
hand-written so `throttle_ms` defaults to 14 ms (ARMED) rather than the
derived `0.0` (DISABLED). The throttle is what its own doc calls the
thing that keeps run-ahead from turning a latency win into stutter, and
the derived default left it off for precisely the user who had just
enabled run-ahead from Settings and had no `throttle_ms` line in their
config. 14 ms sits below the 16.64 ms NTSC deadline with headroom and is
conservative against PAL's 20 ms. An existing config that spells the
field out keeps its value -- `#[serde(default)]` fills only absent
fields, pinned by a negative-control test alongside the positive one.
## Lockstep
Log row for RustyNES `v2.2.1`-`v2.2.4`, and the two 2026-07-12 flags
CORRECTED: the GIF/WAV capture subsystem and CRT shader-preset depth were
both closed by `v1.25.0` (`av_record.rs`; the multi-pass stack + richer
CRT + NTSC pass + `.slangp` import with the naga bridge), yet the roadmap
still asked for a maintainer go/no-go on them two weeks later.
Also carries the verification comment for #276's declined blocking
finding (that `git/matching-refs` 404s on an absent tag -- it returns
HTTP 200 with an empty array; `git/ref/tags` is the one that 404s, which
is why it was rejected). Documentation of that adjudication, not a fix.
Verified: `cargo fmt --check`, frontend clippy `-D warnings`, the new
config tests, and the golden suites run under the exact multi-`--test`
invocation the CI step uses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(config): apply review — drop the epsilon dance, derive the NTSC budget
Two review suggestions applied, both verified rather than taken on
description -- and the first one corrected my own wrong assumption.
`(x - 0.0).abs() < f32::EPSILON` -> `x == 0.0`. I expected this to trip
`clippy::float_cmp` (the workspace runs `pedantic` at `-D warnings`) and
was going to decline it on that basis; testing showed clippy exempts a
constant comparison, so the suggestion is right and the epsilon form was
just noise. The FORM matters though, and the comment now says so: the
apparently-more-idiomatic `assert_eq!(x, 0.0)` DOES fire `float_cmp` and
fails the gate, so `assert!(x == 0.0)` is the spelling that works.
The magic `1000.0 / 60.0988` now derives from `Region::Ntsc.frame_rate()`
so the two cannot drift apart, compared in `f64` (what `frame_rate()`
returns) to avoid a lossy `as f32` the pedantic cast lints would object
to.
Declined: the PR title is 71 characters, not the 73 the review reported,
so it is already inside the 72-character limit.
Verified: `cargo fmt --check`, frontend clippy `-D warnings`, and
`config::tests` 13/13.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* ci(test): --no-fail-fast so one broken golden doesn't hide the other nine
Review round two. Two applied, one declined.
**`--no-fail-fast` on the golden-vector step.** Real, and verified by
injection rather than taken on description: cargo's default stops after
the first failing test BINARY, across binaries and not merely within
one. Injecting a failure into `save_state_backward_compat` and running it
alongside `save_state_determinism` showed the second suite never ran at
all. With ten suites in this step that means one broken golden hides the
status of the other nine — an author fixes one, pushes, and only then
discovers the next, serialising a debug loop over a ~6-minute job.
Fixed with `--no-fail-fast` rather than the suggested split into ten
separate steps: one flag against ten cargo startups and ten log sections,
and re-verified that the step still FAILS the job while reporting every
suite.
**`DEFAULT_RUN_AHEAD_THROTTLE_MS`.** The `14.0` is now a named public
constant so the value, the doc comment justifying it, and the test that
bounds it cannot drift apart; the test asserts against the constant
instead of restating the literal.
**Declined: "the CHANGELOG entry is too verbose, keep it focused on
user-facing behaviour."** This project's CHANGELOG is not a summary — per
`docs/adr/0007` the annotated tag body IS the CHANGELOG section verbatim,
and `release-auto.yml` reuses it as the GitHub release notes with no
separate hand-authored text. Entries are therefore written at
release-note quality by design, and the measurements here are the whole
justification for a decision (run-ahead staying opt-in) that would
otherwise read as an unexplained non-change. Trimming to "user-facing
behaviour" would leave the release notes asserting a conclusion with its
evidence deleted.
Verified: `cargo fmt --check`, frontend clippy `-D warnings`, `cargo doc`
with warnings-as-errors, `config::tests` 13/13, ci.yml parses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(changelog): mark the timings host-specific; record why PAL isn't asserted
Review round three. One applied, two declined with measurements — and the
PAL suggestion is worth recording because I applied it first and it was
wrong.
**Applied: the absolute timings are host-specific.** The CHANGELOG now
says so, and says why they are there anyway: the ratios are what decide
run-ahead's default, and a reader can only check a ratio by re-running
`save_state_cost` + `headless_frame` and comparing. The percentages are
the portable part; the microseconds are the working.
**Declined: add a matching `Region::Pal` assertion.** I applied this,
then checked the arithmetic before pushing: NTSC's frame budget is
16.64 ms and PAL's is 20.00 ms, so PAL is the LOOSER bound by 3.36 ms
and `throttle_ms < NTSC` strictly implies `throttle_ms < PAL`. The added
assertion could never fail while the existing one passed — a vacuous
assertion sitting beside a real one and reading like extra coverage.
That is the exact shape `CLAUDE.md` names under "a guard must not subsume
the assertion it protects", and the project already has four recorded
instances of the vacuous-test trap. Reverted, with the arithmetic left in
the test so the next reader does not re-derive it.
(My first pass also wrote a comment claiming NTSC was the bound that
would "silently carry a PAL-violating value through", which is backwards
— a PAL-violating value exceeds the NTSC bound too and is caught. Fixed.)
**Declined: merge the two harness `cargo test` steps.** Measured: a second
cargo invocation against an already-built target dir costs 0.58 s — 0.16%
of the ~6-minute step. Against that, merging would apply the battery
step's `-- --nocapture` to all ten golden suites (flooding the log) and
collapse two separately-attributed CI steps into one, so a failure no
longer says at a glance whether the battery or the goldens broke.
Verified: `cargo fmt --check`, frontend clippy `-D warnings`,
`config::tests` 13/13.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(config): note the float_cmp form at the second assertion too
Review round four, nitpick applied: the `assert!(x == y)` at the default
check lacked the note its sibling assertion carries about why
`assert_eq!` fails clippy's `float_cmp` under `-D warnings`. Two
identical-looking lines where only one explains itself is how the next
person "simplifies" the unexplained one and breaks the gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
0 commit comments