From 85d3bf35cf196091eac72f5df1fed5adf3f36f9f Mon Sep 17 00:00:00 2001 From: Sujoy Das Date: Sun, 6 Sep 2026 02:13:49 +0300 Subject: [PATCH] fix: await the toast timer in tests instead of racing the clock --- Vinnota/Model/AppState.swift | 21 +++++-- VinnotaTests/PersistenceAndStateTests.swift | 62 +++++++++++---------- 2 files changed, 48 insertions(+), 35 deletions(-) diff --git a/Vinnota/Model/AppState.swift b/Vinnota/Model/AppState.swift index ec7d505..6ff8667 100644 --- a/Vinnota/Model/AppState.swift +++ b/Vinnota/Model/AppState.swift @@ -37,7 +37,19 @@ final class AppState { var tasting = TastingForm() var toast: String? - private var toastTask: Task? + + /// How long a toast stays up. Named so the design's value has one home and + /// a test can pin it without waiting it out. + /// `nonisolated` because a default-argument expression is evaluated in the + /// caller's context, which is not the main actor. Safe: an immutable + /// `Sendable` constant. Without it this is a hard error under Swift 6. + nonisolated static let toastLifetime: Duration = .seconds(2.6) + + /// The timer that blanks the current toast. Readable so a test can *await* + /// the expiry instead of sleeping and hoping: the timer's own completion is + /// the event under test, and awaiting it takes exactly as long as the + /// machine needs — no deadline to blow on a loaded CI runner. + private(set) var toastTask: Task? func go(_ screen: Screen) { withAnimation(.easeOut(duration: 0.2)) { @@ -56,10 +68,9 @@ final class AppState { /// Toasts clear themselves after 2.6s, as the design's `toast()` does. /// The lifetime is a parameter so tests can exercise expiry and timer - /// cancellation without waiting out the real 2.6 seconds — a test that - /// sleeps against the true duration has only a fraction of a second of - /// margin, and flakes on a loaded machine. Callers use the default. - func showToast(_ message: String, for duration: Duration = .seconds(2.6)) { + /// cancellation without waiting out the real 2.6 seconds. Callers use the + /// default. + func showToast(_ message: String, for duration: Duration = AppState.toastLifetime) { toastTask?.cancel() withAnimation(.easeOut(duration: 0.2)) { toast = message } toastTask = Task { diff --git a/VinnotaTests/PersistenceAndStateTests.swift b/VinnotaTests/PersistenceAndStateTests.swift index 7e79494..1bc24cc 100644 --- a/VinnotaTests/PersistenceAndStateTests.swift +++ b/VinnotaTests/PersistenceAndStateTests.swift @@ -1015,53 +1015,55 @@ struct ToastTests { #expect(app.toast == "second") } - /// Polls rather than sleeping a fixed span: the assertion is "it becomes - /// nil", and on a contended CI runner a fixed sleep sized to the real 2.6s - /// lifetime has too little margin. This failed in CI for exactly that - /// reason, on a run that took 312s against 4s locally. - private func waitForToastToClear(_ app: AppState, - within limit: Duration = .seconds(15)) async -> Bool { - let deadline = ContinuousClock.now.advanced(by: limit) - while ContinuousClock.now < deadline { - if app.toast == nil { return true } - try? await Task.sleep(for: .milliseconds(20)) - } - return app.toast == nil - } + /// These two tests used to sleep against the clock and were the only + /// flaky tests in the suite — first at 3.4s against a 2.6s toast, then, + /// after that margin was widened, still failing on a runner where the whole + /// suite took 272s against 2s locally. + /// + /// Sizing a timeout is the wrong instrument: any bound is a guess about a + /// machine you do not control. The timer's *own completion* is the event + /// under test, so both tests now await `toastTask` directly. That takes + /// exactly as long as the machine needs and cannot time out early — a + /// hundredfold-slower runner passes at the same rate as this one. /// The replacement cancels the first toast's timer. Without that, the /// earlier timer would fire mid-way through the second toast and blank it. /// - /// The two lifetimes are deliberately far apart — the first expires almost - /// at once, the second not for half a minute — so the check cannot flake on - /// a slow machine. A delayed wake makes the first timer *more* likely to - /// have fired, not less, so slowness cannot mask the bug. + /// Awaiting the *first* timer is what makes this deterministic: when it + /// returns, that timer has finished deciding whether to blank the toast, so + /// a surviving one has already done its damage. Drop the `cancel()` from + /// `showToast` and this fails every run rather than one in fifty. @Test("The replaced toast's timer does not blank its successor", .timeLimit(.minutes(1))) func replacementCancelsTheOldTimer() async throws { let app = AppState() - app.showToast("first", for: .milliseconds(50)) + app.showToast("first", for: .milliseconds(1)) + let firstTimer = try #require(app.toastTask, "showToast must arm a timer") + app.showToast("second", for: .seconds(30)) - try await Task.sleep(for: .seconds(1)) + await firstTimer.value + #expect(app.toast == "second", - "the first toast's timer was due long ago and must have been cancelled") + "the first toast's timer was cancelled and must not blank its successor") } @Test("A toast clears itself when its life is up", .timeLimit(.minutes(1))) func toastExpires() async throws { let app = AppState() - app.showToast("Added to the book", for: .milliseconds(50)) + app.showToast("Added to the book", for: .milliseconds(1)) #expect(app.toast != nil, "the toast is visible before its timer fires") - #expect(await waitForToastToClear(app), "the toast never cleared itself") + + let timer = try #require(app.toastTask, "showToast must arm a timer") + await timer.value + + #expect(app.toast == nil, "the timer ran to completion and must have cleared the toast") } - /// The shipped lifetime is still the design's 2.6s — the parameter above is - /// a test seam, not a behaviour change, and this pins the default. - @Test("The default lifetime is unchanged") - func defaultLifetimeIsTwoPointSix() async throws { - let app = AppState() - app.showToast("Added to the book") - try await Task.sleep(for: .seconds(1)) - #expect(app.toast != nil, "a 2.6s toast is still up after 1s") + /// The duration parameter is a test seam, not a behaviour change: the + /// shipped lifetime is still the design's 2.6s. Pinned by value rather than + /// by waiting 2.6 seconds to watch it happen. + @Test("The shipped lifetime is still the design's 2.6s") + func defaultLifetimeIsUnchanged() { + #expect(AppState.toastLifetime == .seconds(2.6)) } @Test("reset() clears a live toast at once")