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/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/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/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 } 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"