test(roborazzi): golden the editor with the formatting row gone (#331) - #336
Conversation
The setting that removes the standing `Aa` row shipped with one instrumented assertion — the node does not exist — and a hand check on an emulator. Neither can see the thing the setting exists to fix. The complaint in #328 was not the button, it was the 48dp strip of empty screen it holds above the keyboard, so what "off" has to produce is an absence with nothing left behind. A layout change that left the row's height while dropping its content would keep the node absent and pass, and `EditorFormattingControlsSnapshotTest` cannot cover it either: in this state the row is not mounted, so there is nothing there to photograph. That makes it a screen-level golden, and the screen builds its own `AppSettingsRepository`, which a Robolectric test cannot put into a chosen state — the `preferencesDataStore` delegate caches one instance per JVM and DataStore's `File.renameTo` writes fail on Windows after the first, which is why the repository already has an internal DataStore constructor (#158). `EditorScreen` now takes the repository as a defaulted parameter, so the same seam that makes the repository testable makes the screen it feeds testable too. Nothing else changes: the default is the process-wide store, and the existing golden still composes the screen exactly as the app builds it. `InMemoryPreferencesDataStore` moves out of `AppSettingsRepositoryLockTest`, where it was a private copy also duplicated in `AppSettingsRepositoryListTest`, into one class the snapshot test can use as well. The new test asserts the premise before capturing: an image cannot say why it looks the way it does, and this one is defined by an absence, so a capture that happened before the setting reached the screen would look plausible and pin the wrong state. Both tests check the formatting node — present by default, absent with the setting off — and only then photograph the surface. Golden recorded on the Linux runner via the `record_roborazzi` dispatch, since Windows font hinting produces images CI then rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Recorded on the Linux runner via the record_roborazzi dispatch on this branch (run 32744806146). One new image; the other 69 goldens came back byte-identical from the same run, so taking the settings repository as a parameter moved no rendering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1231740b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // state. Composing this screen with a setting turned off is what the golden | ||
| // for "Show formatting button" needs (#331) — the row is *not mounted* when | ||
| // it is off, so nothing below screen level can show that it left no gap. | ||
| settingsRepository: AppSettingsRepository = rememberAppSettingsRepository() |
There was a problem hiding this comment.
This introduces a production EditorScreen testability seam, shared test infrastructure, and a new golden, but the commit adds no HISTORY.md entry describing the work and its verification. The inherited v2.33.0 history still lists this golden as missing, so add a follow-up entry recording its completion as required by the repository’s code-change documentation rule.
AGENTS.md reference: AGENTS.md:L245-L249
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bc1f53 — the rule is the repository's own (AGENTS.md § Documentation And History: 작업 단위 이력은 HISTORY.md에 기록), and the v2.33.0 entry above did still list this golden as missing.
One entry covers the whole pass rather than one per PR, since #335 (the locale list and its bidirectional gate) and this PR are the same piece of work against the same tracker section, and #335 had merged without one. It records what moved, the two holes you caught on #335, the drift directions each gate was broken in, and what is deliberately left open in #262.
No CHANGELOG.md entry: nothing in either PR is user-visible — the seam is a defaulted parameter over the same store, and the 69 byte-identical goldens from the record run are the evidence for that.
The repository rule is that a work unit lands in HISTORY.md, and the v2.33.0 entry above still lists these five candidates as open — Codex caught the omission on #336. One entry covers the pass rather than one per PR, since #335 (the locale list and its gate) and #336 (the golden and the seam it needed) are the same piece of work against the same tracker section. No CHANGELOG entry: nothing in either PR is user-visible, and the 69 byte-identical goldens are the evidence for the one production change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
The last open item in the v2.33.0 section of #262: no golden for the editor with the formatting row gone (#331).
Why the checks it shipped with cannot see this
"Show formatting button" off was pinned by one instrumented assertion — the node does not exist — and a hand check on an emulator. Neither looks at the thing the setting exists to fix. The complaint in #328 was not the button; it was the 48dp strip of empty screen it holds above the keyboard. So "off" has to produce an absence with nothing left behind, and a layout change that kept the row's height while dropping its content would keep the node absent and pass.
EditorFormattingControlsSnapshotTestcannot cover it either: in this state the row is not mounted, so there is nothing there to photograph.That makes it a screen-level golden.
The seam
EditorScreenbuilt its ownAppSettingsRepository, which a Robolectric test cannot put into a chosen state — thepreferencesDataStoredelegate caches one instance per JVM and DataStore'sFile.renameTowrites fail on Windows after the first. That is the same wall #158 hit, which is whyAppSettingsRepositoryalready has an internal DataStore constructor.EditorScreennow takes the repository as a defaulted parameter, so the seam that makes the repository testable makes the screen it feeds testable too. The default is the process-wide store, and the existing golden still composes the screen exactly as the app builds it — it is passed only when the setting is not the default.InMemoryPreferencesDataStoremoves out ofAppSettingsRepositoryLockTest, where it was a private copy also duplicated inAppSettingsRepositoryListTest, into one class the snapshot test can use as well.The premise is asserted, not assumed
An image cannot say why it looks the way it does, and this one is defined by an absence: a capture that happened before the setting reached the screen would look plausible and pin the wrong state. Both tests now check the formatting node — present by default, absent with the setting off — and only then photograph the surface.
Verification
record_roborazzidispatch on this branch (run 32744806146).Aarow, and the empty-state block sits lower because the body now reaches the bottom edge — that difference againsteditor_screen_quiet_appbar_phoneis the reclaimed strip.:app:testDebugUnitTest --tests "*EditorScreenSnapshotTest*"green locally (2/2), which also proves the injected setting reaches the composition: with it off,assertDoesNotExiston the formatting node is what passes.Tracker: #262 (v2.33.0 section, the fifth and last item — #335 covers the other four).
🤖 Generated with Claude Code