Split the image snapshots into one bundle per module#146
Merged
Conversation
StuffSnapshotTests compiled three modules' image suites into a single test target. This gives each module its own bundle -- WhereUISnapshotTests, PeriscopeToolsSnapshotTests, SwiftDataInspectorSnapshotTests -- all listed in the one StuffSnapshotTests scheme, so CI still runs them in a single invocation. The single bundle was my mistake in #143, and the reasoning behind it was wrong. I claimed a second snapshot bundle would be unsafe because several .xctest bundles in one scheme load into one StuffTestHost process, where each bundle's own copy of SnapshotKitTesting's capture state (the safe-area swizzle's depth counter, SnapshotCaptureLock, the UIView category) would fight over the single shared UIView method exchange. The premise was inherited from the root AGENTS.md double-linking note rather than measured. It is false. Probing ProcessInfo.processIdentifier from two bundles in one scheme reports different PIDs -- 12076 vs 12146 on a full unfiltered Stuff-iOS-Tests run, and likewise on a filtered one. xcodebuild gives each test bundle its own host process, so each copy of that state is genuinely process-wide, exactly as its documentation claims. The hazard is real but needs co-loading, which does not happen. Splitting is better than merely permissible: a module's image bundle now links only what that module needs. PeriscopeToolsSnapshotTests depends on PeriscopeTools and SwiftDataInspectorSnapshotTests on SwiftDataInspector, so neither builds against WhereUI any more -- the layering wart I flagged when arguing for the shared bundle is gone. References were already per-module (swift-snapshot-testing derives __Snapshots__ from the caller's #filePath), so no image moved and none were re-recorded. Docs updated to match, replacing the "never add a second image-snapshot bundle" rule with the measurement and the assumption it rests on: if a toolchain ever starts sharing one host process across bundles, re-measure before adding another. The #143 completed TODO keeps its wrong reasoning with a correction appended rather than being edited away -- two load-bearing claims in these docs have now turned out to be inherited rather than verified, and that pattern is worth leaving visible. Verified: all three bundles green from the one scheme (three separate "All tests" boundaries in the output, which is the process separation showing), no reference images modified; Stuff-iOS-Tests green; swiftformat --lint clean.
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
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.
Gives each module its own image-snapshot bundle —
WhereUISnapshotTests,PeriscopeToolsSnapshotTests,SwiftDataInspectorSnapshotTests— all listed in the singleStuffSnapshotTestsscheme, so CI still runs them in one invocation.Stacked on #145. Review/merge that first.
This corrects my mistake in #143
I argued there for a single shared bundle, on the grounds that a second one would be unsafe: several
.xctestbundles in one scheme load into oneStuffTestHostprocess, where each bundle's own copy ofSnapshotKitTesting's capture state (the safe-area swizzle's depth counter,SnapshotCaptureLock, theUIViewcategory) would fight over the single sharedUIViewmethod exchange.The premise is false. I inherited "one scheme means one process" from the root
AGENTS.mddouble-linking note instead of measuring it. ProbingProcessInfo.processIdentifierfrom two bundles in one scheme:Different PIDs, on both a filtered and a full run. xcodebuild gives each test bundle its own host process, so each copy of that state is genuinely process-wide — exactly as its documentation claims. The hazard is real, but it needs co-loading, which doesn't happen.
Splitting is better than merely permissible
Each module's bundle now links only what that module needs.
PeriscopeToolsSnapshotTestsdepends onPeriscopeToolsandSwiftDataInspectorSnapshotTestsonSwiftDataInspector— neither builds against WhereUI any more. That's the layering wart I explicitly accepted when arguing for the shared bundle, now gone.References were already per-module (swift-snapshot-testing derives
__Snapshots__from the caller's#filePath), so no image moved and none were re-recorded.Docs
The "never add a second image-snapshot bundle" rule is replaced with the measurement and the assumption it rests on: if a toolchain ever starts sharing one host process across bundles, re-measure before adding another. Updated across the root
AGENTS.md,SnapshotKitTesting, and the three modules'AGENTS.md/README.md.The #143 completed TODO keeps its wrong reasoning with a correction appended rather than being edited away. Two load-bearing claims in these docs have now turned out to be inherited rather than verified — the "WhereUI is a dynamic framework" one in #145, and this — and that pattern seems worth leaving visible.
Note on #145
Its filed spike item existed to make a second snapshot bundle safe. That motivation is now moot, so this PR reframes the entry: the findings stay recorded (they're still accurate about how dynamic linking behaves), but it's explicit that the reason for wanting it evaporated.
Verification
All testsboundaries in the output — the process separation showing up directly.Stuff-iOS-Testsgreen../swiftformat --lintclean.