Snapshot testing: review-comment cleanup (structure, product-code de-leak, bridging)#113
Open
kyleve wants to merge 8 commits into
Open
Snapshot testing: review-comment cleanup (structure, product-code de-leak, bridging)#113kyleve wants to merge 8 commits into
kyleve wants to merge 8 commits into
Conversation
…ndle The capture/compare pipeline had no test bundle of its own; its generic regression tests (async settle, concurrent-capture serialization, duplicate identifiers, tile-and-stitch/full-content, the pre-capture hook, and the same-image capture-flag probe) lived in WhereUISnapshotTests despite being WhereUI-free and asserting on probed pixels rather than LFS references. Move them into a new Shared/SnapshotKitTesting/Tests + SnapshotKitTestingTests bundle, wired into Project.swift and the Stuff-iOS-Tests scheme (fast, no __Snapshots__, so it runs in the main test job, not the snapshot job). Promote PixelSample/probePixel into SnapshotKitTesting as @_spi(Testing) API shared by both bundles, and add a SafeAreaComposition regression test pinning that the swizzle zeroes the captured root while a safeAreaInset bar still composes on top. The WhereUI-defined cross-boundary probe test + view stay in WhereUI (they exist to detect a WhereUI<->bundle SnapshotKit split). Closes review comments on test placement (Plan to-do: bundle).
… cutsheet
Each view's SnapshotProviding conformance lived in Sources/Preview/{Screen,
Widget,AppFlow}Snapshots.swift, divorced from the view, and no #Preview
rendered the snapshotPreviews cutsheet — so the preview/snapshot single-source
the architecture is built around wasn't actually realized.
Move every conformance into its own view file under #if DEBUG and point the
view's #Preview at Self.snapshotPreviews, so one declaration drives both the
Xcode cutsheet and the image tests. RegionMapView keeps a bespoke live-Map
preview alongside the cutsheet (the cutsheet renders the deterministic
stand-in); RootView keeps its from-scratch launch preview. Delete the three
Preview/*Snapshots.swift files (WhereSnapshot.swift's helpers stay).
Also centralize the pinned widget instant on PreviewSupport (reusing the same
value the references were recorded with) and add PreviewSupport.onboardingModel()
over in-memory preferences, fixing the onboarding fixture's real-UserDefaults
leak as its declaration moves.
Renders identically — the fixtures/config are byte-for-byte the prior
declarations, verified by a full WhereUISnapshotTests run with zero reference
changes (the lone exception, resolution.Empty_iPhone{,_dark}, is a pre-existing
sub-threshold local rendering drift on the all-clear ContentUnavailableView, not
introduced here — the reference is unchanged and the fixture is identical).
Plan to-do: declarations-in-views.
The ScreenSnapshotTests / WidgetSnapshotTests / AppFlowSnapshotTests omnibus suites lumped every view's references into three shared __Snapshots__/ directories. Split them into one FooSnapshotTests per view, each in its own file, so each view's reference images live in their own on-disk directory. Because swift-snapshot-testing keys the directory on the test file's basename and each per-view suite reuses the original @test function name, relocating the ~210 LFS references is a pure git mv (no renames, no re-recording) — verified by a full WhereUISnapshotTests run: every suite green from its new directory with zero reference-byte changes. (The lone red, resolution.Empty_iPhone{,_dark}, is the pre-existing local rendering drift noted in the prior commit, not the move.) The two third-party inline surfaces (DebugLogViewer, SwiftDataInspector) get their own suites too, carrying their bespoke fixtures. Plan to-do: per-view-tests.
SettingsView and ManualDayView each read \.isCapturingSnapshot and branched between a live DatePicker and SnapshotDatePickerStandIn inline — leaking snapshot-environment checks into product UI. Introduce WhereDatePicker, a compact date/time picker that makes that choice internally (mapping onto DatePicker's in: range overloads for the live path), with SnapshotDatePickerStandIn demoted to a fileprivate detail of its file. Both product views now use WhereDatePicker and no longer import the capture flag. Renders identically — ManualDayView and SettingsView snapshot suites pass with no reference changes. Closes review comments on product code branching on the snapshot flag / applying the wrapper-type pattern. Plan to-do: wheredatepicker.
… flags RegionMapView read \.isCapturingSnapshot and branched between the live MapKit Map and the stand-in inside its own body helper. Move that choice into a private RegionOutlinesMap subview so the screen no longer reads the capture flag at all — it just renders the substrate, unaware which one drew. The stand-in's rendering is unchanged (regionMap references untouched); the live-Map path gains a dedicated hosting test under the fold-in step. LaunchSplashView: document why it holds both motionIsStatic (Reduce Motion or capture — freezes the never-settling pulse) and isCapturingSnapshot (gates the wall-clock caption timer, which Reduce Motion users must still get), answering the review question, and extract RadarPingBackground's phase-clock computation into a fileprivate TimelineViewDefaultContext.radarPhaseClock(capturingSnapshot:) extension. RegionMap + LaunchSplash snapshot suites pass unchanged. Plan to-do: map-splash.
…drop dead enum The capture flag was a pure UITraitBridgedEnvironmentKey, so a SwiftUI-set value (the preview cutsheet setting .environment for isCapturingSnapshot) paid a UITraitCollection round-trip / first-frame lag. Adopt Broadway BContext+SwiftUI pattern: a pure-SwiftUI EnvironmentKey read first, falling back to the trait-bridged value, with the setter mirroring into both. SwiftUI-set values now propagate synchronously; the test pipeline traitOverrides path still reaches SwiftUI via the fallback, and the WhereUI cross-boundary probe still exercises the bridged key. Verified: SnapshotKitTestingTests (pipeline path), SnapshotCaptureFlagProbeTests (cross-boundary), and motion-frozen captures (launchSplash, recentActivity) all pass. Also document that MotionIsStatic nested @Environment reads resolve (it is a DynamicProperty), and delete the unused empty enum SnapshotKit namespace file (SnapshotKitTesting.swift stays; its @_exported imports are load-bearing). Plan to-do: bridging.
…ions Answer the review questions on SnapshotKitTesting with in-code rationale (no behavior change): - hostKeyWindow(): note it is the window StuffTestHost stamps with isMainTestHostWindow (the guaranteed root we set up), not merely the current key window, so it is stable across transient key-window changes. - Settle stability compares quarter-res samples byte-exact, deliberately unlike the final image compare tolerance: this loop asks has rendering stopped changing (the low-res sample already absorbs sub-pixel jitter), and a tolerance would let a slow drift read as settled. - hideTextInputCursors walks the tree targeting only text inputs rather than clearing the root tint once: tintColor inherits, so a clear root tint would also blank every accent-tinted control (buttons, links, toggles) and change the image. (The SafeAreaInsetsSwizzling verification landed as SafeAreaCompositionTests in the SnapshotKitTesting bundle.) Plan to-do: pipeline-questions.
… smell - SnapshotKitTesting/AGENTS: replace the now-false 'no dedicated test bundle' note with SnapshotKitTestingTests (pixel-probe pipeline regressions, in the main Stuff-iOS-Tests scheme). - SnapshotKit/AGENTS: describe isCapturingSnapshot as the Broadway-style hybrid (synchronous SwiftUI key + trait-bridged fallback). - WhereUI/AGENTS + README: matrices now declared in each view's own file (cutsheet via Self.snapshotPreviews), one FooSnapshotTests per view with per-view __Snapshots__ dirs; drop the stale 'bold text' matrix mention. - root AGENTS: note SnapshotKitTestingTests runs in the normal test job (no LFS), unlike the image-reference bundles. - Where/TODOs: file the CalendarView capture-time scroll-skip smell; mark the onboarding-leak and preview/cutsheet-claim findings resolved; note the pinned-instant de-duplication and why the matrix-gap cases stay deferred (local reference recording risks CI drift). Regenerated CLAUDE.md via ./sync-agents (gitignored). Plan to-do: todo-and-docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the review comments on #101, stacked onto that branch (base
snapshot-testing). The theme was structural — where tests live, where snapshot matrices are declared, and product code branching on the capture flag — plus a few pipeline-correctness questions.What changed
Generic pipeline tests moved to their own module (was: in WhereUI).
SnapshotKitTestinghad no test bundle; its generic regression tests lived inWhereUISnapshotTests. NewSnapshotKitTestingTestsbundle (wired inProject.swift, in theStuff-iOS-Testsscheme) now owns them: async-content settle, concurrent-capture serialization, duplicate identifiers, tile-and-stitch / full-content, the pre-capture hook, the same-image capture-flag probe, and a new safe-area composition regression test (the swizzle zeroes the captured root while an interiorsafeAreaInsetstill composes — verifyingSafeAreaInsetsSwizzling).PixelSample/probePixelpromoted to@_spi(Testing)API shared by both bundles. They assert on probed pixels (no LFS refs), so the bundle runs in the normaltestjob. The WhereUI-defined cross-boundary flag probe stays in WhereUI (only it can detect a duplicate-SnapshotKitsplit).Snapshot matrices moved into each view's own file, driving the preview cutsheet.
Each
SnapshotProvidingconformance moved out ofSources/Preview/{Screen,Widget,AppFlow}Snapshots.swiftinto its view file under#if DEBUG, and each view's#Previewnow rendersSelf.snapshotPreviews— so one declaration drives both the Xcode cutsheet and the image tests (the architecture's whole point). The three*Snapshots.swiftfiles are deleted;WhereSnapshot.swift's helpers stay.One
FooSnapshotTestsper view, each with its own on-disk directory.The omnibus
Screen/Widget/AppFlowsuites split into per-view suites in their own files. Because each reuses the original@Testfunction name, relocating the ~210 LFS references was a puregit mv— no renames, no re-recording (a separate, byte-clean commit).Product code no longer branches on
\.isCapturingSnapshot.WhereDatePickerwrapper makes the live-vs-stand-in choice internally (SnapshotDatePickerStandIndemoted to a fileprivate detail);SettingsView/ManualDayViewuse it and drop the capture flag.RegionMapView's capture branch moved into a privateRegionOutlinesMapsubview, so the screen no longer reads the flag.LaunchSplashView: documented why it needs bothmotionIsStatic(freezes the pulse) andisCapturingSnapshot(gates the wall-clock caption timer, which Reduce-Motion users must still get), and extracted the radar phase-clock into afileprivate TimelineViewDefaultContextextension.Synchronous flag bridging (the Broadway pattern).
SnapshotCaptureFlagis now a hybrid accessor likeBContext+SwiftUI: a pure-SwiftUIEnvironmentKeyread first (synchronous — the cutsheet's.environment(\.isCapturingSnapshot, true)no longer pays aUITraitCollectionround-trip), falling back to the trait-bridged key (how the pipeline'straitOverridesreaches SwiftUI, and how the cross-boundary probe works). Also: confirmed/commented thatMotionIsStatic's nested@Environmentreads resolve (it's aDynamicProperty), and deleted the unused emptyenum SnapshotKit {}.Pipeline-correctness questions answered in-code (no behavior change):
hostKeyWindow()is the stampedStuffTestHostwindow (not the current key window); the settle loop compares quarter-res samples byte-exact on purpose (it asks "stopped changing?", not "matches reference?");hideTextInputCursorstargets only text inputs becausetintColorinherits (clearing the root would blank every accent-tinted control).Folded-in review findings (already-filed TODOs): centralized the pinned widget instant on
PreviewSupport; fixed the Onboarding fixture's real-UserDefaultsleak (PreviewSupport.onboardingModel()over in-memory prefs); filed theCalendarViewcapture-time scroll-skip smell as a TODO; docs (AGENTS/READMEfor SnapshotKit, SnapshotKitTesting, WhereUI, root) updated to match.Verification
SnapshotKitTestingTests— green.Stuff-iOS-Tests(all unit bundles, incl.WhereUITests,WhereIntentsTests, Broadway, and the new bundle) — green.WhereUISnapshotTests— green with zero reference-byte changes from the restructure, exceptresolution.Empty_iPhone{,_dark}: a pre-existing ~0.12% local rendering drift on the all-clearContentUnavailableView(same dimensions, reference unchanged in git, identical fixture — it fails the same way on the base branch on this machine). Left untouched rather than re-recorded, since re-recording locally would diverge from CI's renderer. Worth a confirm on CI / the reference environment.Deliberately deferred (kept in
Where/TODOs.md)PrimaryViewempty,RecentActivitySummaryView.loading,ManualDayViewrange-add) and a live-Maphosting test — all need new reference images (or reintroduce a hosting smoke test the snapshot PR removed). Given theresolution.Emptydrift above, recording references locally risks CI divergence, so these are held for the reference environment.I did not post individual replies on the #101 review threads (that's 28 comments on your PR) — happy to, just say the word; this description is the full comment-to-change mapping in the meantime.