From 419741f69294e8bd477b4a0c5cfc8e8078d4a02d Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 14:46:45 -0400 Subject: [PATCH 1/6] docs(changelog): backfill the empty [Unreleased] section, and reject the gate for it `[Unreleased]` was empty while three merged PRs carried user-visible change: #407's Divergence Lens (a whole new tool panel and the v2.3.8 "Parallax" marquee), #409's two-acquisition lock-race measurement, and be the single source of truth for user-visible change did not mention any of them. Backfilled now rather than at cut time. The reasons are recoverable from the commit bodies today and would have to be reconstructed from diffs later, which is how a release section ends up describing what changed instead of why. The obvious gate -- a PR touching `crates/*/src/**` must also touch `CHANGELOG.md` -- was measured against the last 50 first-parent merges before being proposed, per this release's own bar that a CI change is demonstrated against real history rather than argued. It would have gone red on 8 of them. Three are the genuine misses above. The other five are this repo's normal workflow: features land bare and the release-cut PR composes the whole section at once (the probe engine, the Latency Oracle, the RAM Atlas, the throttle instrument), plus one docs PR that touched only `//!` preambles. A 62% false-positive rate against the project's own history. A gate that fires on the normal workflow is suppressed within a week, and a suppressed gate is worse than no gate because it still reads as coverage. Rejected as specified, recorded with its numbers per the convention `docs/performance.md` sets for rejected optimizations. The measurement also relocates the defect. The failure mode is not "a feature landed without an entry" -- at the moment each of those PRs merged, nothing was wrong. It is that v2.3.8 landed its marquee and was never cut, so the cut PR that writes the section never ran and v2.3.9 opened on top of an empty [Unreleased]. No per-PR gate addresses that. What would is a release-ceremony check keyed on the cut, which is named in the plan as a decision rather than built here. --- CHANGELOG.md | 83 ++++++++++++++++++++++++++++ to-dos/plans/v2.3.9-crucible-plan.md | 57 +++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5757ff8f..46d399e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,89 @@ cycle-accurate core later replaced. ## [Unreleased] +Post-v2.3.7 work, landed but not yet cut. Backfilled: PRs #407, #409 and #410 +merged without a CHANGELOG entry, so the file that is supposed to be the single +source of truth for user-visible change did not mention a whole new tool panel. +Recorded here rather than reconstructed at release time, when the reasons are +harder to recover than the diffs. + +### Added + +- **The Divergence Lens — which pixels differ, not just which frame.** (#407, + v2.3.8 "Parallax" item A.) `Probe` could already say whether two + configurations of the same ROM diverge and at which frame, because a trial + reduces each frame to one `u64`. That reduction is the right shape for + *detecting* a difference and the wrong shape for *explaining* one: a hash says + frame 412 differs and cannot say which pixel, so it has nothing to hand to + Pixel Provenance, which is where an answer actually lives. + + `divergence::localise` re-runs both configurations to the detected frame, + keeps the full output instead of its hash, and reports the *shape* of the + difference — population count, first pixel in raster order, and the inclusive + bounding box. Count and box separate kinds of bug from each other: one pixel is + a sprite or a palette entry, 256 in a row is a scanline, tens of thousands is a + scroll or a mode change. + + It localises on the **index** framebuffer — 256x240 `u16`s of + `(emphasis << 6) | colour`, the PPU's own per-pixel output before the palette + lookup — which is half the bytes and at least as sensitive, since the RGBA + buffer is a pure function of it given the same palette. + + Three answers, and the third is the point: `Identical`, `Differs`, and + **`Inconclusive`** for an exhausted budget or two trials that cannot be + compared. The Latency Oracle's precedent applies directly: "I stopped looking" + must not arrive wearing the same shape as "they agree". The budget is checked + up front for all four trials, so spending two on detection and then finding the + localisation pair unaffordable cannot consume the budget that would have + answered the question. + +- **A Latency Oracle measurement is remembered per game.** (#410.) Reopening a + game shows what was measured last time instead of an empty panel, keyed on the + ROM SHA-256 in `[input]` — the same key shape `graphics.hd_packs` already + uses, `#[serde(default)]`, so a pre-existing config round-trips byte-identically + and a user who never opens the panel carries nothing. + + **Remembering is not applying.** Nothing here touches `run_ahead`, and + restoring never queues a pending apply, so a depth measured in an earlier + session is still one explicit click from being applied. An **inconclusive** + result is not remembered at all: a stored "I could not tell" is + indistinguishable from a stored answer once it has lost the context that + produced it. + +- **The two-acquisition lock race is measured rather than reasoned about.** + (#409.) The `needs_nes` render arm — taken exactly when a debugger or tool + panel is open — acquires the emulator lock twice per redraw, and drops it in + between so composite work does not hold the emulator. If the emulation thread + takes the lock in that gap, the screen shows frame N while a panel describes + N+1. + + `Nes::cycle()` is read at both acquisitions and compared: it is cumulative and + monotonic, and `produce_one_frame` holds the lock across a **whole** frame, so + any difference at all means at least one complete frame landed in the gap. + Both a hit count and a denominator are kept — "the race did not fire" and + "nothing was observed" both read as zero hits, and only the denominator + separates them. + +### Changed + +- **The accuracy battery runs at review time, scoped by path.** (#408.) `setup` + computed one `full` flag and `test-roms` ran only when it was true, so a + regular feature PR never ran the battery and an accuracy regression could not + be caught on the PR that caused it — #403 is the worked example. A second + `paths-filter` output covers the chip crates, core, `rustynes-gamedb`, the test + harness and `tests/`, and `test-roms` now runs when it *or* the existing full + flag is true. `rustynes-gamedb` is included for a non-obvious reason: it + rewrites the iNES header on load, so it changes what the emulator *is* before a + cycle runs. + +- **Provisioning steps are bounded, not just the jobs.** (#408, #409.) The + cross-compile gate's `apt-get update && apt-get install` were network fetches + with no timeout of their own, so a stalled mirror hung until the job timeout + fired and the run was reported as cancelled rather than as what it was — four + times during the v2.3.7 cut. Now one bounded, thrice-retried helper, with + elevation outside `timeout` so a killed fetch cannot orphan `apt-get` holding + the dpkg lock. + ## [2.3.7] - 2026-08-19 - "Overtone" (the instruction behind every mixed cycle) An *overtone* is the structure inside a sound that a single pitch reading throws diff --git a/to-dos/plans/v2.3.9-crucible-plan.md b/to-dos/plans/v2.3.9-crucible-plan.md index 0251073d..ca3a1c95 100644 --- a/to-dos/plans/v2.3.9-crucible-plan.md +++ b/to-dos/plans/v2.3.9-crucible-plan.md @@ -431,6 +431,63 @@ Scoped here rather than left implicit, and explicitly lower priority than A5/B: Both are additive and neither touches the deterministic core. +## Item D — the CHANGELOG gate, measured and REJECTED as specified + +Not planned; found while working. `[Unreleased]` was **empty** while three merged +PRs carried user-visible change, including a whole new tool panel: + +| PR | landed | CHANGELOG entry | +| --- | --- | :---: | +| #407 | the Divergence Lens (v2.3.8 "Parallax" marquee) | none | +| #409 | the two-acquisition lock-race measurement | none | +| #410 | per-game Latency Oracle persistence | none | + +So the file that is supposed to be the single source of truth for user-visible +change did not mention a new tool panel. Module 40 says to add the entry in the +same PR and to enforce it with a quality gate, and there is no such gate here. + +### The obvious gate, checked against history rather than argued + +**Proposal:** a PR touching `crates/*/src/**` must also touch `CHANGELOG.md`. + +**Measured** over the last 50 first-parent merges on `main`: it would have gone +red on **8**. Of those, **3** are the genuine misses above. The other **5** are +this repo's normal workflow: + +| merge | why the gate is wrong about it | +| --- | --- | +| `feat(v2.3.6)`: the probe engine + Latency Oracle | entry written in the v2.3.6 cut PR | +| `feat(frontend)`: the Latency Oracle + menu regroup | same | +| `feat`: the RAM Atlas | same | +| `perf(frontend)`: throttle-oscillation instrument | same | +| `docs`: specs + user guide for the v2.3.6 tools | touched only `//!` preambles | + +**A 62% false-positive rate against the project's own history.** Features land +bare and the release-cut PR composes the whole section at once. That is a +deliberate workflow, not sloppiness, and a gate that fires on it would be +suppressed within a week — which is worse than no gate, because a suppressed gate +still reads as coverage. + +### What the measurement actually shows + +The failure mode is not "a feature landed without an entry". It is **"a release +was never cut"**: v2.3.8 "Parallax" landed its marquee and was never tagged, so +the cut PR that would have written the entry never ran, and v2.3.9 opened on top +of an empty `[Unreleased]`. No per-PR gate addresses that, because at the time +each PR merged nothing was wrong. + +**Rejected as specified, and recorded with its numbers** per the convention +`docs/performance.md` sets for rejected optimizations. What was done instead is +the thing the situation actually needed: the section is **backfilled now**, while +the reasons are still recoverable from the commit bodies rather than reconstructed +from diffs at cut time. + +**What would justify revisiting it:** a gate keyed on the *cut*, not the PR — for +example, refusing to tag when `[Unreleased]` is empty while the workspace version +is behind the newest plan doc. That is a release-ceremony check (module 70), it +has no false-positive surface on ordinary PRs, and it targets the mechanism that +actually failed here. Not built in this release; named so it reads as a decision. + ## Item E — the ROM-transition sweep, and the one it found The v2.3.6 hook `DebuggerOverlay::clear_rom_bound_analysis` exists because panel From fa124b8fcd563fc55b0e6bf5b666202288f76a66 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 15:56:35 -0400 Subject: [PATCH 2/6] docs(changelog): the Divergence Lens entry described one tenth of #407 Review was right on both counts. The entry cited #407 as "v2.3.8 'Parallax' item A" and described only the pixel localisation, omitting that the feature is reachable at all. #407 is the whole of v2.3.8. Beyond item A it lands the frontend panel under Tools -> Analysis, trial-scoped provenance capture, an AUDIO lens resolving a divergence to the CPU cycle, and the pixel-cause explanation that closes item B without bisection -- so a located difference comes back as a cause rather than a coordinate. It also carries a real fix found inside the work: the Lens left the emulator thirty frames ahead of where it started, because a trial restores the anchor on the way IN and not on the way out (deliberate -- it is what lets the Lens read the trial's final frame off `nes`) and the outermost caller never put the timeline back. An entry that omits the panel describes a library, not a release. This is the reason the backfill exists at all, reproduced at smaller scale inside the backfill: the further a section is written from the work, the more of it is missing. --- CHANGELOG.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46d399e6..28fca71c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,8 +22,10 @@ harder to recover than the diffs. ### Added -- **The Divergence Lens — which pixels differ, not just which frame.** (#407, - v2.3.8 "Parallax" item A.) `Probe` could already say whether two +- **The Divergence Lens — which pixels differ, not just which frame, and why.** + (#407, the whole of v2.3.8 "Parallax".) Surfaced as a panel under **Tools → + Analysis**, over a headless `rustynes_probe::divergence` core that is tested + independently of it. `Probe` could already say whether two configurations of the same ROM diverge and at which frame, because a trial reduces each frame to one `u64`. That reduction is the right shape for *detecting* a difference and the wrong shape for *explaining* one: a hash says @@ -50,6 +52,19 @@ harder to recover than the diffs. localisation pair unaffordable cannot consume the budget that would have answered the question. + Beyond locating a difference, the Lens **explains** it. Trial-scoped + provenance capture lets a located pixel be handed to the machinery that already + answers "what wrote this, and from which instruction", so the answer is a cause + rather than a coordinate — and it closes v2.3.8 item B without bisection. An + **audio** lens resolves a divergence to the CPU cycle, the cadence at which the + mix is genuinely computed. + + One defect was found and fixed inside the same work: the Lens left the emulator + **thirty frames ahead** of where it started. A trial restores the anchor on the + way in and not on the way out, which is deliberate — it is what lets the Lens + read the trial's final frame off `nes` directly — but the outermost caller has + to put the timeline back, and did not. + - **A Latency Oracle measurement is remembered per game.** (#410.) Reopening a game shows what was measured last time instead of an empty panel, keyed on the ROM SHA-256 in `[input]` — the same key shape `graphics.hd_packs` already From 68fa45a6a611d716a2f0aafead0e868a6956bd8f Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 15:59:04 -0400 Subject: [PATCH 3/6] docs(changelog): drop the note about the changelog's own maintenance Review is right that this belongs elsewhere. The paragraph explained that #407, #409 and #410 merged without an entry -- a fact about how this document is maintained, not about what changed for a user. The CHANGELOG's own header already says it is the record of user-visible change, so a section describing its gaps is the one thing in it that is not. It is not lost: the full account, including the measured rejection of the per-PR gate and its 62% false-positive rate, is item D of the v2.3.9 plan, and the reasoning is in this branch's commit bodies. The companion nitpick -- that the entries read like architecture decision records rather than concise release notes -- is declined. That is this project's CHANGELOG voice, not an accident of this PR: every released section explains the mechanism and what was believed before the measurement. Matching a generic house style would make these entries inconsistent with the file they are joining. --- CHANGELOG.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28fca71c..9c5925e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,11 +14,7 @@ cycle-accurate core later replaced. ## [Unreleased] -Post-v2.3.7 work, landed but not yet cut. Backfilled: PRs #407, #409 and #410 -merged without a CHANGELOG entry, so the file that is supposed to be the single -source of truth for user-visible change did not mention a whole new tool panel. -Recorded here rather than reconstructed at release time, when the reasons are -harder to recover than the diffs. +Post-v2.3.7 work, landed but not yet cut. ### Added From 6dd1afb227742f2ceb85db4e3d4db044993c7e23 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 18:21:39 -0400 Subject: [PATCH 4/6] fix(frontend): omit the latency map when empty, so the claim about it is true Review found that the entry claimed something `#[serde(default)]` does not provide, and the claim was wrong rather than imprecise. `serde(default)` covers LOADING a config that lacks the key. It says nothing about SAVING, and the TOML serializer emits an empty table for an empty map -- so a user who never opened the Latency Oracle would have had their config rewritten with a bare `[input.latency_reports]` on the first save after upgrading. Checked rather than reasoned about: serializing a default `Config` and grepping the output puts the table at line 100. Fixed by making the claim true rather than by weakening it. A `skip_serializing_if` keeps the key out of the file until there is something to store, and the CHANGELOG now separates the two guarantees instead of attributing both to `default`. The same false claim turns out to sit on two SHIPPED fields -- `graphics.hd_packs` (v1.5.0) and `graphics.shader_presets` (v1.2.0) -- both saying a pre-feature config "is byte-identical" when it is only byte-identical until the first save. Those are corrected in PROSE only, deliberately: adding `skip_serializing_if` there would change the file two shipped features write, which is a separate decision with its own risk, and the wrong half was the claim rather than the behaviour. The distinction now stated at each site is that `serde(default)` is a LOAD guarantee. Two mutations, both directions. Removing `skip_serializing_if` fails the test -- it is the original defect -- and so does an over-eager `skip_serializing_if` that always returns true, which would silently discard real measurements. A one-directional test here would have passed against a field that never persists anything at all. --- CHANGELOG.md | 8 ++-- crates/rustynes-frontend/src/config.rs | 66 +++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c5925e8..1648478c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,9 +63,11 @@ Post-v2.3.7 work, landed but not yet cut. - **A Latency Oracle measurement is remembered per game.** (#410.) Reopening a game shows what was measured last time instead of an empty panel, keyed on the - ROM SHA-256 in `[input]` — the same key shape `graphics.hd_packs` already - uses, `#[serde(default)]`, so a pre-existing config round-trips byte-identically - and a user who never opens the panel carries nothing. + ROM SHA-256 in `[input]` — the same key shape `graphics.hd_packs` already uses. + `#[serde(default)]` so an older config loads unchanged, plus + `skip_serializing_if` so the key stays out of the file until there is something + to store: a user who never opens the panel carries nothing and their config is + not rewritten. **Remembering is not applying.** Nothing here touches `run_ahead`, and restoring never queues a pending apply, so a depth measured in an earlier diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index fa99032a..d8b14f2b 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -150,15 +150,25 @@ pub struct InputConfig { /// run-ahead depth, which is the field directly above; a measurement and the /// setting it recommends belong together. /// - /// `#[serde(default)]` = empty, so a pre-v2.3.9 config round-trips - /// byte-identically and a user who never opens the panel carries nothing. + /// `#[serde(default)]` = empty, so a pre-v2.3.9 config LOADS unchanged, and + /// `skip_serializing_if` keeps the key out of the file entirely until there + /// is something to store — so a user who never opens the panel carries + /// nothing and their config round-trips byte-identically. + /// + /// The two halves are separate guarantees and only one of them is + /// `default`'s. `#[serde(default)]` alone covers LOADING a file that lacks + /// the key; on SAVE the TOML serializer still emits an empty + /// `[input.latency_reports]` table, so the first save after upgrading would + /// rewrite the file. Review on #414 caught the original claim here, and it + /// was settled by serializing a default `Config` and grepping the output + /// rather than by re-reading the derive. /// /// **Remembering a measurement is not applying it.** Nothing here changes /// `run_ahead`; the panel still requires an explicit Apply, which is the /// separation v2.3.6 built deliberately ("measured" and "applied" are two /// auditable steps). Persisting the recommendation would quietly convert one /// into the other across a restart. - #[serde(default)] + #[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")] pub latency_reports: std::collections::BTreeMap, /// v1.1.0 beta.1 (T-110-B2) — turbo/autofire on the A button: while held, A /// rapid-fires. Off by default (`false`) = byte-identical input. Applied @@ -851,16 +861,24 @@ pub struct GraphicsConfig { #[serde(default)] pub shader_stack: crate::shader_pass::ShaderStackConfig, /// v1.2.0 C2 — saved named shader-stack presets (the CRT preset bank + - /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config is - /// byte-identical. Persisted under `[graphics.shader_presets]`. + /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config LOADS + /// unchanged. Persisted under `[graphics.shader_presets]`. Same correction as + /// `hd_packs` above: `serde(default)` says nothing about what SAVE writes. #[serde(default)] pub shader_presets: crate::shader_pass::ShaderPresetBank, /// v1.2.0 beta.2 (Workstream C3) — per-game HD-pack paths, keyed on the /// ROM SHA-256 (hex). When the loaded ROM's hash has an entry here AND the /// `hd-pack` feature is built in, the frontend loads the referenced pack /// (folder or `.zip`) and substitutes hi-res tiles at blit time. Empty by - /// default and `#[serde(default)]`, so a pre-C3 config is byte-identical - /// and the default presentation is unchanged. Presentation-only. + /// default and `#[serde(default)]`, so a pre-C3 config LOADS unchanged and + /// the default presentation is unchanged. Presentation-only. + /// + /// Deliberately says "loads", not "is byte-identical": `serde(default)` is a + /// LOAD guarantee only, and on save the TOML serializer emits an empty + /// `[graphics.hd_packs]` table. Left as-is rather than given a + /// `skip_serializing_if` like `input.latency_reports`, because that would + /// change the file this shipped feature writes; the wrong half here was the + /// claim, not the behaviour. #[serde(default)] pub hd_packs: std::collections::BTreeMap, /// v1.5.0 "Lens" Workstream D1 — per-side overscan crop, in NES pixels. The @@ -2130,6 +2148,40 @@ fn canonicalize_pad(pad: &PadBindings) -> PadBindings { #[cfg(test)] mod tests { + + /// `#[serde(default)]` covers LOADING a config that lacks the key. It says + /// nothing about SAVING, and the TOML serializer emits an empty table for an + /// empty map — so a user who never opened the Latency Oracle would have had + /// their config rewritten with a bare `[input.latency_reports]` on the first + /// save after upgrading. Review on #414 caught the claim; this pins the fix. + /// + /// Both directions are asserted, because a `skip_serializing_if` that is too + /// eager would silently discard real measurements. + #[test] + fn an_empty_latency_map_is_not_written_but_a_populated_one_is() { + let empty = toml::to_string_pretty(&Config::default()).expect("serialize default"); + assert!( + !empty.contains("latency_reports"), + "an empty map was written to the config, so an untouched file does \ + not round-trip:\n{empty}" + ); + + let mut c = Config::default(); + c.input.latency_reports.insert( + "deadbeef".to_owned(), + RememberedLatency { + frames: 3, + unanimous: true, + frame_micros: 16_639, + }, + ); + let filled = toml::to_string_pretty(&c).expect("serialize populated"); + assert!( + filled.contains("latency_reports"), + "a real measurement was dropped on save:\n{filled}" + ); + assert!(filled.contains("deadbeef"), "the ROM key was not written"); + } use super::*; use tempfile::TempDir; From 5c61fcf14f5b96329f03ea430ca2f7b10ef8be20 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 19:00:37 -0400 Subject: [PATCH 5/6] docs(config): correct a direction word, and move the test below the imports Two review points, both small and both real. The `shader_presets` note said "same correction as `hd_packs` above". `hd_packs` is defined eighteen lines BELOW it. A cross-reference that sends the reader the wrong way is worse than none, and it is the kind of error that survives because nobody checks a direction word. The test had been inserted above the module's `use` statements. The insertion point right after `mod tests {` is before the imports, not after -- the same slip caught on #420, from the same habit of anchoring on the `mod tests {` line. --- crates/rustynes-frontend/src/config.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index d8b14f2b..181aa49f 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -863,7 +863,7 @@ pub struct GraphicsConfig { /// v1.2.0 C2 — saved named shader-stack presets (the CRT preset bank + /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config LOADS /// unchanged. Persisted under `[graphics.shader_presets]`. Same correction as - /// `hd_packs` above: `serde(default)` says nothing about what SAVE writes. + /// `hd_packs` below: `serde(default)` says nothing about what SAVE writes. #[serde(default)] pub shader_presets: crate::shader_pass::ShaderPresetBank, /// v1.2.0 beta.2 (Workstream C3) — per-game HD-pack paths, keyed on the @@ -2149,6 +2149,9 @@ fn canonicalize_pad(pad: &PadBindings) -> PadBindings { #[cfg(test)] mod tests { + use super::*; + use tempfile::TempDir; + /// `#[serde(default)]` covers LOADING a config that lacks the key. It says /// nothing about SAVING, and the TOML serializer emits an empty table for an /// empty map — so a user who never opened the Latency Oracle would have had @@ -2182,8 +2185,6 @@ mod tests { ); assert!(filled.contains("deadbeef"), "the ROM key was not written"); } - use super::*; - use tempfile::TempDir; #[test] fn parse_pal_reads_64_colours_and_rejects_short() { From eee14707890ebd525c73c05dd9b21e3d78bbfc61 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 19:40:46 -0400 Subject: [PATCH 6/6] test(config): round-trip the latency map, not just its key Review is right that the string checks were half a test. `contains( "latency_reports")` verifies the KEY and says nothing about the VALUE, so a `skip_serializing_if` on a field whose `Deserialize` had drifted would still produce exactly the right text and load back as something else -- and it is the load side the documentation promises. Both directions now round-trip: the empty config re-parses to an empty map (so the omitted key is genuinely equivalent to an absent one, which is the whole claim), and the populated one re-parses to the same `RememberedLatency` it was given. --- crates/rustynes-frontend/src/config.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 181aa49f..c8ce40f8 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2184,6 +2184,27 @@ mod tests { "a real measurement was dropped on save:\n{filled}" ); assert!(filled.contains("deadbeef"), "the ROM key was not written"); + + // Round-trip both, because the string checks alone verify the KEY and + // say nothing about the VALUE. `skip_serializing_if` on a field whose + // `Deserialize` had drifted would still produce the right text and load + // back as something else, and it is the load side the documentation + // promises. (Review on #414.) + let empty_back: Config = toml::from_str(&empty).expect("empty config re-parses"); + assert!( + empty_back.input.latency_reports.is_empty(), + "the omitted key did not come back as an empty map" + ); + let filled_back: Config = toml::from_str(&filled).expect("populated config re-parses"); + assert_eq!( + filled_back.input.latency_reports.get("deadbeef").copied(), + Some(RememberedLatency { + frames: 3, + unanimous: true, + frame_micros: 16_639, + }), + "the measurement did not survive a save/load round trip" + ); } #[test]