Skip to content

File the broken-snapshots follow-ups from PR #101 as TODOs#142

Merged
kyleve merged 1 commit into
mainfrom
file-broken-snapshot-todos
Jul 27, 2026
Merged

File the broken-snapshots follow-ups from PR #101 as TODOs#142
kyleve merged 1 commit into
mainfrom
file-broken-snapshot-todos

Conversation

@kyleve

@kyleve kyleve commented Jul 27, 2026

Copy link
Copy Markdown
Owner

PR #101 (the WhereUI snapshot suite) was merged with twelve [Fix later] review comments enumerating renderings the suite pinned that are actually broken. Those comments were the only record of the debt. This moves them into the TODOs.md files, under a greppable broken-snapshots marker, and files the related placeholder-test gap alongside them.

Docs only — no code or reference images change.

What's filed

Where/TODOs.md — a broken-snapshots parent under P1s

Eight children, one per finding. Each was re-verified against the reference image and the current source rather than transcribed, so every item carries file:line evidence and a suggested fix:

Finding Evidence
VoiceOver-annotated calendar captures are blank calendarContent.WithData_{iPhone,iPad}_accessibility.png are 66/171 KB of white against 205 KB–2.2 MB for every other screen-sized _accessibility reference — specific to CalendarContentView, and it slips past every ImageRenderingError guard in AccessibilitySnapshotViewController
Calendar day grid breaks at ax5 Two-digit dates truncate to one digit (the 10th–31st render as "1"/"2"/"3") off the fixed numberSize square; weekday headers wrap mid-word
Presence timeline accent bar doesn't scale Fixed 4×34 stylesheet tokens, so a 4pt stub beside ~40pt text at ax5
Presence timeline row squishes instead of restacking One HStack at every type size; "California" hyphenates over three lines
YearView overflows horizontally at ax5 Month title reads "nuary", grid clipped both edges; YearModePicker's fixed-width bottom inset is the suspect
Resolve toolbar badge sits half outside the glass capsule Hand-rolled Capsule overlay with a fixed .offset
Resolve sheet's ax5 reference is cut off Captures at the fixed .iPhone frame; Frame.fullContent exists for this
Elsewhere subtitle bakes in ^[3 region](inflect: true) Cross-reference to the P1 already filed for it, so the image isn't mistaken for correct output

The parent notes these are not flake — the file's existing "Deferred snapshot-test flakiness" ledger is a separate thing — so nobody re-records them.

Root TODOs.md — the two relocations

PeriscopeViewerSnapshotTests and SwiftDataInspectorSnapshotTests snapshot views WhereUI doesn't own. Each spans WhereUI and a shared module, so per the placement rule they sit in the root file rather than in either module's. Both are blocked on the same prerequisite: neither shared module has a snapshot bundle, and adding one means a target plus a standalone scheme and a snapshot CI job.

Shared/Periscope/TODOs.md — the placeholder tests

Eighteen tests across nine PeriscopeTools test files assert only #expect(await waitUntil { host.view.window != nil }) (or the try waitFor spelling). The predicate restates what show/showHosted already guarantee, so each test proves construction didn't crash and nothing about what rendered — including the parts the seeding goes to trouble to set up (the hierarchy outline, the injected comfortable density, the documented "No Events" state). Filed as a conversion to a PeriscopeToolsSnapshotTests bundle over the same seeded stores, sharing the bundle prerequisite with the PeriscopeViewer relocation above.

Shared/LifecycleKit/Tests/ has two window != nil checks that are explicitly out of scope and called out as such — they assert the hosting helper's own lifecycle contract, which is the one place the check is the point.

Shared/SwiftDataInspector/TODOs.md — a correction

Its existing test-split item suggests "optionally adding hosted UI smoke tests", which now contradicts the convention. A nested child points at the snapshot bundle instead, so the split isn't done against smoke tests that would immediately be replaced.

Verification

./swiftformat --lint clean. No Swift changed, so no test run applies.

PR #101 landed the WhereUI snapshot suite with twelve "[Fix later]" review
comments enumerating renderings it pinned that are actually broken. Those
comments were the only record; this files them as TODO items so they survive
the PR.

Where/TODOs.md gets a "broken-snapshots" parent under P1s with one child per
finding, each verified against the reference image and the source: the blank
VoiceOver-annotated calendar captures, the calendar day grid truncating
two-digit dates at ax5, the presence timeline's unscaled accent bar and
non-restacking row, YearView's horizontal overflow, the Resolve toolbar
badge, the clipped Resolve sheet reference, and the Elsewhere subtitle's raw
inflection markup (a cross-reference to the P1 already filed for it).

The root TODOs.md takes the two relocations — the PeriscopeViewer and
SwiftDataInspector suites snapshot views WhereUI doesn't own — since each
spans WhereUI and a shared module, per the placement rule.

Also files the related test-quality gap the user flagged: eighteen
PeriscopeTools tests across nine files assert only
`#expect(host.view.window != nil)`, which restates what show/showHosted
already guarantee and says nothing about what rendered. Shared/Periscope's
TODOs.md tracks converting them to a snapshot bundle, and
SwiftDataInspector's picks up a note not to add the hosting smoke tests its
existing split item suggests. Both share the missing-bundle prerequisite in
the root item. LifecycleKit's two window checks are deliberately out of
scope — they test the hosting helper's own contract.

Docs only; no behavior change.
@kyleve
kyleve merged commit dafa08c into main Jul 27, 2026
4 checks passed
kyleve added a commit that referenced this pull request Jul 27, 2026
Brings in #142, which ledgers the broken-snapshot follow-ups from #101.
Docs-only — four TODOs.md files, no code — so the branch's existing
verification still stands.

One conflict, in the root TODOs.md P2 bucket, and purely additive: main
added the broken-snapshots item and its two nested sub-items while this
branch had added the PeriscopeCore LogSession one. Kept both.
cursor Bot pushed a commit that referenced this pull request Jul 27, 2026
main's #142 added the broken-snapshots P2 while this branch split the
StuffTestHost item in two: the WhereCore half moved to Completed and the
scene-configuration-name half was re-filed as its own quick-win. Kept
both sides — main's new item plus this branch's split — and dropped the
combined item they both descend from.
kyleve added a commit that referenced this pull request Jul 27, 2026
… their modules (#143)

Resolves the two relocation items filed in #142: the `PeriscopeViewer` and `SwiftDataInspector` snapshot suites now live in the modules that own those views, and every module's image suite runs from one scheme and one CI job.

**Stacked on #142** (the TODO-filing PR) — it edits the same `TODOs.md` files. Review/merge that first.

## The design changed during the work, deliberately

The filed TODO assumed each module would get its own `*SnapshotTests` bundle, scheme, and CI job. That turned out to be **actively unsafe**, so it wasn't built.

Each `.xctest` statically embeds what it links, so a second snapshot bundle carries a second copy of `SnapshotKitTesting` — whose "process-global" capture state is *module*-global, and therefore per copy. Verified with `nm` against the two bundles Xcode already builds side by side into `StuffTestHost.app/PlugIns/`:

```
WhereUISnapshotTests:     0000000000e12860 b _..._swizzleDepth...
SnapshotKitTestingTests:  00000000001f0980 b _..._swizzleDepth...
```

Two such copies co-loaded into one host process would count separate swizzle depths against the one shared `UIView` method exchange — flipping parity so captures silently render with the simulator's real safe-area insets — and neither `SnapshotCaptureLock` could see the other's captures. They never co-load today only because they sit in different schemes.

And per-module bundles were never needed for the actual goal. swift-snapshot-testing derives the `__Snapshots__` directory from the calling file's `#filePath`, which `assertSnapshots` threads through, so **a suite records its references next to itself wherever it lives**. Ownership follows the source directory, not the target.

So the aggregation happens at the bundle level: `WhereUISnapshotTests` → `StuffSnapshotTests`, one `sources` directory per module, one scheme, one CI job. This keeps one copy of the pipeline, which is what makes those types' "process wide" docs true rather than aspirational. The rule is now recorded in the root `AGENTS.md` and next to the state it protects in `SnapshotKitTesting/AGENTS.md`.

## The two moves

**`PeriscopeViewer` — zero pixels changed.** Git records both references as `R100` and the suite passes against them untouched. That disproves the old suite's own comment: it wrapped the viewer in `whereBroadwayRoot()` "so it renders with app styling", but `PeriscopeViewer` seeds its own `periscopeBroadwayRoot()` internally (`PeriscopeViewer.swift:89`), which re-seeds from the same system traits. The Where root was contributing nothing.

**`SwiftDataInspector` — genuinely re-recorded.** The old suite reached through `PreviewSupport.previewServices()`, a seeded manual day, and a real `WhereSession` just to obtain a configuration, so an app-agnostic module was snapshotted through Where's store and its references showed Where's schema. It now uses a local in-memory fixture schema, matching the module's unit tests and what a non-Where consumer would see. The new references show three fixture entities with their seeded row counts (3 / 1 / 12) and no design-system root.

## Also

- Corrects the now-stale pointer in `SwiftDataInspectorWiringTests`, which credited the moved snapshot with covering rendering against Where's store. It no longer does, making that wiring test the only thing tying the inspector to the real schema.
- Rewrites the two dependent TODO items whose blocker is gone. Periscope's hosting-test conversion now names the real remaining work (per-view fixtures, a `SnapshotKit` dependency for `SnapshotProviding` conformances since the module has **no** `#Preview`s today, and `OpenSpansView`'s ticking `TimelineView` as the one genuine determinism problem). SwiftDataInspector's drops to `quick-win`.
- The completed root item records why the per-module-bundle prerequisite was abandoned, so nobody rebuilds it.

## Verification

- Full `StuffSnapshotTests` suite green: 27 suites, no recording, **zero reference modifications** — including `SnapshotCaptureFlagProbeTests`, the guard that the capture flag still resolves across the WhereUI boundary after the rename.
- `Stuff-iOS-Tests` green.
- `./swiftformat --lint` clean.
- New references confirmed LFS-tracked.
kyleve added a commit that referenced this pull request Jul 27, 2026
`StuffTestHost` force-linked `WhereCore` so that `Stuff_WhereCore.bundle` and — transitively — the GeoJSON `Stuff_RegionKit.bundle` were embedded in the host, on the theory that a hosted test bundle's `Bundle.module` resolves against the host's main bundle.

**On Xcode 27 that's no longer necessary.** Each `.xctest` now carries its own copies of the resource bundles for the code it links — `WhereUITests.xctest` ships `Stuff_WhereCore.bundle`, `Stuff_RegionKit.bundle`, `Stuff_WhereUI.bundle`, and `Stuff_LifecycleKitUI.bundle` — so SwiftPM's `Bundle.module` resolves via `Bundle(for: BundleFinder.self)` (the test bundle) and never falls back to `Bundle.main`.

So every bundle in the scheme was paying for a host dependency nothing needed, and the full GeoJSON set was being embedded twice.

## On the old comment

It cited two canaries as proof the dependency was load-bearing:

| Canary | Status |
|---|---|
| `WhereUITests.StringsTests` | **No longer exists** — went with the String Catalog symbol migration that deleted the hand-maintained `Strings` facades |
| `SwiftDataInspectorWiringTests` | Still exists, passes without the dependency |

The claim quietly stopped being true rather than being wrong when written. That's why the replacement comment records *what makes it unnecessary* and what to do instead of re-adding a product here, rather than just deleting the note — the next person hitting a missing-resource failure should give the bundle that needs the resource a dependency on the product that owns it, not re-embed it in the host for everyone.

## Verification

- `Stuff-iOS-Tests` green.
- `WhereUISnapshotTests` green — the resource-sensitive one (`Assets.car`, GeoJSON, string catalogs), with **zero reference images modified**. This is the scheme that would fail loudly if a resource lookup silently degraded.
- Built host confirmed to contain no WhereCore symbols and no resource bundles at all (`StuffTestHost.app` is now just the executable, dylib, and `PlugIns/`).
- `./swiftformat --lint` clean.

## Notes

Closes the WhereCore half of the `StuffTestHost` item in `TODOs.md`; the scene-configuration-name duplication half stays open and is re-filed as its own `quick-win`.

Independent of #142 / #143 and mergeable in any order, though all three touch `TODOs.md`, so whichever lands last may need a trivial conflict resolution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant