From dafca3eb8c5c6ad7a95f51816caf82d1d0263465 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Tue, 15 Sep 2026 03:53:16 -0700 Subject: [PATCH 1/2] fix(tests): make the event buses register synchronously and replace fixed sleeps with post-condition polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intermittent, unattributable "1 failure" in the App target had two causes, only one of which was a test-hygiene problem. The real race is in production code. All four feature event buses registered their subscriber inside a `Task`, so `events()` returned a stream that was not yet in the subscriber table. A caller that subscribed and immediately wrote could miss its own event, and no number of `Task.yield()`s closed the window because the wait was on actor scheduling, not on cooperative yields. Two tests papered over it with a fixed 10ms sleep commented "give the subscription a beat to register", then awaited a 1s expectation — which is exactly the "one unnamed failure that does not reproduce" signature in the issue. In the running app the same window is an unread badge that occasionally does not move. The four identical private actors are now one `EventBusStorage` that registers under a `Mutex` inside the `AsyncStream` build closure, which `AsyncStream` invokes synchronously during init. `post` broadcasts synchronously too, and `subscriberCount` lets a test assert a subscription is live rather than wait for one. The second cause is fixed wall-clock waits for asynchronous post-conditions — thirteen flat 100ms sleeps in ExportViewModelTests, and a 50ms bounded poll in SearchViewModelTests that fell through silently so a slow machine failed on the assertion below rather than on the wait that actually expired. Support/AsyncSettle.swift now carries one idiom: settle(until:) polls and fails at the caller's own line, settleQuiet(for:) is the deliberately short wait for absence claims, and settledValue(of:) waits for a counter to stop moving before taking a baseline — needed by the poll-cancellation test, which sampled the count the instant after stopPolling() and so failed a correctly-cancelled loop whenever a poll was still in flight. CurrentUserStoreTests and TagCompletionViewModelTests already had private versions of this; both now point at the shared one. Naming the failure came first, and paid on its first run. scripts/gate.sh runs the whole gate with a result bundle per App-target run and prints the failing test identifiers; CI does the same and uploads the bundle. The gate's Decision 0003 grep is anchored to column 0 — the unanchored form matches the prose in file headers that say a file does *not* import the kit, and reports a violation on a compliant tree. The four sleeps in the persistence outbox tests are deliberately untouched: they exist because outboxEntries() sorts by `enqueuedAt` alone, which is not a total order. That is a FIFO-ordering defect in a sync queue, not a test race, and it is filed as #84 rather than smuggled into a test-hygiene change. Refs #82 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gSWb3scYobtxLJioV1qF9 --- .github/workflows/ci.yml | 31 ++++ App/Composition/ComposerEventBus.swift | 47 ++---- App/Composition/DirectMessagesEventBus.swift | 46 ++---- App/Composition/EventBusStorage.swift | 75 ++++++++++ App/Composition/ListsEventBus.swift | 46 ++---- App/Composition/NotificationsEventBus.swift | 46 ++---- AppTests/CurrentUserStoreTests.swift | 13 +- AppTests/DMThreadViewModelTests.swift | 30 ++-- .../DirectMessagesListViewModelTests.swift | 11 +- AppTests/EventBusStorageTests.swift | 141 ++++++++++++++++++ AppTests/ExportViewModelTests.swift | 28 ++-- AppTests/SearchViewModelTests.swift | 10 +- AppTests/Support/AsyncSettle.swift | 85 +++++++++++ AppTests/TagCompletionViewModelTests.swift | 41 ++--- scripts/gate.sh | 114 ++++++++++++++ 15 files changed, 563 insertions(+), 201 deletions(-) create mode 100644 App/Composition/EventBusStorage.swift create mode 100644 AppTests/EventBusStorageTests.swift create mode 100644 AppTests/Support/AsyncSettle.swift create mode 100755 scripts/gate.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 20867f3..712feef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,6 +92,9 @@ jobs: CODE_SIGNING_REQUIRED=NO \ CODE_SIGN_IDENTITY="" + # `-resultBundlePath` so a failure names the test (GitHub #82). Two + # App-target runs have reported "1 failure" with no name and neither + # reproduced; a count is not a bug report. - name: Run app-target tests (xcodebuild) run: | xcodebuild test \ @@ -99,10 +102,38 @@ jobs: -scheme InterlinedList \ -destination "platform=macOS" \ -derivedDataPath build/DerivedData \ + -resultBundlePath build/test-results/app.xcresult \ CODE_SIGNING_ALLOWED=NO \ CODE_SIGNING_REQUIRED=NO \ CODE_SIGN_IDENTITY="" + - name: Name the failing tests + if: failure() + run: | + xcrun xcresulttool get test-results tests \ + --path build/test-results/app.xcresult --format json \ + | python3 -c ' + import json, sys + def walk(node): + for child in node.get("children") or []: + yield from walk(child) + if node.get("nodeType") == "Test Case" and node.get("result") == "Failed": + yield node.get("nodeIdentifier") or node.get("name") + doc = json.load(sys.stdin) + names = [n for root in doc.get("testNodes", []) for n in walk(root)] + print("::error::Failing tests: " + (", ".join(names) if names else "none listed")) + for n in names: + print("FAILED:", n) + ' + + - name: Upload the App-target result bundle + if: failure() + uses: actions/upload-artifact@v7 + with: + name: app-test-results-${{ github.sha }} + path: build/test-results/app.xcresult + retention-days: 14 + - name: Locate built .app id: locate run: | diff --git a/App/Composition/ComposerEventBus.swift b/App/Composition/ComposerEventBus.swift index 039dd17..7201fab 100644 --- a/App/Composition/ComposerEventBus.swift +++ b/App/Composition/ComposerEventBus.swift @@ -50,48 +50,27 @@ enum ComposerEvent: Sendable, Equatable { /// terminate the stream by cancelling the consuming task. final class ComposerEventBus: Sendable { - private let storage = Storage() + /// Subscriber registry. Shared with the other three feature buses; see + /// `EventBusStorage` for why registration is synchronous (GitHub #82). + private let storage = EventBusStorage() init() {} /// Returns an `AsyncStream` that yields every event posted after - /// subscription. The stream finishes when the consumer cancels. + /// subscription. The subscriber is registered before this returns, so an + /// immediately-following `post` is delivered. The stream finishes when the + /// consuming task is cancelled. func events() -> AsyncStream { - let id = UUID() - return AsyncStream { continuation in - Task { await self.storage.register(id: id, continuation: continuation) } - continuation.onTermination = { _ in - Task { await self.storage.unregister(id: id) } - } - } + storage.stream() } - /// Publish an event to every active subscriber. Late subscribers - /// do not receive past events. + /// Publish an event to every active subscriber. Late subscribers do not + /// receive past events. Delivery is synchronous with the call. func post(_ event: ComposerEvent) { - Task { await storage.broadcast(event) } + storage.broadcast(event) } - // MARK: - Storage - - /// Holds the live continuations keyed by registration UUID. An - /// actor because subscribers / publishers are not serialized to - /// any thread. - private actor Storage { - private var continuations: [UUID: AsyncStream.Continuation] = [:] - - func register(id: UUID, continuation: AsyncStream.Continuation) { - continuations[id] = continuation - } - - func unregister(id: UUID) { - continuations[id] = nil - } - - func broadcast(_ event: ComposerEvent) { - for continuation in continuations.values { - continuation.yield(event) - } - } - } + /// Live subscriber count, for tests that need to assert a subscription + /// exists rather than wait for one. + var subscriberCount: Int { storage.subscriberCount } } diff --git a/App/Composition/DirectMessagesEventBus.swift b/App/Composition/DirectMessagesEventBus.swift index 6e0e21f..7d81d2f 100644 --- a/App/Composition/DirectMessagesEventBus.swift +++ b/App/Composition/DirectMessagesEventBus.swift @@ -42,47 +42,27 @@ enum DirectMessagesEvent: Sendable, Equatable { /// a subscription stream; terminate by cancelling the consuming task. final class DirectMessagesEventBus: Sendable { - private let storage = Storage() + /// Subscriber registry. Shared with the other three feature buses; see + /// `EventBusStorage` for why registration is synchronous (GitHub #82). + private let storage = EventBusStorage() init() {} /// Returns an `AsyncStream` that yields every event posted after - /// subscription. The stream finishes when the consumer cancels. + /// subscription. The subscriber is registered before this returns, so an + /// immediately-following `post` is delivered. The stream finishes when the + /// consuming task is cancelled. func events() -> AsyncStream { - let id = UUID() - return AsyncStream { continuation in - Task { await self.storage.register(id: id, continuation: continuation) } - continuation.onTermination = { _ in - Task { await self.storage.unregister(id: id) } - } - } + storage.stream() } - /// Publish an event to every active subscriber. Late subscribers do - /// not receive past events. + /// Publish an event to every active subscriber. Late subscribers do not + /// receive past events. Delivery is synchronous with the call. func post(_ event: DirectMessagesEvent) { - Task { await storage.broadcast(event) } + storage.broadcast(event) } - // MARK: - Storage - - /// Holds the live continuations keyed by registration UUID. An actor - /// because publishers and subscribers aren't serialized. - private actor Storage { - private var continuations: [UUID: AsyncStream.Continuation] = [:] - - func register(id: UUID, continuation: AsyncStream.Continuation) { - continuations[id] = continuation - } - - func unregister(id: UUID) { - continuations[id] = nil - } - - func broadcast(_ event: DirectMessagesEvent) { - for continuation in continuations.values { - continuation.yield(event) - } - } - } + /// Live subscriber count, for tests that need to assert a subscription + /// exists rather than wait for one. + var subscriberCount: Int { storage.subscriberCount } } diff --git a/App/Composition/EventBusStorage.swift b/App/Composition/EventBusStorage.swift new file mode 100644 index 0000000..2e9a43e --- /dev/null +++ b/App/Composition/EventBusStorage.swift @@ -0,0 +1,75 @@ +// EventBusStorage +// +// The shared subscriber registry behind the four feature event buses +// (`ListsEventBus`, `ComposerEventBus`, `NotificationsEventBus`, +// `DirectMessagesEventBus`). Each of those was carrying its own private copy of +// the same actor; this is that copy, written once and made synchronous. +// +// Why synchronous registration matters (GitHub #82). The previous actor-backed +// version registered the continuation *inside a `Task`*: +// +// return AsyncStream { continuation in +// Task { await self.storage.register(id: id, continuation: continuation) } +// } +// +// so `events()` returned a stream that was not yet in the subscriber table. A +// caller that subscribed and then immediately performed a write could miss its +// own event, and no number of `Task.yield()`s closed the window — registration +// was waiting on actor scheduling, not on cooperative yields. The tests papered +// over it with a fixed 10 ms sleep ("give the subscription a beat to register"), +// which is exactly the pattern that passes on an idle machine and loses the race +// when the suite competes with a cold build. In the running app the same window +// showed up as an unread badge that occasionally did not move. +// +// Registering under a lock inside the `AsyncStream` build closure — which +// `AsyncStream` invokes synchronously during init — closes it: by the time +// `events()` returns, the subscriber is live. +// +// `Mutex` rather than an actor because every operation here is a short, +// non-suspending dictionary mutation and `yield` never blocks (`AsyncStream`'s +// default buffering policy is unbounded). An actor buys serialization this does +// not need and costs the synchrony this does. +// +// Per Decision 0003 this file lives in `App/Composition/` and imports no kit. + +import Foundation +import Synchronization + +final class EventBusStorage: Sendable { + + private let continuations = Mutex<[UUID: AsyncStream.Continuation]>([:]) + + init() {} + + /// A stream that is **already registered** by the time it is returned. + /// + /// The stream finishes when the consuming task is cancelled; termination + /// unregisters the continuation so a dropped subscriber does not leak. + func stream() -> AsyncStream { + let id = UUID() + return AsyncStream { continuation in + continuations.withLock { $0[id] = continuation } + continuation.onTermination = { [weak self] _ in + self?.continuations.withLock { $0[id] = nil } + } + } + } + + /// Delivers `event` to every subscriber registered at the moment of the + /// call. Late subscribers do not receive past events. + /// + /// The values are copied out under the lock and yielded outside it, so a + /// subscriber that reacts by subscribing or unsubscribing cannot deadlock. + func broadcast(_ event: Event) { + let live = continuations.withLock { Array($0.values) } + for continuation in live { + continuation.yield(event) + } + } + + /// Live subscriber count. Exists so a test can assert that subscription + /// happened without waiting on a clock. + var subscriberCount: Int { + continuations.withLock { $0.count } + } +} diff --git a/App/Composition/ListsEventBus.swift b/App/Composition/ListsEventBus.swift index 02af9bc..7c9e089 100644 --- a/App/Composition/ListsEventBus.swift +++ b/App/Composition/ListsEventBus.swift @@ -63,47 +63,27 @@ enum ListsEvent: Sendable, Equatable { /// subscription stream; terminate by cancelling the consuming task. final class ListsEventBus: Sendable { - private let storage = Storage() + /// Subscriber registry. Shared with the other three feature buses; see + /// `EventBusStorage` for why registration is synchronous (GitHub #82). + private let storage = EventBusStorage() init() {} /// Returns an `AsyncStream` that yields every event posted after - /// subscription. The stream finishes when the consumer cancels. + /// subscription. The subscriber is registered before this returns, so an + /// immediately-following `post` is delivered. The stream finishes when the + /// consuming task is cancelled. func events() -> AsyncStream { - let id = UUID() - return AsyncStream { continuation in - Task { await self.storage.register(id: id, continuation: continuation) } - continuation.onTermination = { _ in - Task { await self.storage.unregister(id: id) } - } - } + storage.stream() } - /// Publish an event to every active subscriber. Late subscribers - /// do not receive past events. + /// Publish an event to every active subscriber. Late subscribers do not + /// receive past events. Delivery is synchronous with the call. func post(_ event: ListsEvent) { - Task { await storage.broadcast(event) } + storage.broadcast(event) } - // MARK: - Storage - - /// Holds the live continuations keyed by registration UUID. An - /// actor because publishers and subscribers aren't serialized. - private actor Storage { - private var continuations: [UUID: AsyncStream.Continuation] = [:] - - func register(id: UUID, continuation: AsyncStream.Continuation) { - continuations[id] = continuation - } - - func unregister(id: UUID) { - continuations[id] = nil - } - - func broadcast(_ event: ListsEvent) { - for continuation in continuations.values { - continuation.yield(event) - } - } - } + /// Live subscriber count, for tests that need to assert a subscription + /// exists rather than wait for one. + var subscriberCount: Int { storage.subscriberCount } } diff --git a/App/Composition/NotificationsEventBus.swift b/App/Composition/NotificationsEventBus.swift index 14b89f6..0330e56 100644 --- a/App/Composition/NotificationsEventBus.swift +++ b/App/Composition/NotificationsEventBus.swift @@ -50,47 +50,27 @@ enum NotificationsEvent: Sendable, Equatable { /// cancelling the consuming task. final class NotificationsEventBus: Sendable { - private let storage = Storage() + /// Subscriber registry. Shared with the other three feature buses; see + /// `EventBusStorage` for why registration is synchronous (GitHub #82). + private let storage = EventBusStorage() init() {} /// Returns an `AsyncStream` that yields every event posted after - /// subscription. The stream finishes when the consumer cancels. + /// subscription. The subscriber is registered before this returns, so an + /// immediately-following `post` is delivered. The stream finishes when the + /// consuming task is cancelled. func events() -> AsyncStream { - let id = UUID() - return AsyncStream { continuation in - Task { await self.storage.register(id: id, continuation: continuation) } - continuation.onTermination = { _ in - Task { await self.storage.unregister(id: id) } - } - } + storage.stream() } - /// Publish an event to every active subscriber. Late subscribers - /// do not receive past events. + /// Publish an event to every active subscriber. Late subscribers do not + /// receive past events. Delivery is synchronous with the call. func post(_ event: NotificationsEvent) { - Task { await storage.broadcast(event) } + storage.broadcast(event) } - // MARK: - Storage - - /// Holds the live continuations keyed by registration UUID. An - /// actor because publishers and subscribers aren't serialized. - private actor Storage { - private var continuations: [UUID: AsyncStream.Continuation] = [:] - - func register(id: UUID, continuation: AsyncStream.Continuation) { - continuations[id] = continuation - } - - func unregister(id: UUID) { - continuations[id] = nil - } - - func broadcast(_ event: NotificationsEvent) { - for continuation in continuations.values { - continuation.yield(event) - } - } - } + /// Live subscriber count, for tests that need to assert a subscription + /// exists rather than wait for one. + var subscriberCount: Int { storage.subscriberCount } } diff --git a/AppTests/CurrentUserStoreTests.swift b/AppTests/CurrentUserStoreTests.swift index 95cbfb2..1321768 100644 --- a/AppTests/CurrentUserStoreTests.swift +++ b/AppTests/CurrentUserStoreTests.swift @@ -78,18 +78,13 @@ final class CurrentUserStoreTests: XCTestCase { // MARK: - Helpers - /// Polls `condition` until it returns `true` or 2 s elapses. Used - /// when the assertion depends on a value arriving asynchronously - /// from a stream we don't directly own a continuation on. + /// Thin shim onto the shared `settle(until:)` helper (`Support/AsyncSettle.swift`). + /// This file's private polling loop was the pattern the rest of the suite + /// should have been using all along; it now lives in one place (GitHub #82). private func waitFor( condition: @MainActor () -> Bool, timeout: Double = 2.0 ) async throws { - let deadline = Date().addingTimeInterval(timeout) - while Date() < deadline { - if condition() { return } - try await Task.sleep(nanoseconds: 10_000_000) // 10ms - } - XCTFail("Condition did not become true within \(timeout)s") + await settle(until: condition, timeout: .seconds(timeout)) } } diff --git a/AppTests/DMThreadViewModelTests.swift b/AppTests/DMThreadViewModelTests.swift index f7d2cea..aa4005d 100644 --- a/AppTests/DMThreadViewModelTests.swift +++ b/AppTests/DMThreadViewModelTests.swift @@ -192,16 +192,19 @@ final class DMThreadViewModelTests: XCTestCase { await service.enqueueMarkReadSuccess() await service.enqueueUnreadCount(success: 0) - // Capture the threadRead event. + // Capture the threadRead event. `events()` is called out here, not + // inside the Task: the bus registers the subscriber synchronously, so + // subscribing before the Task starts removes the race the old + // "give it a beat" sleep was covering (GitHub #82). let readEvent = expectation(description: "threadRead") + let stream = bus.events() let task = Task { - for await event in bus.events() { + for await event in stream { if case .threadRead(let username) = event, username == "ada" { readEvent.fulfill(); return } } } - try? await Task.sleep(nanoseconds: 10_000_000) await vm.markInboundRead() @@ -295,15 +298,24 @@ final class DMThreadViewModelTests: XCTestCase { await service.enqueueThreadUpdates(success: DMThread(messages: [], otherUser: ada, isMutual: true)) } + let pollCount: @MainActor () async -> Int = { + await service.recorded.filter { if case .threadUpdates = $0.kind { return true } else { return false } }.count + } + await vm.startPolling() - // Let a few poll cycles run. - try? await Task.sleep(nanoseconds: 40_000_000) + // Wait for the loop to have actually polled at least once, so the test + // proves cancellation rather than proving the loop never started. + await settle(until: { await pollCount() > 0 }, "The poll loop never ran") vm.stopPolling() - let countAfterStop = await service.recorded.filter { if case .threadUpdates = $0.kind { return true } else { return false } }.count - // Wait well past several more intervals; the count must not grow. - try? await Task.sleep(nanoseconds: 60_000_000) - let countLater = await service.recorded.filter { if case .threadUpdates = $0.kind { return true } else { return false } }.count + // Take the baseline only once the count has stopped moving: a poll that + // was already in flight when `stopPolling` landed will still record, and + // sampling before it does would fail a correctly-cancelled loop. + let countAfterStop = await settledValue(of: pollCount) + await settleQuiet(for: .milliseconds(60)) + let countLater = await pollCount() + + XCTAssertGreaterThan(countAfterStop, 0, "The poll loop must have run before cancellation is meaningful") XCTAssertEqual(countAfterStop, countLater, "No threadUpdates fire after stopPolling") } diff --git a/AppTests/DirectMessagesListViewModelTests.swift b/AppTests/DirectMessagesListViewModelTests.swift index fd109d5..1e4d897 100644 --- a/AppTests/DirectMessagesListViewModelTests.swift +++ b/AppTests/DirectMessagesListViewModelTests.swift @@ -252,10 +252,15 @@ final class DirectMessagesListViewModelTests: XCTestCase { let (vm, service, bus) = makeViewModel() await service.enqueueUnreadCount(success: 4) - // Subscribe before the refresh so the event is captured. + // Subscribe before the refresh so the event is captured. `events()` is + // called here rather than inside the Task: the bus registers the + // subscriber synchronously, so the subscription is live by the time this + // line returns and no "give it a beat" sleep is needed (GitHub #82). let received = expectation(description: "unread event") + let stream = bus.events() + XCTAssertEqual(bus.subscriberCount, 1, "The subscription must be live before the write") let task = Task { - for await event in bus.events() { + for await event in stream { if case .unreadCountChanged(let count) = event { XCTAssertEqual(count, 4) received.fulfill() @@ -263,8 +268,6 @@ final class DirectMessagesListViewModelTests: XCTestCase { } } } - // Give the subscription a beat to register. - try? await Task.sleep(nanoseconds: 10_000_000) await vm.refreshUnreadCount() diff --git a/AppTests/EventBusStorageTests.swift b/AppTests/EventBusStorageTests.swift new file mode 100644 index 0000000..d33984a --- /dev/null +++ b/AppTests/EventBusStorageTests.swift @@ -0,0 +1,141 @@ +// EventBusStorageTests +// +// BDD quartet for the shared subscriber registry behind the four feature event +// buses (GitHub #82). +// +// The behaviour under test is the one the old actor-backed version did not have: +// a subscriber is registered *before* `events()` returns, so a caller that +// subscribes and immediately writes receives its own event. Every test here is +// written so that it would fail — not merely flake — against the previous +// implementation, which is the only way a regression test for a race is worth +// anything. + +import XCTest +@testable import InterlinedList + +final class EventBusStorageTests: XCTestCase { + + // MARK: - Happy path + + func test_givenSubscriber_whenEventPostedImmediately_thenItIsDelivered() async { + // Given — a bus with one subscriber, created and *not* waited on. + let bus = DirectMessagesEventBus() + let stream = bus.events() + + // When — the write happens on the very next line, with no sleep, no + // yield, and no opportunity for a background registration to catch up. + bus.post(.unreadCountChanged(7)) + + // Then — the event is there. Under the old async registration this is + // the line that lost the race. + var iterator = stream.makeAsyncIterator() + let event = await iterator.next() + XCTAssertEqual(event, .unreadCountChanged(7)) + } + + func test_givenSubscription_whenCreated_thenRegistrationIsSynchronous() { + // The invariant stated directly: no awaiting anywhere in this test. + let bus = DirectMessagesEventBus() + XCTAssertEqual(bus.subscriberCount, 0) + let stream = bus.events() + XCTAssertEqual(bus.subscriberCount, 1, "events() must register before it returns") + // The stream has to be held: dropping it deinitialises the continuation, + // which fires `onTermination` and unregisters. That is the correct + // behaviour — asserted on its own below — but it makes `_ = bus.events()` + // a misleading way to write this test. + withExtendedLifetime(stream) {} + } + + func test_givenDiscardedStream_whenItDeinitialises_thenTheSubscriberIsDropped() { + // The mirror image: a stream nobody keeps must not leave a dead + // continuation in the table. This is the leak the registry would + // otherwise accumulate one entry at a time per transient view. + let bus = DirectMessagesEventBus() + do { + let stream = bus.events() + XCTAssertEqual(bus.subscriberCount, 1) + withExtendedLifetime(stream) {} + } + XCTAssertEqual(bus.subscriberCount, 0, "a dropped stream unregisters itself") + } + + // MARK: - Invalid / no-subscriber input + + func test_givenNoSubscribers_whenPosting_thenNothingHappensAndNoCrash() { + // Given a bus nobody is listening to — the ordinary state at launch. + let bus = NotificationsEventBus() + + // When / Then — posting into the void is a no-op, not a trap. + bus.post(.markedAllRead) + XCTAssertEqual(bus.subscriberCount, 0) + } + + // MARK: - "Upstream failure" analogue: a subscriber that goes away + + func test_givenCancelledSubscriber_whenPosting_thenItIsUnregistered() async { + // Given — a subscriber that is consuming, then cancelled. + let bus = ListsEventBus() + let stream = bus.events() + XCTAssertEqual(bus.subscriberCount, 1) + + let task = Task { for await _ in stream { } } + task.cancel() + + // Then — the registry drops it rather than leaking a dead continuation. + // Termination is delivered by the runtime, so this one genuinely has to + // wait for a post-condition; it polls rather than sleeping. + await settleOffMainActor(until: { bus.subscriberCount == 0 }) + XCTAssertEqual(bus.subscriberCount, 0) + } + + // MARK: - Boundary: several subscribers, and late ones + + func test_givenSeveralSubscribers_whenPosting_thenEachReceivesTheEventOnce() async { + // Given — three independent streams on one bus. + let bus = ComposerEventBus() + let streams = (0..<3).map { _ in bus.events() } + XCTAssertEqual(bus.subscriberCount, 3) + + // When + bus.post(.messageDeleted(id: "m1")) + + // Then — every subscriber sees it, and sees it once. + for stream in streams { + var iterator = stream.makeAsyncIterator() + let event = await iterator.next() + XCTAssertEqual(event, .messageDeleted(id: "m1")) + } + } + + func test_givenLateSubscriber_whenEventWasAlreadyPosted_thenItReceivesNothing() async { + // Boundary in the other direction: the bus is explicitly not a replay + // log, and a subscriber created after the fact must not see history. + let bus = ComposerEventBus() + bus.post(.messageDeleted(id: "m1")) + + let stream = bus.events() + bus.post(.messageDeleted(id: "m1")) + + var iterator = stream.makeAsyncIterator() + let first = await iterator.next() + XCTAssertEqual(first, .messageDeleted(id: "m1"), "only the post made after subscribing") + XCTAssertEqual(bus.subscriberCount, 1) + } + + // MARK: - Helpers + + /// `settle(until:)` is `@MainActor`; this suite is not, because the bus + /// deliberately has no actor affinity. Same contract, no isolation. + private func settleOffMainActor( + until condition: @Sendable () -> Bool, + timeout: Duration = .seconds(5) + ) async { + let deadline = ContinuousClock.now.advanced(by: timeout) + while ContinuousClock.now < deadline { + if condition() { return } + await Task.yield() + try? await Task.sleep(for: .milliseconds(1)) + } + XCTAssertTrue(condition(), "Condition never became true within \(timeout)") + } +} diff --git a/AppTests/ExportViewModelTests.swift b/AppTests/ExportViewModelTests.swift index 4d4bacf..a51cdcc 100644 --- a/AppTests/ExportViewModelTests.swift +++ b/AppTests/ExportViewModelTests.swift @@ -43,8 +43,8 @@ final class ExportViewModelTests: XCTestCase { // When vm.export(.messages) - // Allow async Task to complete. - try await Task.sleep(nanoseconds: 100_000_000) + // Wait for the fire-and-forget export Task to run its `defer`. + await settle(until: { !vm.isExporting }) // Then — pendingExport populated, no error, service was called. XCTAssertNotNil(vm.pendingExport) @@ -58,7 +58,7 @@ final class ExportViewModelTests: XCTestCase { service.enqueueLists() vm.export(.lists) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) XCTAssertNotNil(vm.pendingExport) XCTAssertNil(vm.errorMessage) @@ -70,7 +70,7 @@ final class ExportViewModelTests: XCTestCase { service.enqueueListDataRows() vm.export(.listDataRows) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) XCTAssertNotNil(vm.pendingExport) XCTAssertNil(vm.errorMessage) @@ -82,7 +82,7 @@ final class ExportViewModelTests: XCTestCase { service.enqueueFollows() vm.export(.follows) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) XCTAssertNotNil(vm.pendingExport) XCTAssertNil(vm.errorMessage) @@ -100,7 +100,7 @@ final class ExportViewModelTests: XCTestCase { vm.export(.messages) // Second call while isExporting should be true. vm.export(.lists) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — only messages was dispatched; lists was dropped. XCTAssertEqual(service.recorded, [.messages]) @@ -115,7 +115,7 @@ final class ExportViewModelTests: XCTestCase { // When vm.export(.messages) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — error surfaced; no pending export; isExporting cleared. XCTAssertNil(vm.pendingExport) @@ -132,7 +132,7 @@ final class ExportViewModelTests: XCTestCase { // When vm.export(.messages) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — empty data is a valid domain result; forward it for the save panel. let export = try XCTUnwrap(vm.pendingExport) @@ -147,13 +147,13 @@ final class ExportViewModelTests: XCTestCase { let (vm, service) = makeSUT() service.enqueueMessages(failure: TestError.upstream("timeout")) vm.export(.messages) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) XCTAssertNotNil(vm.errorMessage) // When — user retries. service.enqueueMessages() vm.export(.messages) - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — stale error is cleared before the retry. XCTAssertNil(vm.errorMessage) @@ -170,7 +170,7 @@ final class ExportViewModelTests: XCTestCase { // When vm.exportListsAsMarkdown() - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — a Markdown document with the list heading and a schema-ordered table row. let export = try XCTUnwrap(vm.pendingMarkdownExport) @@ -188,7 +188,7 @@ final class ExportViewModelTests: XCTestCase { await lists.enqueueMyLists(success: .empty) vm.exportListsAsMarkdown() - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) // Then — an empty document is still a valid export; no rows call was made. let export = try XCTUnwrap(vm.pendingMarkdownExport) @@ -203,7 +203,7 @@ final class ExportViewModelTests: XCTestCase { await lists.enqueueMyLists(failure: TestError.upstream("session expired")) vm.exportListsAsMarkdown() - try await Task.sleep(nanoseconds: 100_000_000) + await settle(until: { !vm.isExporting }) XCTAssertNil(vm.pendingMarkdownExport) XCTAssertNotNil(vm.errorMessage) @@ -220,7 +220,7 @@ final class ExportViewModelTests: XCTestCase { await lists.enqueueRows(success: .init(rows: [row("r3", ["K": .string("three")])], hasMore: false, nextOffset: nil)) vm.exportListsAsMarkdown() - try await Task.sleep(nanoseconds: 150_000_000) + await settle(until: { !vm.isExporting }) // Then — both lists and all rows appear; two myLists calls were made. let export = try XCTUnwrap(vm.pendingMarkdownExport) diff --git a/AppTests/SearchViewModelTests.swift b/AppTests/SearchViewModelTests.swift index d378fb1..ebe1713 100644 --- a/AppTests/SearchViewModelTests.swift +++ b/AppTests/SearchViewModelTests.swift @@ -156,11 +156,11 @@ final class SearchViewModelTests: XCTestCase { await service.enqueueAll(messages: [message("m1")], lists: [], documents: []) vm.query = "swift" - // Let the debounced task (zero window) run to completion. - await Task.yield() - for _ in 0..<10 where !vm.hasSearched { - try? await Task.sleep(nanoseconds: 5_000_000) - } + // Let the debounced task (zero window) run to completion. The old + // version capped the wait at ten 5 ms ticks and then fell through + // silently, so a loaded machine failed on the assertion below instead of + // on the wait that actually timed out (GitHub #82). + await settle(until: { vm.hasSearched }, "The debounced search never resolved") XCTAssertEqual(vm.results.messages.map(\.id), ["m1"]) XCTAssertTrue(vm.hasSearched) diff --git a/AppTests/Support/AsyncSettle.swift b/AppTests/Support/AsyncSettle.swift new file mode 100644 index 0000000..9313337 --- /dev/null +++ b/AppTests/Support/AsyncSettle.swift @@ -0,0 +1,85 @@ +// AsyncSettle +// +// Shared waiting helpers for App-target tests (GitHub #82). +// +// The rule these encode: **never wait a fixed amount of wall-clock time for an +// asynchronous post-condition.** A `try await Task.sleep(100ms)` after a +// fire-and-forget `Task` passes on an idle machine and fails when the first run +// of a session competes with indexing or a cold build — which is exactly the +// profile of the two unattributable App-target failures that prompted this file. +// A `Task.yield()` is no better: it is not a barrier for work that suspends on +// the clock or on actor scheduling, so a fixed number of yields is just a sleep +// with extra steps. +// +// Poll for the post-condition instead, and fail loudly with the caller's own +// file and line when it never arrives — so the failure names the assertion that +// timed out rather than the assertion that ran too early. + +import XCTest + +/// Polls `condition` until it holds, then returns. Fails the test at the +/// caller's line if the deadline passes first. +/// +/// The tick is deliberately small (1 ms) and the ceiling generous (5 s): a +/// satisfied condition exits on the first pass, so a long ceiling costs nothing +/// on a healthy run and buys tolerance on a loaded machine. +@MainActor +func settle( + until condition: @MainActor () async -> Bool, + timeout: Duration = .seconds(5), + _ message: @autoclosure () -> String = "Condition never became true", + file: StaticString = #filePath, + line: UInt = #line +) async { + let deadline = ContinuousClock.now.advanced(by: timeout) + while ContinuousClock.now < deadline { + if await condition() { return } + await Task.yield() + try? await Task.sleep(for: .milliseconds(1)) + } + // One last read: the loop can exit on the deadline in the same instant the + // condition becomes true, and failing there would be its own flake. + if await condition() { return } + XCTFail("\(message()) within \(timeout)", file: file, line: line) +} + +/// Waits a bounded, deliberately short window **without** a post-condition, for +/// assertions that something did *not* happen. +/// +/// A negative assertion cannot exit early — there is no event to wait for — so +/// this one genuinely burns its whole budget and is kept small on purpose. Reach +/// for it only when the claim is an absence; every positive assertion belongs in +/// `settle(until:)`. +@MainActor +func settleQuiet(for duration: Duration = .milliseconds(50)) async { + let deadline = ContinuousClock.now.advanced(by: duration) + while ContinuousClock.now < deadline { + await Task.yield() + try? await Task.sleep(for: .milliseconds(1)) + } +} + +/// Polls `sample` until two consecutive reads agree, then returns that value. +/// +/// For "a loop was stopped" assertions: rather than sleeping past N intervals +/// and hoping no straggler lands between the two reads, wait for the value to +/// stop moving and only then take the baseline. `settleQuiet` afterwards proves +/// it stays put. +@MainActor +func settledValue( + of sample: @MainActor () async -> T, + timeout: Duration = .seconds(5), + file: StaticString = #filePath, + line: UInt = #line +) async -> T { + let deadline = ContinuousClock.now.advanced(by: timeout) + var previous = await sample() + while ContinuousClock.now < deadline { + try? await Task.sleep(for: .milliseconds(5)) + let current = await sample() + if current == previous { return current } + previous = current + } + XCTFail("Value never stopped changing within \(timeout)", file: file, line: line) + return previous +} diff --git a/AppTests/TagCompletionViewModelTests.swift b/AppTests/TagCompletionViewModelTests.swift index 256400c..aa66d73 100644 --- a/AppTests/TagCompletionViewModelTests.swift +++ b/AppTests/TagCompletionViewModelTests.swift @@ -15,26 +15,13 @@ final class TagCompletionViewModelTests: XCTestCase { TagCompletionViewModel(service: stub, debounce: .zero) } - /// Lets the debounced lookup task run to completion. - /// - /// The lookup suspends on `Task.sleep(for: debounce)`, which goes through - /// the clock even at `.zero` — so a fixed number of `Task.yield()`s is not - /// a barrier, and the original ten-yield version lost the race - /// intermittently when the whole suite ran under load. Poll on a real - /// (short) sleep and stop as soon as the model settles. - private func settle( - until condition: (@MainActor () -> Bool)? = nil - ) async { - // A condition can exit early, so it can afford a generous ceiling. With - // no condition the assertion is that something did NOT happen, so the - // loop always runs to the end — keep that window short. - let iterations = condition == nil ? 25 : 400 - for _ in 0../dev/null \ + | python3 -c ' +import json, sys +def walk(node): + for child in node.get("children", []) or []: + yield from walk(child) + if node.get("nodeType") == "Test Case" and node.get("result") == "Failed": + yield node.get("nodeIdentifier") or node.get("name") +try: + doc = json.load(sys.stdin) +except Exception: + sys.exit(1) +names = [n for root in doc.get("testNodes", []) for n in walk(root)] +if names: + for n in names: + print(" FAILED:", n) +else: + print(" (result bundle parsed, but no failed test cases were listed)") +'; then + echo " (could not parse the result bundle; open it with: xed $bundle)" + fi +} + +run_app_tests() { + local i="$1" + local bundle="$RESULTS_DIR/app-$(printf '%03d' "$i").xcresult" + rm -rf "$bundle" + echo "=== App target tests (run $i/$ITERATIONS)" + if xcodebuild test \ + -project "$PROJECT" -scheme "$SCHEME" -destination "$DEST" \ + -resultBundlePath "$bundle" \ + "${NOSIGN[@]}" 2>&1 | tail -n 40; then + echo "--- run $i: PASS" + else + echo "--- run $i: FAIL" + name_failures "$bundle" + FAILED=1 + fi +} + +if [ "$MODE" = "all" ]; then + echo "=== Build" + xcodebuild build -project "$PROJECT" -scheme "$SCHEME" -destination "$DEST" "${NOSIGN[@]}" \ + 2>&1 | tail -n 5 || FAILED=1 + + for pkg in InterlinedKit InterlinedDomain InterlinedPersistence; do + echo "=== swift test: $pkg" + swift test --package-path "Packages/$pkg" 2>&1 | tail -n 3 || FAILED=1 + done + + # Anchored at column 0: the unanchored form in the checklist matches the + # *prose* in file-header comments that say a file does NOT import the kit, + # which reports a violation on a compliant tree. + echo "=== Decision 0003: no Kit imports in features" + if grep -rn "^import InterlinedKit" App/Features App/Navigation App/MenuCommands 2>/dev/null; then + echo "VIOLATION: Kit imported in a feature layer" + FAILED=1 + else + echo "zero hits — OK" + fi +fi + +for ((i = 1; i <= ITERATIONS; i++)); do + run_app_tests "$i" +done + +echo +if [ "$FAILED" -eq 0 ]; then + echo "GATE: PASS" +else + echo "GATE: FAIL — see the named tests above; bundles in $RESULTS_DIR" +fi +exit "$FAILED" From 2ddfb00b5645ea8c051c325190f987887b48ae03 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Tue, 15 Sep 2026 04:21:11 -0700 Subject: [PATCH 2/2] fix(tests): replace the bounded yield-count waits the flake hunt caught MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the App target with `-test-iterations 20 -run-tests-until-failure` reproduced the failure on iteration 3 and — because the run now captures a result bundle — named it: DocumentEditorViewModelTests/test_givenBodyEdit_whenDebounceElapses_thenCallsUpdate XCTAssertFalse failed `waitForSaveCompletion` yielded eight times and then fell through silently, so a save that had not finished failed the caller's assertion rather than reporting that the wait expired. The debounced save suspends on `Task.sleep(for: debounce)`, which goes through the clock even at `.zero`, and a fixed yield count is not a barrier for that. This is the same defect as the fixed sleeps, wearing different clothes — which is why the issue's nine-file table missed it: it counted `Task.sleep` and this one spends `Task.yield()`. Swept for the whole class; the other instance was `FollowRequestRowViewModelTests`, whose five yields were waiting for the bus registration that is now synchronous, so the wait is replaced by an assertion that the subscription is live. 20 consecutive App-target iterations now pass, where the same command previously stopped at 3. Refs #82 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gSWb3scYobtxLJioV1qF9 --- AppTests/DocumentEditorViewModelTests.swift | 16 ++++++++++++---- AppTests/FollowRequestRowViewModelTests.swift | 16 ++++++++-------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/AppTests/DocumentEditorViewModelTests.swift b/AppTests/DocumentEditorViewModelTests.swift index 2729a60..4d7dbdc 100644 --- a/AppTests/DocumentEditorViewModelTests.swift +++ b/AppTests/DocumentEditorViewModelTests.swift @@ -210,10 +210,18 @@ final class DocumentEditorViewModelTests: XCTestCase { /// Yields the runloop several times so the debounce task + the /// service round-trip both have a chance to complete. With /// `debounce: .zero`, two-three turns is enough. + /// Waits for the debounced save to complete. + /// + /// This was eight `Task.yield()`s that then **fell through silently**, so a + /// save that had not finished failed the caller's `XCTAssertFalse` rather + /// than reporting that the wait expired. It is the flake that a 20-iteration + /// run of the App target reproduced (GitHub #82): the save suspends on + /// `Task.sleep(for: debounce)`, which goes through the clock even at + /// `.zero`, and a fixed yield count is not a barrier for that. private func waitForSaveCompletion(viewModel: DocumentEditorViewModel) async { - for _ in 0..<8 { - await Task.yield() - if !viewModel.hasUnsavedChanges { return } - } + await settle( + until: { !viewModel.hasUnsavedChanges }, + "The debounced save never completed" + ) } } diff --git a/AppTests/FollowRequestRowViewModelTests.swift b/AppTests/FollowRequestRowViewModelTests.swift index a731dd8..10e8794 100644 --- a/AppTests/FollowRequestRowViewModelTests.swift +++ b/AppTests/FollowRequestRowViewModelTests.swift @@ -148,15 +148,15 @@ final class FollowRequestRowViewModelTests: XCTestCase { break } } - // Give the actor-storage register call a turn to land. - for _ in 0..<5 { await Task.yield() } + // No turn is needed for registration any more: the bus registers the + // subscriber synchronously inside `events()`, so the subscription above + // is already live (GitHub #82). Assert that rather than yielding at it. + XCTAssertEqual(bus.subscriberCount, 1) await work() - // Give the broadcast a few turns to make it through the bus - // actor and into the mailbox. - for _ in 0..<20 { - if await mailbox.value != nil { break } - await Task.yield() - } + // The broadcast itself is synchronous, but the consumer Task still has + // to be scheduled to move it into the mailbox — so poll for the + // post-condition instead of spending a fixed number of turns on it. + await settle(until: { await mailbox.value != nil }, "No event reached the mailbox") consumer.cancel() return await mailbox.value }