Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 24 additions & 3 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<Module>/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,
Expand All @@ -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
Expand Down
148 changes: 84 additions & 64 deletions Project.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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"),
Expand Down
11 changes: 6 additions & 5 deletions Shared/Periscope/PeriscopeTools/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading