Second pre-push review on tooling/makefile-and-swiftlint, scoped to the new work since review #1: the flaky-test fix that isolates the App Group UserDefaults per test, plus the Makefile review refinements.
UserDefaults(suiteName:) because Swift Testing runs suites in parallel; .serialized only orders tests within a suite.@TaskLocal suite-name override on SharedDataStore (nil in production) + a recursive .isolatedAppGroupStore trait giving each test its own throwaway suite.@_spi(Testing), documented the task-local invariant, and named the prefix constant.Ready to push
The test-isolation fix is correct and well-scoped. Three review agents found no major or critical issues — only three minor points, all addressed: the test seam is now gated behind @_spi(Testing) (out of the public API), the TaskLocal propagation invariant is documented, and the suite-name prefix is a named constant. The quality agent verified the TaskLocal binding covers every current SharedDataStore caller (no detached task touches it) and that .serialized + .isolatedAppGroupStore are orthogonal, not redundant. Isolation confirmed by 9 consecutive clean full phaseTests runs.
5c597bd [fix]: Isolate App Group UserDefaults per test to fix flaky suite d5e691e [fix]: Address pre-push review on test-isolation seam dd22ce8 [feat]: Address pre-push review findings on build tooling Some of the app's tests were failing randomly — pass one run, fail the next, with different tests failing each time. That's the classic sign of tests stepping on each other's shared data.
The shared data here is a small key-value store (UserDefaults) that the app and its widget use to pass data. Many test groups read and wrote the same store at the same time, so one test could wipe the store while another was mid-check.
Each test now gets its own private copy of that store, created fresh and thrown away afterwards. With no shared store, the tests can't interfere with each other.
The mechanism is a TaskLocal — a value that's visible only within one running test and invisible to others, even when they run at the same time. In the real app this value is never set, so the app behaves exactly as before.
All App Group access funnels through SharedDataStore.sharedDefaults, a computed property that returned UserDefaults(suiteName: appGroupID). The six suites touching it were each .serialized, but Swift Testing runs suites in parallel, so they raced on that one process-global store. (-parallel-testing-worker-count 1 only governs XCTest workers, not Swift Testing's in-process parallelism.)
A @TaskLocal override redirects the suite name per test, bound by a custom TestScoping trait (isRecursive = true so it wraps every test in the suite). Each test gets a unique phase.tests.<uuid> suite and tears it down in a defer. This mirrors the existing KeychainServiceTests unique-namespace convention.
The seam is public on a shared library — necessary because the test target links the compiled package product (so neither package nor @testable reaches it). It's fenced with @_spi(Testing) so plain import PhaseShared clients can't see it.
A single mutable static override would itself be shared global state — two parallel suites binding it would race on the override. A @TaskLocal is bound per task via withValue, so each test's task hierarchy sees its own value with no cross-test interference, preserving parallelism instead of forcing global serialization.
TaskLocal bindings propagate only to work that structurally inherits the task context. An unstructured Task {}/Task.detached started inside the scope escapes the binding and would hit the real store. The quality agent traced every SharedDataStore caller and every detached task in DashboardViewModel (lines 497/580/591/628/634/658) and confirmed none touch the store today; the invariant is now documented at the seam so a future detached call doesn't silently reintroduce the race.
.serialized remains necessary: each suite's MockURLProtocol has a nonisolated(unsafe) static var requestHandler shared by its tests, which must not be mutated concurrently. .isolatedAppGroupStore handles the cross-suite UserDefaults axis; .serialized handles the within-suite static-handler axis. Both are required.
PhaseShared/Sources/PhaseShared/Services/SharedDataStore.swift
Why it matters. Production-facing shared library gains a test seam. Correctness of the nil-in-production path and the API-surface fencing both matter.
What to look at. SharedDataStore.swift:86-101 (suiteNameOverride + sharedDefaults)
phaseTests/IsolatedAppGroupStoreTrait.swift
Why it matters. New test infrastructure; its correctness is what removes the flakiness.
What to look at. phaseTests/IsolatedAppGroupStoreTrait.swift (whole file)
phaseTests/SharedDataStoreTests.swift
Why it matters. Scopes the fix precisely; the two TimingAPIClient suites have zero SharedDataStore calls and were correctly left alone.
What to look at. @Suite(.serialized, .isolatedAppGroupStore) across 6 files
Isolation keeps Swift Testing's parallelism and fixes the root cause (shared store) rather than masking it. Nesting all suites under one .serialized parent would have been a large structural change across seven large files.
The test target links the compiled PhaseShared product, so package access and @testable import don't reach it. @_spi(Testing) keeps the symbol out of the normal public API while letting the trait file opt in via @_spi(Testing) import PhaseShared.
Cheaper to reason about and robust even if a future suite runs its cases in parallel; the per-test cost (a lazy UserDefaults plus a no-op removePersistentDomain) is negligible.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| minor | SharedDataStore API surface | The TaskLocal test seam was plain `public` with a production-looking name, leaking a test concern into the shipped library API. | Gated behind @_spi(Testing); the trait reaches it via `@_spi(Testing) import PhaseShared`. Cleaner alternatives (package access, internal+@testable) don't reach the externally-linked test target. |
| minor | TaskLocal propagation invariant | Isolation relies on no detached Task calling SharedDataStore inside the scope; correct today but unenforced. | Verified no current caller violates it (all SharedDataStore access is on directly-awaited paths) and documented the invariant at the seam. |
| minor | IsolatedAppGroupStoreTrait | The `phase.tests.` suite-name prefix was an inline string literal. | Hoisted to a named constant, matching the KeychainServiceTests convention. |
| minor | SharedDataStoreTests.clearTestData() | Per-test isolation makes the existing key-by-key clearTestData() calls redundant. | Left in place — harmless, defensive, pre-existing; pruning would be test-file churn for no behavioural benefit. |
Click to expand.
diff --git a/PhaseShared/Sources/PhaseShared/Services/SharedDataStore.swift b/PhaseShared/Sources/PhaseShared/Services/SharedDataStore.swiftindex 0974737..dda2734 100644--- a/PhaseShared/Sources/PhaseShared/Services/SharedDataStore.swift+++ b/PhaseShared/Sources/PhaseShared/Services/SharedDataStore.swift@@ -83,9 +83,22 @@ public enum SharedDataStore { /// Required number of widget slots private static let widgetSlotCount = 4 + /// Test seam, gated behind `@_spi(Testing)` so it stays out of the public API.+ /// When bound within a `$suiteNameOverride.withValue(...)` scope, `sharedDefaults`+ /// resolves to that suite instead of the real App Group, so each test can run+ /// against an isolated `UserDefaults`. It is a `TaskLocal` so concurrent tests+ /// don't clobber each other's binding. Always `nil` in production, where+ /// `sharedDefaults` is unchanged. Reach it via `@_spi(Testing) import PhaseShared`.+ ///+ /// The binding only propagates to work that inherits the task context. Tests+ /// must not drive `SharedDataStore` from an unstructured `Task {}`/`Task.detached`+ /// started inside the scope — that work would escape the binding and hit the real+ /// store, reintroducing the cross-suite race this isolates.+ @_spi(Testing) @TaskLocal public static var suiteNameOverride: String?+ /// Shared UserDefaults for App Group nonisolated public static var sharedDefaults: UserDefaults? {- UserDefaults(suiteName: appGroupID)+ UserDefaults(suiteName: suiteNameOverride ?? appGroupID) } // MARK: - Timer Statediff --git a/phaseTests/IsolatedAppGroupStoreTrait.swift b/phaseTests/IsolatedAppGroupStoreTrait.swiftnew file mode 100644index 0000000..d6553a6--- /dev/null+++ b/phaseTests/IsolatedAppGroupStoreTrait.swift@@ -0,0 +1,46 @@+//+// IsolatedAppGroupStoreTrait.swift+// phaseTests+//+// A Swift Testing trait that gives each test its own isolated App Group+// UserDefaults suite.+//++import Foundation+@_spi(Testing) import PhaseShared+import Testing++/// Runs each test against a unique, throwaway `UserDefaults` suite instead of the+/// real App Group store.+///+/// Several suites exercise `SharedDataStore`, whose `sharedDefaults` is a single+/// process-global `UserDefaults(suiteName:)`. Swift Testing runs suites in parallel,+/// so without isolation a test in one suite can clear or overwrite the store while a+/// test in another suite is mid-assertion — producing non-deterministic failures.+///+/// This trait binds `SharedDataStore.suiteNameOverride` (a `TaskLocal`) to a fresh+/// suite name per test and tears the suite down afterwards. `isRecursive` makes it+/// apply to every test in the suite it is attached to.+struct IsolatedAppGroupStore: SuiteTrait, TestTrait, TestScoping {+ /// Matches the unique-namespace convention used by `KeychainServiceTests`.+ private static let suitePrefix = "phase.tests."++ var isRecursive: Bool { true }++ func provideScope(+ for test: Test,+ testCase: Test.Case?,+ performing function: @concurrent @Sendable () async throws -> Void+ ) async throws {+ let suiteName = "\(Self.suitePrefix)\(UUID().uuidString)"+ defer { UserDefaults().removePersistentDomain(forName: suiteName) }+ try await SharedDataStore.$suiteNameOverride.withValue(suiteName) {+ try await function()+ }+ }+}++extension Trait where Self == IsolatedAppGroupStore {+ /// Isolate each test's `SharedDataStore` access to its own `UserDefaults` suite.+ static var isolatedAppGroupStore: IsolatedAppGroupStore { IsolatedAppGroupStore() }+}diff --git a/phaseTests/SharedDataStoreTests.swift b/phaseTests/SharedDataStoreTests.swiftindex 3829521..8a8e467 100644--- a/phaseTests/SharedDataStoreTests.swift+++ b/phaseTests/SharedDataStoreTests.swift@@ -13,7 +13,7 @@ import PhaseShared /// Tests for SharedDataStore using the real App Group UserDefaults /// Note: These tests require the App Group entitlement to be configured /// Tests clean up after themselves to avoid state pollution-@Suite("SharedDataStore Tests", .serialized)+@Suite("SharedDataStore Tests", .serialized, .isolatedAppGroupStore) struct SharedDataStoreTests { // MARK: - Setup / Teardowndiff --git a/phaseTests/FavouritesStoreTests.swift b/phaseTests/FavouritesStoreTests.swiftindex 7d08997..485cc8e 100644--- a/phaseTests/FavouritesStoreTests.swift+++ b/phaseTests/FavouritesStoreTests.swift@@ -13,7 +13,7 @@ import PhaseShared /// Tests for FavouritesStore using in-memory ModelContainer /// Tests are serialized to avoid SwiftData concurrency issues-@Suite("FavouritesStore Tests", .serialized)+@Suite("FavouritesStore Tests", .serialized, .isolatedAppGroupStore) struct FavouritesStoreTests { // MARK: - Test Helpersdiff --git a/phaseTests/FavouriteEntityQueryTests.swift b/phaseTests/FavouriteEntityQueryTests.swiftindex ea790f3..5e550b3 100644--- a/phaseTests/FavouriteEntityQueryTests.swift+++ b/phaseTests/FavouriteEntityQueryTests.swift@@ -12,7 +12,7 @@ import Testing /// Tests for FavouriteEntityQuery /// Verifies entity resolution and suggestions read from App Group storage (NFR-1.4)-@Suite("FavouriteEntityQuery Tests", .serialized)+@Suite("FavouriteEntityQuery Tests", .serialized, .isolatedAppGroupStore) struct FavouriteEntityQueryTests { // MARK: - Setup / Teardowndiff --git a/phaseTests/WidgetTests.swift b/phaseTests/WidgetTests.swiftindex 74a621b..e08c53e 100644--- a/phaseTests/WidgetTests.swift+++ b/phaseTests/WidgetTests.swift@@ -13,7 +13,7 @@ import Testing // Note: Widget extension code is compiled separately and cannot be directly tested from the main app's test target. // These tests verify the shared components used by both the app and the widget. -@Suite(.serialized)+@Suite(.serialized, .isolatedAppGroupStore) struct WidgetTests { // MARK: - SharedTimerState Testsdiff --git a/phaseTests/AppIntentsTests.swift b/phaseTests/AppIntentsTests.swiftindex ef64fb3..7cc88c0 100644--- a/phaseTests/AppIntentsTests.swift+++ b/phaseTests/AppIntentsTests.swift@@ -14,7 +14,7 @@ import Testing // Test suites are intentionally long; these length limits target production code. // swiftlint:disable type_body_length file_length -@Suite("App Intents Tests", .serialized)+@Suite("App Intents Tests", .serialized, .isolatedAppGroupStore) struct AppIntentsTests { // MARK: - Setup / Teardowndiff --git a/phaseTests/DashboardViewModelTests.swift b/phaseTests/DashboardViewModelTests.swiftindex 2bed16e..38af5ea 100644--- a/phaseTests/DashboardViewModelTests.swift+++ b/phaseTests/DashboardViewModelTests.swift@@ -11,9 +11,9 @@ import Testing @testable import phase // Test suites are intentionally long; these length limits target production code.-// swiftlint:disable type_body_length file_length function_body_length+// swiftlint:disable type_body_length file_length -@Suite("DashboardViewModel Tests", .serialized)+@Suite("DashboardViewModel Tests", .serialized, .isolatedAppGroupStore) struct DashboardViewModelTests { // MARK: - Mock API Key Provider@@ -485,6 +485,7 @@ struct DashboardViewModelTests { @MainActor @Test("loadDashboard filters out parent projects")+ // swiftlint:disable:next function_body_length func loadDashboardFiltersParentProjects() async { // Report JSON with both parent and leaf projects let reportWithParent = """@@ -1255,4 +1256,4 @@ struct DashboardViewModelTests { } } -// swiftlint:enable type_body_length file_length function_body_length+// swiftlint:enable type_body_length file_lengthdiff --git a/Makefile b/Makefileindex 769d6bc..ed0265b 100644--- a/Makefile+++ b/Makefile@@ -33,6 +33,7 @@ help: @echo " build-ios - Build for iOS Simulator" @echo " build-macos - Build for macOS" @echo " build - Build for both platforms"+ @echo " build-macos-clean - Clean macOS build (only if cross-platform module errors)" @echo " test-quick - Run unit tests on macOS (fast)" @echo " test - Run full test suite on iOS Simulator" @echo " test-ui - Run UI tests only"@@ -60,15 +61,17 @@ help: @echo "Override with: make install DEVICE_MODEL='iPhone 16'" # Linting-# Use --no-cache so lint runs don't depend on a writable user/global SwiftLint-# cache directory (which isn't guaranteed in sandboxed or CI environments).+# Cache into the repo-local, gitignored DerivedData so lint runs are incremental+# without depending on a writable user/global cache dir (not guaranteed in+# sandboxed or CI environments). `make clean` wipes it; it rebuilds on next run.+SWIFTLINT_CACHE = ./DerivedData/swiftlint .PHONY: lint lint:- swiftlint lint --strict --no-cache+ swiftlint lint --strict --cache-path $(SWIFTLINT_CACHE) .PHONY: lint-fix lint-fix:- swiftlint lint --fix --strict --no-cache+ swiftlint lint --fix --strict --cache-path $(SWIFTLINT_CACHE) # Building DERIVED_DATA = ./DerivedData@@ -85,7 +88,7 @@ build-ios: $(PIPE_PRETTY) .PHONY: build-macos-build-macos: clean+build-macos: xcodebuild build \ -project $(PROJECT) \ -scheme $(SCHEME) \@@ -95,6 +98,13 @@ build-macos: clean -derivedDataPath $(DERIVED_DATA) \ $(PIPE_PRETTY) +# Clean macOS build. xcodebuild keys products by platform inside one DerivedData+# (Debug-iphonesimulator vs Debug), so iOS and macOS coexist and build-macos does+# NOT need a clean normally. Use this only if a stale shared module cache produces+# "module was built for X but linking against Y" errors across platforms.+.PHONY: build-macos-clean+build-macos-clean: clean build-macos+ .PHONY: build build: build-ios build-macos @@ -229,23 +239,34 @@ archive-macos: $(PIPE_PRETTY) @echo "Archive created at $(ARCHIVE_PATH_MACOS)" +EXPORT_OPTIONS = ExportOptions.plist++# Fail fast (before the expensive archive) if the export options are missing.+.PHONY: check-export-options+check-export-options:+ @test -f $(EXPORT_OPTIONS) || { \+ echo "Error: $(EXPORT_OPTIONS) not found in repo root."; \+ echo "Create it with your App Store Connect export settings before uploading."; \+ exit 1; \+ }+ # To re-upload an existing archive without rebuilding: # xcodebuild -exportArchive -archivePath ./build/phase-ios.xcarchive \ # -exportOptionsPlist ExportOptions.plist -exportPath ./build/export-ios -allowProvisioningUpdates .PHONY: upload-upload: archive+upload: check-export-options archive xcodebuild -exportArchive \ -archivePath $(ARCHIVE_PATH_IOS) \- -exportOptionsPlist ExportOptions.plist \+ -exportOptionsPlist $(EXPORT_OPTIONS) \ -exportPath $(EXPORT_PATH_IOS) \ -allowProvisioningUpdates @echo "Uploaded iOS to App Store Connect" .PHONY: upload-macos-upload-macos: archive-macos+upload-macos: check-export-options archive-macos xcodebuild -exportArchive \ -archivePath $(ARCHIVE_PATH_MACOS) \- -exportOptionsPlist ExportOptions.plist \+ -exportOptionsPlist $(EXPORT_OPTIONS) \ -exportPath $(EXPORT_PATH_MACOS) \ -allowProvisioningUpdates @echo "Uploaded macOS to App Store Connect"diff --git a/CLAUDE.md b/CLAUDE.mdindex d821020..63565fb 100644--- a/CLAUDE.md+++ b/CLAUDE.md@@ -19,15 +19,21 @@ Phase Space is an iOS/macOS companion app for Timing (automatic time-tracking to ## Build Commands -This is an Xcode project without a Makefile. Use these commands:+Use the Makefile for development tooling. Run `make help` for the full list. Common targets: ```bash-# Build-xcodebuild -project phase.xcodeproj -scheme phase -destination 'platform=iOS Simulator,name=iPhone 17 Pro' build+make lint # SwiftLint (strict); lint-fix to auto-correct+make build-ios # Build for iOS Simulator+make build-macos # Build for macOS+make build # Build both platforms+make test-quick # Unit tests on macOS (fast, no simulator boot)+make test # Full suite on iOS Simulator+make test-ui # UI tests only+``` -# Run tests-xcodebuild -project phase.xcodeproj -scheme phase -destination 'platform=iOS Simulator,name=iPhone 17 Pro' test+The targets wrap `xcodebuild`. For ad-hoc invocations (e.g. a single test method) call it directly: +```bash # Run a single test file xcodebuild -project phase.xcodeproj -scheme phase -destination 'platform=iOS Simulator,name=iPhone 17 Pro' test -only-testing:phaseTests/DashboardViewModelTests diff --git a/CHANGELOG.md b/CHANGELOG.mdindex abf4731..c3102db 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -31,6 +31,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- Flaky unit-test suite: isolated the App Group `UserDefaults` per test. Suites exercising `SharedDataStore` raced on the single process-global `UserDefaults(suiteName:)` because Swift Testing runs suites in parallel (the existing `.serialized` only orders tests *within* a suite). Added a `TaskLocal` suite-name override on `SharedDataStore` (nil in production) and an `.isolatedAppGroupStore` test trait that gives each test its own throwaway suite; verified with 6 consecutive clean full-suite runs.+- Makefile review refinements+ - `make build` no longer discards the iOS build it just compiled: dropped the `clean` prerequisite from `build-macos` (products are platform-separated within one DerivedData) and added an explicit `build-macos-clean` for cross-platform module errors+ - `upload`/`upload-macos` fail fast with a clear message when `ExportOptions.plist` is missing, instead of after the expensive archive+ - `lint`/`lint-fix` use a repo-local incremental cache (`./DerivedData/swiftlint`) instead of `--no-cache`+ - Updated `CLAUDE.md` build commands to use the Makefile - Prevented an intermittent SwiftUI compiler type-check timeout in `RunningTimerCard` by keeping the `let _ = elapsedTimeTick` ViewBuilder discard (a bare `_ =` assignment is folded into the result-builder type-check chain) - Silent error swallowing when stopping timer before starting favourite - Added os.Logger to StartFavouriteTimerIntent and StartTimerForFavouriteIntentThe two TimingAPIClient* suites have zero SharedDataStore calls and did not get the isolation trait. If they ever flake under heavy parallel load, that is a separate axis (their own MockURLProtocol/RateLimitTracker), not this App Group race.
Flakiness was non-deterministic, so a single green run is not proof. Confidence comes from 9 consecutive clean full phaseTests runs after the fix, against an original that failed most runs with a shifting failure set.