diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 481aa8d9..1f5935cb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -155,10 +155,10 @@ jobs: # Image snapshots are slow and LFS-backed, so they run in their own job via # the standalone `StuffSnapshotTests` scheme (declared in Project.swift) # rather than the aggregate `Stuff-iOS-Tests` scheme the `test` job uses. - # That one scheme covers every module's image suite: they all compile into - # a single bundle, deliberately — see the `StuffSnapshotTests` comment in - # Project.swift for why a second snapshot bundle in this process would - # break the capture pipeline's process-global state. + # That one scheme gathers every module's image bundle + # (WhereUISnapshotTests, PeriscopeToolsSnapshotTests, + # SwiftDataInspectorSnapshotTests), so this stays a single invocation as + # more modules opt in. # # Deliberately NOT `-parallel-testing-enabled`: the capture pipeline is # single-tenant — every test renders into the one StuffTestHost key diff --git a/AGENTS.md b/AGENTS.md index 9af9f0a0..2fb2377e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -204,11 +204,12 @@ that nothing in this repo depends on a skill having been loaded. `AGENTS.md` says what it is and how it may be used. - Add SPM library targets in `Package.swift` and wire apps/tests in `Project.swift` (see existing `unitTests` helper). A new module also ships a root `README.md` and `AGENTS.md` — see [Per-module docs](#per-module-docs). - **CI scheme**: CI runs the explicit shared **Stuff-iOS-Tests** scheme (all test bundles) rather than the autogenerated `Stuff-Workspace` scheme. New test bundles must be added to the `Stuff-iOS-Tests` scheme in `Project.swift` or CI won't run them. -- **Image snapshots are the exception, and they all live in one bundle**: **StuffSnapshotTests** is slow and LFS-backed, so it runs via its own standalone scheme (declared in `Project.swift`) and a dedicated CI `snapshot` job — deliberately **not** in `Stuff-iOS-Tests`. Reference images under any `__Snapshots__/` directory are stored in **Git LFS** (see `.gitattributes`); the CI job checks out with `lfs: true`. The framework halves live in `Shared/SnapshotKit` (shippable matrix + previews) and `Shared/SnapshotKitTesting` (test-only capture/compare pipeline). The pipeline's own regression tests are a *separate* bundle, **SnapshotKitTestingTests**, which pixel-probes captures (no LFS references) and so runs in the normal `Stuff-iOS-Tests` scheme / `test` job — only the image-reference bundle takes the standalone-scheme + `snapshot`-job route. -- **Never add a second image-snapshot bundle.** A new module's image suite joins `StuffSnapshotTests` by adding its `SnapshotTests/` directory to that target's `sources` — it does **not** get a bundle of its own. Every `.xctest` statically embeds what it links, so a second snapshot bundle means a second copy of `SnapshotKitTesting`, whose capture state is module-global and therefore per copy: the safe-area swizzle's depth counter and override globals, `SnapshotCaptureLock`, and the `UIView` category it installs. Two copies in one `StuffTestHost` process would count separate depths against the one shared method exchange (captures silently rendering with the simulator's real safe-area insets) and neither lock would see the other's captures. Ownership still lives with the module: suites go in `/SnapshotTests/` and record references beside themselves, because swift-snapshot-testing derives the `__Snapshots__` directory from the calling file's `#filePath`. Per-module ownership needs per-module *directories*, not per-module bundles. +- **Image snapshots are the exception: one bundle per module, one shared scheme**: each module that owns image references has its own `*SnapshotTests` target (currently **WhereUISnapshotTests**, **PeriscopeToolsSnapshotTests**, **SwiftDataInspectorSnapshotTests**), and they are all listed in the single shared **StuffSnapshotTests** scheme, which a dedicated CI `snapshot` job runs in one invocation. They are slow and LFS-backed, so they stay deliberately **out of** `Stuff-iOS-Tests`. Reference images under any `__Snapshots__/` directory are stored in **Git LFS** (see `.gitattributes`); the CI job checks out with `lfs: true`. The framework halves live in `Shared/SnapshotKit` (shippable matrix + previews) and `Shared/SnapshotKitTesting` (test-only capture/compare pipeline). The pipeline's own regression tests are a *separate* bundle, **SnapshotKitTestingTests**, which pixel-probes captures (no LFS references) and so runs in the normal `Stuff-iOS-Tests` scheme / `test` job. +- **A new image suite gets a target, not a scheme.** Add a `*SnapshotTests` target over the module's `SnapshotTests/` directory, list only `SnapshotKitTesting` in `extraPackageProducts`, and add it to the `StuffSnapshotTests` scheme's build and test target lists — never a scheme or CI job of its own. A module's image bundle should link only what that module needs, which is the point of splitting them: the Periscope and SwiftDataInspector suites don't build against WhereUI at all. References follow the sources automatically, since swift-snapshot-testing derives the `__Snapshots__` directory from the calling file's `#filePath`. +- **Why separate snapshot bundles are safe — and what would make them unsafe.** Each `.xctest` statically embeds its own copy of `SnapshotKitTesting`, whose capture state is module-global and therefore per copy: the safe-area swizzle's depth counter and override globals, `SnapshotCaptureLock`, and the `UIView` category it installs. Two such copies **in one process** would count separate depths against the single shared `UIView` method exchange (captures silently rendering with the simulator's real safe-area insets), and neither lock would see the other's captures. That doesn't happen because **xcodebuild gives each test bundle its own `StuffTestHost` process** — measured on Xcode 27 by probing `ProcessInfo.processIdentifier` from two bundles in one scheme, which reported different PIDs on both a filtered and a full unfiltered run. This is the load-bearing assumption behind the layout: if a toolchain ever starts sharing one host process across bundles, re-measure before adding another image bundle. - A UI module that opts into image snapshots adds a `SnapshotTests/` folder alongside the usual `Sources/`+`Tests/` skeleton, holding the bundle and its LFS-tracked `__Snapshots__/` references. -### Never double-link a product a dynamic framework already carries +### Never double-link a product WhereUI already carries A target that depends on **WhereUI** must not also list any of WhereUI's own dependencies (WhereCore, Broadway, LifecycleKit/LifecycleKitUI, Periscope, @@ -226,6 +227,26 @@ comment in [`Project.swift`](Project.swift) — and `WhereStylesheetTests.resolvesTraitAwareTokensFromTheBroadwayRoot` is the guard that fails if a duplicate copy answers. +**Nothing in this project is a dynamic framework.** Earlier versions of this +rule (and of `SnapshotKitTesting/AGENTS.md`) described WhereUI as one; that is +not what gets built. The local package is wired as an `XCLocalSwiftPackageReference` +and handed to Xcode's own SPM integration, which links every product statically +into each consumer: no `.framework` products exist and every `.app/Frameworks` +is empty. So "WhereUI carries its dependencies" means *statically embeds them +into whatever links WhereUI*, and a double-link lands two copies in one image. +The rule above is unchanged; only its stated mechanism was wrong. + +One nuance measured while establishing that, because it predicts which +duplications actually bite: **`external` symbols coalesce across images, +`non-external` ones never do.** A `public` type's metadata is external, so dyld +picks one definition and a type-keyed lookup still agrees — verified by +splitting SnapshotKit across two images and watching `\.isCapturingSnapshot` +keep working. A file-scope `private` global or an `enum`'s `static var` is +`non-external` and is genuinely per copy — which is why `SnapshotKitTesting`'s +capture state duplicates. This does not license double-linking (the historical +failure was real and is still guarded), but it does mean the guard test is the +authority on whether a given duplication is harmful, not the general claim. + ## Deployment Platforms and minimum OS live in [`Project.swift`](Project.swift). To get the diff --git a/Project.swift b/Project.swift index dc3d1567..7ce73d63 100644 --- a/Project.swift +++ b/Project.swift @@ -427,59 +427,60 @@ let project = Project( productDependency: "WhereIntents", sources: ["Where/WhereIntents/Tests/**"], ), - // Every image snapshot suite in the repo, in ONE bundle. Slow + - // LFS-backed, so it runs in its own `snapshot` CI job (see - // .github/workflows/ci.yml) via its own `testScheme` below — it is - // deliberately NOT in the `Stuff-iOS-Tests` scheme, keeping image - // snapshots out of the main `test` job. + // Image snapshot bundles: one per module that owns image references, + // all gathered into the single `StuffSnapshotTests` scheme below so CI + // runs them in one `snapshot` job. They are slow and LFS-backed, so + // they are deliberately NOT in the `Stuff-iOS-Tests` scheme. // - // One bundle, not one per module, and that is load-bearing rather than - // convenience. Every `.xctest` statically embeds what it links, so a - // second snapshot bundle would carry a second copy of - // `SnapshotKitTesting` — and its "process-global" capture state is - // module-global, i.e. per copy: `_swizzleDepth` and the override - // globals in `SafeAreaInsetsSwizzling.swift`, `SnapshotCaptureLock`, - // and the `UIView.snapshotKitOverriddenSafeAreaInsets` category. Two - // copies loaded into one `StuffTestHost` process would each count - // their own swizzle depth against the one shared `UIView` method - // exchange (parity flips → captures silently rendering with the - // simulator's real safe-area insets), and neither copy's capture lock - // could see the other's captures. Verified with `nm` on the built - // bundles, which show a private `_swizzleDepth` per `.xctest`. Keeping - // one bundle keeps one copy, which is what makes those types' "process - // wide" docs true. Suites still live in — and record their references - // next to — the module they cover: swift-snapshot-testing derives the - // `__Snapshots__` directory from the calling file's `#filePath`, which - // `assertSnapshots` threads through, so per-module ownership needs - // per-module *source directories*, not per-module bundles. + // One bundle per module rather than one shared bundle, because a + // module's image suite should link only what that module needs: the + // Periscope and SwiftDataInspector suites don't build against WhereUI + // at all. Each records its references beside its own sources — + // swift-snapshot-testing derives the `__Snapshots__` directory from the + // calling file's `#filePath`, which `assertSnapshots` threads through. // - // Lists only `SnapshotKitTesting` in `extraPackageProducts`: the - // test-only capture pipeline, which WhereUI deliberately never links. - // PeriscopeTools and SwiftDataInspector are NOT listed — they arrive - // transitively through WhereUI, and re-listing either would land the - // duplicate copy the root AGENTS.md "Targets" note warns about. Their - // suites import them anyway, exactly as WhereUITests imports WhereCore - // and RegionKit without listing them. + // Separate bundles are safe here because each `.xctest` gets its own + // `StuffTestHost` process: measured on Xcode 27 by probing + // `ProcessInfo.processIdentifier` from two bundles in one scheme, which + // reported different PIDs on both a filtered and a full unfiltered run. + // That matters specifically for these bundles, because each statically + // embeds its own copy of `SnapshotKitTesting`, whose capture state is + // module-global and therefore per copy (`_swizzleDepth` and the + // override globals in `SafeAreaInsetsSwizzling.swift`, + // `SnapshotCaptureLock`, the `UIView` category). Co-loaded into one + // process those copies would fight over the single `UIView` method + // exchange; in separate processes each is genuinely process-wide, as + // its docs claim. If a future toolchain ever starts sharing one host + // process across bundles, re-measure before adding a fourth. // - // `SnapshotKitTesting` embeds its dependency closure, SnapshotKit - // included — which WhereUI carries too — but that does *not* land a - // second copy here: the linker coalesces them. Verified on the built - // bundle, which defines exactly one set of SnapshotKit symbols, the - // same count as WhereUITests (a bundle that links no extra products at - // all). So `\.isCapturingSnapshot` has no copy boundary to cross. - // `SnapshotCaptureFlagProbeTests` still pins the path end to end — the + // Each lists only `SnapshotKitTesting` in `extraPackageProducts` — the + // test-only capture pipeline, which no shipping module links. Nothing + // else is re-listed: whatever the module already carries arrives + // transitively, per the double-linking rule in the root AGENTS.md. + // `SnapshotCaptureFlagProbeTests` (in WhereUISnapshotTests) pins the // pipeline's `traitOverrides` write reaching a WhereUI-defined view's - // read — so a toolchain that stopped coalescing would fail loudly - // rather than silently returning defaults. + // `\.isCapturingSnapshot` read, so a toolchain that stopped coalescing + // the two SnapshotKit copies in that bundle would fail loudly rather + // than silently returning defaults. unitTests( - name: "StuffSnapshotTests", - bundleIdSuffix: "snapshot", + name: "WhereUISnapshotTests", + bundleIdSuffix: "whereui.snapshot", productDependency: "WhereUI", - sources: [ - "Where/WhereUI/SnapshotTests/**", - "Shared/Periscope/PeriscopeTools/SnapshotTests/**", - "Shared/SwiftDataInspector/SnapshotTests/**", - ], + sources: ["Where/WhereUI/SnapshotTests/**"], + extraPackageProducts: ["SnapshotKitTesting"], + ), + unitTests( + name: "PeriscopeToolsSnapshotTests", + bundleIdSuffix: "periscopetools.snapshot", + productDependency: "PeriscopeTools", + sources: ["Shared/Periscope/PeriscopeTools/SnapshotTests/**"], + extraPackageProducts: ["SnapshotKitTesting"], + ), + unitTests( + name: "SwiftDataInspectorSnapshotTests", + bundleIdSuffix: "swiftdatainspector.snapshot", + productDependency: "SwiftDataInspector", + sources: ["Shared/SwiftDataInspector/SnapshotTests/**"], extraPackageProducts: ["SnapshotKitTesting"], ), .target( @@ -609,24 +610,43 @@ let project = Project( testScheme(name: "WhereCoreTests"), testScheme(name: "WhereTests"), testScheme(name: "WhereUITests"), - // Pins the environment the LFS reference images were recorded on. The - // `assertSnapshots` runner compares the SNAPSHOT_EXPECTED_* values - // against the live simulator and fails fast with one clear message on - // a mismatched runtime, screen scale, or timezone — instead of - // hundreds of confusing image diffs. TZ pins the test process's - // timezone itself: several references bake Pacific wall-clock - // dates/times into the image (widget day labels, log-viewer - // timestamps), so an unpinned UTC CI runner would shift every - // date-rendering snapshot. SNAPSHOT_EXPECTED_TIMEZONE is the guard - // that verifies the TZ pin actually reached the test process. - testScheme( + // Every image-snapshot bundle, in one scheme, so CI runs them all in + // the single `snapshot` job. A new module's image suite gets its own + // `*SnapshotTests` target above and joins the lists here — it must not + // get a scheme (or CI job) of its own. + // + // The environment pins are why this scheme exists rather than folding + // the bundles into `Stuff-iOS-Tests`. The `assertSnapshots` runner + // compares the SNAPSHOT_EXPECTED_* values against the live simulator + // and fails fast with one clear message on a mismatched runtime, + // screen scale, or timezone — instead of hundreds of confusing image + // diffs. TZ pins the test process's timezone itself: several + // references bake Pacific wall-clock dates/times into the image + // (widget day labels, log-viewer timestamps), so an unpinned UTC CI + // runner would shift every date-rendering snapshot. + // SNAPSHOT_EXPECTED_TIMEZONE is the guard that verifies the TZ pin + // actually reached the test process. + .scheme( name: "StuffSnapshotTests", - testEnvironmentVariables: [ - "SNAPSHOT_EXPECTED_SIMULATOR_RUNTIME_VERSION": "27.0", - "SNAPSHOT_EXPECTED_SCREEN_SCALE": "3", - "SNAPSHOT_EXPECTED_TIMEZONE": "America/Los_Angeles", - "TZ": "America/Los_Angeles", - ], + shared: true, + buildAction: .buildAction(targets: [ + "WhereUISnapshotTests", + "PeriscopeToolsSnapshotTests", + "SwiftDataInspectorSnapshotTests", + ]), + testAction: .targets( + [ + "WhereUISnapshotTests", + "PeriscopeToolsSnapshotTests", + "SwiftDataInspectorSnapshotTests", + ], + arguments: .arguments(environmentVariables: [ + "SNAPSHOT_EXPECTED_SIMULATOR_RUNTIME_VERSION": "27.0", + "SNAPSHOT_EXPECTED_SCREEN_SCALE": "3", + "SNAPSHOT_EXPECTED_TIMEZONE": "America/Los_Angeles", + "TZ": "America/Los_Angeles", + ]), + ), ), testScheme(name: "WhereIntentsTests"), testScheme(name: "BroadwayCoreTests"), diff --git a/Shared/Periscope/PeriscopeTools/AGENTS.md b/Shared/Periscope/PeriscopeTools/AGENTS.md index 077a6b16..a8d536ee 100644 --- a/Shared/Periscope/PeriscopeTools/AGENTS.md +++ b/Shared/Periscope/PeriscopeTools/AGENTS.md @@ -51,7 +51,7 @@ use `PeriscopeStylesheet.default`. - Because PeriscopeTools links Broadway as a **static** library it can seed Broadway directly. Were it ever to become a dynamic framework, or be embedded in one, the root - [double-linking rule](../../../AGENTS.md#never-double-link-a-product-a-dynamic-framework-already-carries) + [double-linking rule](../../../AGENTS.md#never-double-link-a-product-whereui-already-carries) would start applying to its consumers. ## Invariants @@ -94,10 +94,11 @@ directly, and host views with `TestHostSupport`'s `show()` helpers. How the tools *look* is pinned separately by the image snapshots in [`SnapshotTests/`](SnapshotTests), with references under `SnapshotTests/__Snapshots__/` in Git LFS. Those files compile into the -repo-wide `StuffSnapshotTests` bundle rather than `PeriscopeToolsTests` — there -is exactly one image-snapshot bundle in the repo, for reasons the root -[`AGENTS.md`](../../../AGENTS.md#targets) explains — but they live and record -here, so this module owns its own reference images. Seed a **frozen** store +`PeriscopeToolsSnapshotTests` bundle rather than `PeriscopeToolsTests` — image +suites get their own target, gathered with every other module's into the shared +`StuffSnapshotTests` scheme and its CI job (root +[`AGENTS.md`](../../../AGENTS.md#targets)). It links PeriscopeTools directly, so +nothing here builds against an app module. Seed a **frozen** store (fixed records at timestamps pinned to a fixed instant); the process-global Periscope store's wall-clock timestamps would churn the images. Tool views seed their own `periscopeBroadwayRoot()`, so a snapshot needs no host-app root and diff --git a/Shared/Periscope/PeriscopeTools/README.md b/Shared/Periscope/PeriscopeTools/README.md index 1f7a692d..3e895c50 100644 --- a/Shared/Periscope/PeriscopeTools/README.md +++ b/Shared/Periscope/PeriscopeTools/README.md @@ -112,8 +112,9 @@ stores (`@_spi(Testing) PeriscopeStore.inMemory`), views host via How the tools render is pinned by image snapshots in [`SnapshotTests/`](SnapshotTests), with reference images under -`SnapshotTests/__Snapshots__/` in Git LFS. They compile into the repo-wide -`StuffSnapshotTests` bundle (one snapshot bundle for the whole repo) but record -their references here. Run with `tuist test StuffSnapshotTests`; to re-record +`SnapshotTests/__Snapshots__/` in Git LFS. They build as the module's own +`PeriscopeToolsSnapshotTests` bundle, which runs alongside every other module's +image suite in the shared `StuffSnapshotTests` scheme. Run with +`tuist test StuffSnapshotTests`; to re-record after an intentional UI change, see the [SnapshotKitTesting README](../../SnapshotKitTesting/README.md#recording). diff --git a/Shared/SnapshotKitTesting/AGENTS.md b/Shared/SnapshotKitTesting/AGENTS.md index a187f747..9e82fbe1 100644 --- a/Shared/SnapshotKitTesting/AGENTS.md +++ b/Shared/SnapshotKitTesting/AGENTS.md @@ -13,29 +13,30 @@ Complements the root [`AGENTS.md`](../../AGENTS.md) — read that first. comparison engine + XCTest/Testing, so it is **only** consumed by the `StuffSnapshotTests` and `SnapshotKitTestingTests` bundles via `extraPackageProducts` — **never** a shipping app or `StuffTestHost`. -- **Exactly one image-snapshot bundle may exist, and this module is why.** - Everything below described as "process-global" — the safe-area swizzle's - depth counter and override globals, `SnapshotCaptureLock`, the animations - save/restore — is *module*-global, so it is per copy of this module. Each - `.xctest` statically embeds what it links, so two snapshot bundles co-loaded - into one `StuffTestHost` process would hold two independent copies of that - state while sharing one `UIView` method exchange: the depth counters flip - parity against each other (captures silently render with the simulator's - real safe-area insets) and neither capture lock can see the other's - captures. Verified with `nm` on the built bundles — each defines its own - private `_swizzleDepth`. Hence one bundle, `StuffSnapshotTests`; a new - module's image suite joins it as another `sources` directory (root - [`AGENTS.md`](../../AGENTS.md#targets)). `SnapshotKitTestingTests` is the - sanctioned second consumer only because it runs in a *different* scheme, so - the two never share a process. +- **"Process-global" here means per *process*, and that is only safe because + each test bundle gets its own.** Everything below described as + process-global — the safe-area swizzle's depth counter and override globals, + `SnapshotCaptureLock`, the animations save/restore — is *module*-global, so + it is one copy per copy of this module, and each `.xctest` statically embeds + its own (verified with `nm`: every consuming bundle defines a private + `_swizzleDepth`). Two copies **co-loaded into one process** would hold + independent depth counters against the single shared `UIView` method + exchange — parity flips, captures silently render with the simulator's real + safe-area insets, and neither lock sees the other's captures. xcodebuild + gives each bundle its own `StuffTestHost` process (measured on Xcode 27 via + `ProcessInfo.processIdentifier` probes from two bundles in one scheme), so + the several image bundles and `SnapshotKitTestingTests` never collide. Treat + that as this module's load-bearing environmental assumption: if bundles ever + start sharing a host process, this state has to become genuinely + process-wide before another consumer is added. - **The consuming bundle double-embeds `SnapshotKit`, tolerated and guarded.** Listing this product in `extraPackageProducts` statically embeds its - dependency closure — including `SnapshotKit` — into the `.xctest`, while a - dynamic-framework dependency (WhereUI) carries its own copy: the + dependency closure — including `SnapshotKit` — into the `.xctest`, and + WhereUI statically embeds its own copy into the same image: the duplicate-type-metadata hazard from the root `AGENTS.md` "Targets" note, with `\.isCapturingSnapshot` as the type-keyed cross-boundary lookup at risk. There is no cleaner wiring (the pipeline must reach the bundle without - ever linking into the UI framework), the trait lookup demonstrably resolves + ever linking into the UI module), the trait lookup demonstrably resolves across both copies today, and `StuffSnapshotTests.SnapshotCaptureFlagProbeTests` fails loudly if the copies ever split — see the StuffSnapshotTests comment in diff --git a/Shared/SwiftDataInspector/AGENTS.md b/Shared/SwiftDataInspector/AGENTS.md index e70d397a..722f45bd 100644 --- a/Shared/SwiftDataInspector/AGENTS.md +++ b/Shared/SwiftDataInspector/AGENTS.md @@ -52,10 +52,11 @@ in-memory `ModelContainer` with local `@Model` fixtures, drive wiring is covered from the consumer side (`WhereUITests`). How it renders is pinned by the image snapshots in -[`SnapshotTests/`](SnapshotTests), which compile into the repo-wide -`StuffSnapshotTests` bundle but record their references here — there is exactly -one image-snapshot bundle in the repo, for reasons the root -[`AGENTS.md`](../../AGENTS.md#targets) explains. Snapshot the module the way a +[`SnapshotTests/`](SnapshotTests), which build as the module's own +`SwiftDataInspectorSnapshotTests` bundle — image suites get their own target, +gathered with every other module's into the shared `StuffSnapshotTests` scheme +and its CI job (root [`AGENTS.md`](../../AGENTS.md#targets)). It links only +`SwiftDataInspector`, so nothing here builds against an app module. Snapshot the module the way a consumer gets it: a **local fixture schema**, never a host app's store, and no design-system root. Prefer adding a case there over a hosted "renders without crashing" test, which asserts nothing about the result. Fixture `@Model` names diff --git a/Shared/SwiftDataInspector/README.md b/Shared/SwiftDataInspector/README.md index 5c287d37..b561ada3 100644 --- a/Shared/SwiftDataInspector/README.md +++ b/Shared/SwiftDataInspector/README.md @@ -205,8 +205,9 @@ How the inspector *renders* is pinned by image snapshots in [`SnapshotTests/`](SnapshotTests), with reference images under `SnapshotTests/__Snapshots__/` in Git LFS. They build the same kind of local in-memory fixture schema and apply no design-system root, so the capture shows -what a consumer actually gets rather than any one host app's styling. The files -compile into the repo-wide `StuffSnapshotTests` bundle (one snapshot bundle for -the whole repo) but record their references here. Run with `tuist test +what a consumer actually gets rather than any one host app's styling. They build +as the module's own `SwiftDataInspectorSnapshotTests` bundle, which runs +alongside every other module's image suite in the shared `StuffSnapshotTests` +scheme. Run with `tuist test StuffSnapshotTests`; to re-record after an intentional UI change, see the [SnapshotKitTesting README](../SnapshotKitTesting/README.md#recording). diff --git a/TODOs.md b/TODOs.md index 9ee5fe10..bdd5e944 100644 --- a/TODOs.md +++ b/TODOs.md @@ -98,6 +98,11 @@ inbox rather than here. ## P1s (Should do) ## P2s (Nice to have) +- refactor(SnapshotKitTesting) [needs-design]: Dynamically link `SnapshotKitTesting` so its capture state is one copy per *process image* rather than one per bundle. **Spiked; it works, was deliberately not landed, and its original motivation has since evaporated** — kept only so a future attempt starts from the findings rather than the dead ends. The spike existed to make a second image-snapshot bundle safe; measuring afterwards showed xcodebuild already gives each `.xctest` its own `StuffTestHost` process (different `ProcessInfo.processIdentifier` from two bundles in one scheme, on both filtered and full runs), so separate bundles were never unsafe and the repo now ships one per module. Only revisit this if bundles ever start sharing a host process — which is the assumption recorded in [`AGENTS.md`](AGENTS.md#targets) and in `SnapshotKitTesting/AGENTS.md`. (spike 2026-07-26) + - What works: `.library(name: "SnapshotKitTesting", type: .dynamic, …)` in [`Package.swift`](Package.swift) produces a real framework; the `.xctest` then links `@rpath/SnapshotKitTesting.framework` and its private `_swizzleDepth` count drops from one-per-bundle to **zero**. The whole goal, in one line. + - The blocker: a dynamic product that *statically absorbs* a resource-bearing dependency orphans that dependency's resources. `AccessibilitySnapshotParser`'s code moves into the framework while its `.bundle` stays in the `.xctest`, and SwiftPM's generated accessor searches only `Bundle.main.resourceURL`, `Bundle(for: BundleFinder.self).resourceURL`, `Bundle.main.bundleURL` — so `Bundle.module` hits its `fatalError` and **every VoiceOver-annotated capture traps** (`AccessibilitySnapshotBaseView.parseAccessibility()` → `StringLocalization.preferredBundle(for:)`). Verified fix: copy the resource bundles into `PackageFrameworks/SnapshotKitTesting.framework/`, after which the accessibility suites pass with no pixel drift. Automating it needs a build phase against a framework Xcode's SPM integration generates — the same `Bundle.module` placement fragility the StuffTestHost WhereCore embed was, which is the main argument against landing it. + - Dead end: Tuist's `PackageSettings(productTypes:)` does nothing here. The local package is wired as an `XCLocalSwiftPackageReference` and resolved by Xcode's own SPM integration, so Tuist's product-type machinery never applies — only SwiftPM's `type:`. Relatedly, `type: .dynamic` takes effect only for a product an Xcode target consumes *as a product*: WhereUI depends on the SnapshotKit *target*, so SnapshotKit stayed static despite the annotation. + - Trap: **a shared DerivedData reports false negatives here.** Two separate runs reported "no frameworks produced" from an incremental build that had not re-resolved the package graph. Spike this into a fresh `-derivedDataPath` or it will lie to you. - feat(PeriscopeCore) [needs-design]: A `LogSession` can't name the build it came from. `LogSession.current()` reads only `CFBundleShortVersionString` / `CFBundleVersion` off the main bundle (`Shared/Periscope/PeriscopeCore/Sources/Store/LogSession.swift:32`), and the viewer renders that pair as the session's identity (`PeriscopeTools/Sources/Viewer/PeriscopeViewer.swift:269`) — but the Where app pins both in the manifest (`Project.swift:101`), so every developer build reads `v1.0 (1)` and weeks-old logs can't be tied to the code that produced them. The commit is now available: the app stamps `WhereGitSHA` / `WhereGitStatus` into its Info.plist and `WhereCore.BuildInfo` reads them back. PeriscopeCore can't depend on WhereCore (Periscope sits below the Where modules), so this needs a seam rather than a direct adoption — an optional commit field, or general resource attributes on `LogSession` that the host app fills at bootstrap. Spans Periscope and Where, hence here. (agent 2026-07-26) - perf(StuffTestHost) [needs-design]: Two loose ends in the shared test host, both reaching the root Tuist manifest, which is why they sit here rather than in a StuffTestHost file. The WhereCore-always-embedded build trade-off is documented and verified load-bearing at `Project.swift:256` — decide whether to keep documenting it or split the host so unrelated bundles don't pay for it. Separately, the scene configuration name is spelled twice, in `Shared/StuffTestHost/Sources/AppDelegate.swift:11` and `Project.swift:244`, so the two can drift silently. (audit 2026-07-26) @@ -108,3 +113,4 @@ inbox rather than here. ## P2s (Nice to have) - test(WhereUI) [needs-design]: broken-snapshots — two snapshot suites pinned views WhereUI doesn't own (`PeriscopeViewer` and `SwiftDataInspector`), flagged `[Fix later]` on PR #101. (Resolved: both suites, and their reference images, now live under the module they cover. The prerequisite this item assumed — a snapshot *bundle* per module, each with its own scheme and CI job — turned out to be the wrong shape and was **not** built. Per-module bundles are actively unsafe: each `.xctest` statically embeds what it links, so a second one would carry a second copy of `SnapshotKitTesting`, whose "process-global" capture state is module-global and therefore per copy — two copies co-loaded into one `StuffTestHost` would flip the safe-area swizzle's parity against each other and neither capture lock would see the other's captures (verified with `nm`: each built bundle defines its own private `_swizzleDepth`). And per-module bundles were never needed for ownership: swift-snapshot-testing derives the `__Snapshots__` directory from the calling file's `#filePath`, so a suite records beside itself wherever it lives. So the single bundle was renamed `WhereUISnapshotTests` → `StuffSnapshotTests` and gained a `sources` directory per module, keeping one scheme and one CI job. The one-bundle rule is now recorded in the root `AGENTS.md` "Targets" section and in `SnapshotKitTesting/AGENTS.md`. Moving `PeriscopeViewer` changed no pixels — it seeds its own `periscopeBroadwayRoot()`, so the `whereBroadwayRoot()` wrapper the old suite credited for "app styling" was contributing nothing; `SwiftDataInspector` did re-record, since it moved off Where's store onto a local fixture schema.) + - **Correction (2026-07-26, same day):** the "per-module bundles are actively unsafe" reasoning above was wrong, and the single-bundle shape it produced has been undone. It assumed several `.xctest` bundles in one scheme load into one `StuffTestHost` process — inherited from the root `AGENTS.md` double-linking note rather than measured. They don't: probing `ProcessInfo.processIdentifier` from two bundles in one scheme reports different PIDs, on both a filtered and a full unfiltered run (Xcode 27). Each bundle gets its own host process, so its own copy of `SnapshotKitTesting`'s capture state is genuinely process-wide and cannot collide with another bundle's. The repo now has one image bundle per module (`WhereUISnapshotTests`, `PeriscopeToolsSnapshotTests`, `SwiftDataInspectorSnapshotTests`) gathered into the one `StuffSnapshotTests` scheme — which also lets the Periscope and SwiftDataInspector suites stop linking WhereUI. Left here rather than edited away because the wrong reasoning is the useful part: two separate load-bearing claims in these docs turned out to be inherited rather than verified. diff --git a/Where/AGENTS.md b/Where/AGENTS.md index 60376dd2..61e2e46f 100644 --- a/Where/AGENTS.md +++ b/Where/AGENTS.md @@ -297,7 +297,7 @@ Root [testing conventions](../AGENTS.md#testing) apply. What's specific here: remote-import path is exercised with the `@_spi(Testing)` `inMemory(remoteChangeSource:)` + `ScriptedStoreRemoteChangeSource`. - How screens render is pinned by the matrixed image snapshots in - `WhereUI/SnapshotTests/` (compiled into the repo-wide `StuffSnapshotTests` - bundle — own scheme + CI job, not `Stuff-iOS-Tests`) — see + `WhereUI/SnapshotTests/` (the `WhereUISnapshotTests` bundle, run from the + shared `StuffSnapshotTests` scheme + CI job, not `Stuff-iOS-Tests`) — see [`WhereUI/AGENTS.md`](WhereUI/AGENTS.md#testing). Don't add "hosts without crashing" smoke tests for surfaces those suites cover. diff --git a/Where/WhereIntents/AGENTS.md b/Where/WhereIntents/AGENTS.md index 9cab573f..1d487119 100644 --- a/Where/WhereIntents/AGENTS.md +++ b/Where/WhereIntents/AGENTS.md @@ -14,7 +14,7 @@ layering, localization, and the WhereUI duplicate-metadata rule). - Dependencies live in the root [`Package.swift`](../../Package.swift). It depends on **WhereUI** for its snippet cards — mirroring **WhereWidgets** — so it must **not** link `BroadwayUI`/`BroadwayCore` directly (see the root - [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-a-dynamic-framework-already-carries)). + [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-whereui-already-carries)). - Intents stay **thin adapters**: they `await intentServices.current()` and delegate to that `WhereServices`' collaborators. Domain rules, persistence, and aggregation stay in `WhereCore`; presentation (the card bodies) stays in diff --git a/Where/WhereUI/AGENTS.md b/Where/WhereUI/AGENTS.md index c20735ea..efacd9ae 100644 --- a/Where/WhereUI/AGENTS.md +++ b/Where/WhereUI/AGENTS.md @@ -18,7 +18,7 @@ extended, and how its snapshot suites are organized (see [`Package.swift`](../../Package.swift). - Consumers (`WhereWidgets`, `WhereIntents`) get Broadway *through* WhereUI and must **not** link `BroadwayUI`/`BroadwayCore` themselves (see the root - [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-a-dynamic-framework-already-carries)). + [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-whereui-already-carries)). That's why `whereBroadwayRoot()` lives here rather than being called as `broadwayRoot` at each site. - Continuous/looping motion (repeat-forever pulses, `TimelineView(.animation)`, @@ -154,11 +154,10 @@ required previews) live in the feature [`Where/AGENTS.md`](../AGENTS.md). Screens, widgets, and app-flow surfaces are pinned as matrixed image snapshots under [`SnapshotTests/`](SnapshotTests) — those, not hosting smoke tests, own -"does this screen render". The files compile into the repo-wide -`StuffSnapshotTests` bundle (shared with the other modules' image suites; see -the root [`AGENTS.md`](../../AGENTS.md#targets) for why there is exactly one -snapshot bundle), but they live and record their references here, next to the -views they cover. **Each view +"does this screen render". They build as this module's own +`WhereUISnapshotTests` bundle, which runs alongside the other modules' image +suites in the shared `StuffSnapshotTests` scheme and its CI job (root +[`AGENTS.md`](../../AGENTS.md#targets)). **Each view declares its matrix once, in its own source file**, via a `SnapshotProviding` conformance under `#if DEBUG` whose `#Preview` renders `Self.snapshotPreviews` — so one declaration drives both the Xcode cutsheet and the image tests (the diff --git a/Where/WhereUI/README.md b/Where/WhereUI/README.md index 99823789..c066a5f0 100644 --- a/Where/WhereUI/README.md +++ b/Where/WhereUI/README.md @@ -151,8 +151,9 @@ iPhone/iPad, contrast, right-to-left, VoiceOver annotations) in `SnapshotProviding` conformance **in its own source file**, shared with its `#Preview` cutsheet (`Self.snapshotPreviews`); there is one `FooSnapshotTests` suite per view, so each view's references live in their own `__Snapshots__/` -directory. These files compile into the repo-wide `StuffSnapshotTests` bundle -alongside the other modules' image suites, which has its own scheme and CI job; +directory. They build as this module's own `WhereUISnapshotTests` bundle, which +runs alongside the other modules' image suites in the shared +`StuffSnapshotTests` scheme and its CI job; to re-record after an intentional UI change, forward the record mode into the test process (see the [SnapshotKitTesting README](../../Shared/SnapshotKitTesting/README.md#recording) diff --git a/Where/WhereUI/Sources/Shared/MotionIsStatic.swift b/Where/WhereUI/Sources/Shared/MotionIsStatic.swift index fceff17e..c79f7c87 100644 --- a/Where/WhereUI/Sources/Shared/MotionIsStatic.swift +++ b/Where/WhereUI/Sources/Shared/MotionIsStatic.swift @@ -24,7 +24,7 @@ import SwiftUI /// populates their environment before `body`, so the two reads resolve exactly /// as they would on the view itself. Proven end-to-end by the `launchSplash.*` /// captures (the pulse/radar freeze only if this reads the capture flag) and by -/// `StuffSnapshotTests.SnapshotCaptureFlagProbeTests`. +/// `WhereUISnapshotTests.SnapshotCaptureFlagProbeTests`. @propertyWrapper struct MotionIsStatic: DynamicProperty { @Environment(\.accessibilityReduceMotion) private var reduceMotion diff --git a/Where/WhereUI/Sources/Shared/SnapshotCaptureFlagProbe.swift b/Where/WhereUI/Sources/Shared/SnapshotCaptureFlagProbe.swift index 5ae5229d..96e74fad 100644 --- a/Where/WhereUI/Sources/Shared/SnapshotCaptureFlagProbe.swift +++ b/Where/WhereUI/Sources/Shared/SnapshotCaptureFlagProbe.swift @@ -7,9 +7,9 @@ /// /// This view exists to be *compiled into WhereUI* (the dynamic framework) and /// pixel-probed from the snapshot bundle - /// (`StuffSnapshotTests.SnapshotCaptureFlagProbeTests`): the bundle + /// (`WhereUISnapshotTests.SnapshotCaptureFlagProbeTests`): the bundle /// statically embeds a second copy of `SnapshotKit` via `SnapshotKitTesting` - /// (see the StuffSnapshotTests comment in `Project.swift`), so the pipeline + /// (see the snapshot-bundle comment in `Project.swift`), so the pipeline /// *writes* `SnapshotCaptureTrait` in the bundle's copy while WhereUI's /// stand-ins *read* `\.isCapturingSnapshot` through WhereUI's copy. If those /// copies ever split (the duplicate-type-metadata hazard in the root diff --git a/Where/WhereWidgets/AGENTS.md b/Where/WhereWidgets/AGENTS.md index db9cd7bf..24797523 100644 --- a/Where/WhereWidgets/AGENTS.md +++ b/Where/WhereWidgets/AGENTS.md @@ -40,4 +40,4 @@ This file complements the root [`AGENTS.md`](../../AGENTS.md) and the feature add a direct `BroadwayCore`/`BroadwayUI` dependency — Broadway arrives through `WhereUI`, which is why the seam lives there rather than a `broadwayRoot` call here (see the root - [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-a-dynamic-framework-already-carries)). + [`AGENTS.md`](../../AGENTS.md#never-double-link-a-product-whereui-already-carries)).