Commit `cc8a97bf86`

cc8a97bf865f1ff2dabdeb1008fe9d59090c48a7

parent: 4d6ea462de

Unsigned

cmc <hello@cleberg.net> · 2026-09-07 02:23 UTC

Make the timing-sensitive tests deterministic (!57)

Eleven tests changed something that fires an unstructured task, slept a
fixed interval, then asserted on whichever request happened to be last.
Two problems at once: a probabilistic wait, and asserting on "the last
one" rather than on the request the test is actually about.

Three of them were passing vacuously. Two assert that the reload after a
filter change carries no `--cursor` — but with the reload not yet landed
they were asserting against the initial request, which has no cursor
either, so they passed whatever the code did. The third asserted the
screen was not in a failed state, which is also true of a reload that
never started. Each was confirmed failing against injected faults before
being trusted.

They now poll for the request they mean, identified by its argv, and
assert on that one. A shared `until` helper replaces the sleeps: faster
in the common case, and deterministic rather than merely likely.

Separately, fifteen sites indexed `stub.seen` directly, so a regression
that produced fewer requests crashed the test process instead of failing.
They now go through `#require`, which reports and lets the rest of the
suite finish.

No assertion was weakened and no application code changed.

Layout: unified · split

gitbayTests/BlameEditTests.swift +2 −1

	@@ -64,7 +64,8 @@ struct BlameViewModelTests {
64	await model.loadMore()	64	await model.loadMore()
65	#expect(model.hunks.count == 2)	65	#expect(model.hunks.count == 2)
66	#expect(!model.hasMore)	66	#expect(!model.hasMore)
67	#expect(stub.seen[1].url.query()?.contains("argv=--from&argv=1001") == true)	67	let second = try #require(stub.seen.dropFirst().first)
		68	#expect(second.url.query()?.contains("argv=--from&argv=1001") == true)
68		69
69	// Nothing further to ask for.	70	// Nothing further to ask for.
70	await model.loadMore()	71	await model.loadMore()

gitbayTests/ExploreRefLogTests.swift +2 −1

	@@ -84,7 +84,8 @@ struct ExploreTests {
84	await list.loadMore()	84	await list.loadMore()
85	#expect(list.state.value?.count == 3)	85	#expect(list.state.value?.count == 3)
86	#expect(!list.hasMore)	86	#expect(!list.hasMore)
87	#expect(stub.seen[1].url.query()?.contains("argv=--cursor&argv=ZXhwbG9yZTpr") == true)	87	let second = try #require(stub.seen.dropFirst().first)
		88	#expect(second.url.query()?.contains("argv=--cursor&argv=ZXhwbG9yZTpr") == true)
88	}	89	}
89		90
90	@Test func ownerAndNameSplitForDisplay() {	91	@Test func ownerAndNameSplitForDisplay() {

gitbayTests/GitbayClientTests.swift +6 −3

         #expect(first[0].path == "krz/gitbay")
         let requests = stub.seen
         #expect(requests.count == 2)
-        #expect(requests[0].headers["If-None-Match"] == nil)
+        let firstRequest = try #require(requests.first)
-        #expect(requests[1].headers["If-None-Match"] == etag)
+        #expect(firstRequest.headers["If-None-Match"] == nil)
+        let secondRequest = try #require(requests.dropFirst().first)
+        #expect(secondRequest.headers["If-None-Match"] == etag)
     }
     @Test func changedBodyReplacesTheCachedValidator() async throws {
         #expect(refreshed.isEmpty)
         #expect(revalidated.isEmpty)
-        #expect(stub.seen[2].headers["If-None-Match"] == "\"bb\"")
+        let third = try #require(stub.seen.dropFirst(2).first)
+        #expect(third.headers["If-None-Match"] == "\"bb\"")
     }
     @Test func absentDataDecodesAsEmptyList() async throws {

gitbayTests/ListFilterTests.swift +39 −8

         stub.enqueue(.init(status: 200, json: emptyPage, match: "argv=issue"))
         model.filter.label = "bug"
-        try await Task.sleep(for: .milliseconds(150))
+        await until { stub.seen.filter { $0.url.absoluteString.contains("argv=issue") }.count == 2 }
         let reads = stub.seen.filter { $0.url.absoluteString.contains("argv=issue") }
         let last = try #require(reads.last)
         stub.enqueue(.init(status: 200, json: emptyPage, match: "argv=issue"))
         model.filter.label = "bug"
-        try await Task.sleep(for: .milliseconds(150))
+        // Poll for the read that actually carries the new filter, not
+        // whatever happens to be last: a reload that never landed would
+        // otherwise leave `last` pointing at the initial request, which
+        // also carries no cursor and would pass just as wrongly.
+        await until {
+            stub.seen.contains { $0.url.absoluteString.contains("argv=issue") && argvFrom($0.url).contains("bug") }
+        }
-        let reads = stub.seen.filter { $0.url.absoluteString.contains("argv=issue") }
+        let reload = try #require(stub.seen.first {
-        let last = try #require(reads.last)
+            $0.url.absoluteString.contains("argv=issue") && argvFrom($0.url).contains("bug")
-        #expect(!argvFrom(last.url).contains("--cursor"))
+        })
+        #expect(!argvFrom(reload.url).contains("--cursor"))
     }
     /// Two rapid picker changes cancel the first reload's task. The
         stub.enqueue(.init(status: 200, json: emptyPage, match: "argv=issue"))
         model.filter.label = "bug"
         model.filter.label = "feature"
-        try await Task.sleep(for: .milliseconds(200))
+        // Wait for the winning reload's read to land rather than a fixed
+        // sleep: checking `.failed` against a still-untouched state (the
+        // spawned task hasn't run yet) would pass no matter what the real
+        // behaviour is. Whether the cancelled first task's own read also
+        // reaches the stub before it notices cancellation is a race in
+        // its own right, so this only requires the winning one.
+        await until {
+            stub.seen.filter { $0.url.absoluteString.contains("argv=issue") }.count >= 2
+        }
+        // The cancelled task's own completion can still land after the
+        // winning reload's; a poll alone stops the instant the count is
+        // satisfied and would miss a bogus overwrite arriving right
+        // after. A short settle catches it.
+        try await Task.sleep(for: .milliseconds(50))
+        // The state the poll waited for is "a reload's read landed", not
+        // "the screen settled" — a filter change that reloaded nothing at
+        // all would leave the pre-existing, unchanged, non-failed state
+        // sitting there and pass just as wrongly as a `.loading` screen
+        // checked too early would.
+        let reads = stub.seen.filter { $0.url.absoluteString.contains("argv=issue") }.count
+        guard reads >= 2 else {
+            Testing.Issue.record("rapid filter changes never reloaded")
+            return
+        }
         if case .failed(let message) = model.state {
             Testing.Issue.record("rapid filter changes left the screen failed: \(message)")
         stub.enqueue(.init(status: 200, json: emptyPage))
         model.filter.search = "retarget"
-        try await Task.sleep(for: .milliseconds(500))
+        await until { stub.seen.count == 2 }
         let last = try #require(stub.seen.last)
         let argv = argvFrom(last.url)
         stub.enqueue(.init(status: 200, json: emptyPage))
         model.filter.state = .sourceGone
-        try await Task.sleep(for: .milliseconds(150))
+        await until { stub.seen.count == 2 }
         #expect(argvFrom(try #require(stub.seen.last).url).contains("source_gone"))
     }

gitbayTests/MRViewModelTests.swift +17 −7

         await model.load()
         model.filter.state = .merged
-        // The reload happens in a spawned task; give it a beat.
+        // The reload happens in a spawned task; wait for it to settle
-        try await Task.sleep(for: .milliseconds(300))
+        // rather than for a fixed interval to pass. Checking `model.state`
+        // alone is not enough: right after the assignment, before the
+        // spawned task has even run, the state is still the *previous*
+        // load's non-loading result, which would satisfy a bare
+        // "not loading" check before the reload ever started.
+        await until {
+            guard stub.seen.count == 2 else { return false }
+            if case .loading = model.state { return false }
+            return true
+        }
         #expect(stub.seen.count == 2)
-        #expect(stub.seen[1].url.query()?.contains("argv=merged") == true)
+        let second = try #require(stub.seen.dropFirst().first)
+        #expect(second.url.query()?.contains("argv=merged") == true)
         guard case .empty = model.state else {
             Issue.record("expected .empty after filtering, got \(model.state)")
             return
         await model.review(.approve)
-        let write = stub.seen[3]
+        let write = try #require(stub.seen.dropFirst(3).first)
         #expect(write.method == "POST")
         #expect(write.url.path() == "/api/v1/cmd")
         let body = try #require(try JSONSerialization.jsonObject(with: write.body) as? [String: Any])
         await model.comment("long review text\nwith lines")
-        let write = stub.seen[3]
+        let write = try #require(stub.seen.dropFirst(3).first)
         let body = try #require(try JSONSerialization.jsonObject(with: write.body) as? [String: Any])
         #expect(body["argv"] as? [String] == ["mr", "comment", "krz/gitbay", "7", "--file", "-"])
         #expect(body["stdin"] as? String == "long review text\nwith lines")
         await model.setResolved(thread, true)
-        let write = stub.seen[3]
+        let write = try #require(stub.seen.dropFirst(3).first)
         let body = try #require(try JSONSerialization.jsonObject(with: write.body) as? [String: Any])
         #expect(body["argv"] as? [String] == ["mr", "resolve", "krz/gitbay", "7", "3"])
     }
         await model.reply(to: thread, "because 5xx is transient")
-        let write = stub.seen[3]
+        let write = try #require(stub.seen.dropFirst(3).first)
         let body = try #require(try JSONSerialization.jsonObject(with: write.body) as? [String: Any])
         #expect(body["argv"] as? [String] ==
             ["mr", "diff-comment", "krz/gitbay", "7", "--reply", "3", "--file", "-"])

gitbayTests/NotificationTests.swift +9 −5

	@@ -127,7 +127,7 @@ struct NotificationsViewModelTests {
127		127
128	stub.enqueue(.init(status: 200, json: page))	128	stub.enqueue(.init(status: 200, json: page))
129	model.showAll = true	129	model.showAll = true
130	try await Task.sleep(for: .milliseconds(150))	130	await until { stub.seen.count == 2 }
131		131
132	#expect(argvFrom(try #require(stub.seen.last).url).contains("--all"))	132	#expect(argvFrom(try #require(stub.seen.last).url).contains("--all"))
133	}	133	}
	@@ -143,10 +143,14 @@ struct NotificationsViewModelTests {
143		143
144	stub.enqueue(.init(status: 200, json: page))	144	stub.enqueue(.init(status: 200, json: page))
145	model.showAll = true	145	model.showAll = true
146	try await Task.sleep(for: .milliseconds(150))	146	// Poll for the read that actually carries the new filter, not
147		147	// whatever happens to be last: a reload that never landed would
148	let argv = argvFrom(try #require(stub.seen.last).url)	148	// otherwise leave `last` pointing at the initial request, which
149	#expect(argv.contains("--cursor") == false)	149	// also carries no cursor and would pass just as wrongly.
		150	await until { stub.seen.contains { argvFrom($0.url).contains("--all") } }
		151
		152	let reload = try #require(stub.seen.first { argvFrom($0.url).contains("--all") })
		153	#expect(!argvFrom(reload.url).contains("--cursor"))
150	}	154	}
151		155
152	@Test func markingOneReadSendsItsId() async throws {	156	@Test func markingOneReadSendsItsId() async throws {

gitbayTests/PagedListTests.swift +8 −4

         #expect(first.next == "abc")
         #expect(second.items == [Row(number: 3)])
         #expect(second.next == nil)
-        #expect(stub.seen[0].url.query() ==
+        let firstSeen = try #require(stub.seen.first)
+        #expect(firstSeen.url.query() ==
             "argv=issue&argv=list&argv=krz/gitbay&argv=--limit&argv=2")
-        #expect(stub.seen[1].url.query() ==
+        let secondSeen = try #require(stub.seen.dropFirst().first)
+        #expect(secondSeen.url.query() ==
             "argv=issue&argv=list&argv=krz/gitbay&argv=--limit&argv=2&argv=--cursor&argv=abc")
     }
 }
         // Further calls are no-ops, not requests.
         await list.loadMore()
         #expect(stub.seen.count == 3)
-        #expect(stub.seen[1].url.query()?.contains("argv=--cursor&argv=c1") == true)
+        let second = try #require(stub.seen.dropFirst().first)
-        #expect(stub.seen[2].url.query()?.contains("argv=--cursor&argv=c2") == true)
+        #expect(second.url.query()?.contains("argv=--cursor&argv=c1") == true)
+        let third = try #require(stub.seen.dropFirst(2).first)
+        #expect(third.url.query()?.contains("argv=--cursor&argv=c2") == true)
     }
     @Test func anEmptyFirstPageIsAnEmptyState() async throws {

gitbayTests/ProfileCommitTests.swift +2 −1

	@@ -59,7 +59,8 @@ struct ProfileAggregateTests {
59	#expect(profile.activityTotal == 15)	59	#expect(profile.activityTotal == 15)
60	// One read builds the whole page.	60	// One read builds the whole page.
61	#expect(stub.seen.count == 1)	61	#expect(stub.seen.count == 1)
62	#expect(stub.seen[0].url.query() == "argv=profile&argv=show&argv=cmc")	62	let first = try #require(stub.seen.first)
		63	#expect(first.url.query() == "argv=profile&argv=show&argv=cmc")
63	}	64	}
64		65
65	@Test func anOrgProfileCarriesMembersInsteadOfOrgs() async throws {	66	@Test func anOrgProfileCarriesMembersInsteadOfOrgs() async throws {

gitbayTests/RefsMilestoneTests.swift +13 −2

	@@ -107,10 +107,21 @@ struct MilestoneListViewModelTests {
107	await model.load()	107	await model.load()
108		108
109	model.filter = .closed	109	model.filter = .closed
110	try await Task.sleep(for: .milliseconds(300))	110	// The reload happens in a spawned task; wait for it to settle
		111	// rather than for a fixed interval to pass. Checking `model.state`
		112	// alone is not enough: right after the assignment, before the
		113	// spawned task has even run, the state is still the previous
		114	// load's non-loading result, which would satisfy a bare
		115	// "not loading" check before the reload ever started.
		116	await until {
		117	guard stub.seen.count == 2 else { return false }
		118	if case .loading = model.state { return false }
		119	return true
		120	}
111		121
112	#expect(stub.seen.count == 2)	122	#expect(stub.seen.count == 2)
113	#expect(stub.seen[1].url.query()?.contains("argv=closed") == true)	123	let second = try #require(stub.seen.dropFirst().first)
		124	#expect(second.url.query()?.contains("argv=closed") == true)
114	}	125	}
115	}	126	}
116		127

gitbayTests/SearchTests.swift +1 −1

	@@ -137,7 +137,7 @@ struct SearchViewModelTests {
137		137
138	stub.enqueue(.init(status: 200, json: mixedResults))	138	stub.enqueue(.init(status: 200, json: mixedResults))
139	model.kind = .issue	139	model.kind = .issue
140	try await Task.sleep(for: .milliseconds(150))	140	await until { stub.seen.count == 2 }
141		141
142	#expect(argvFrom(try #require(stub.seen.last).url)	142	#expect(argvFrom(try #require(stub.seen.last).url)
143	== ["search", "parity", "--kind", "issue"])	143	== ["search", "parity", "--kind", "issue"])

gitbayTests/SessionStoreTests.swift +3 −2

         await #expect(throws: GitbayError.self) {
             _ = try await client.readList(["repo", "list"], of: RepoSummary.self)
         }
-        // The sign-out hops through the main actor; give it a beat.
+        // The sign-out hops through the main actor; wait for it rather
-        try await Task.sleep(for: .milliseconds(200))
+        // than for a fixed interval to pass.
+        await until { session.current == nil }
         #expect(session.current == nil)
         #expect(session.client == nil)

gitbayTests/StubProtocol.swift +16

	@@ -161,3 +161,19 @@ nonisolated final class Mutex<Value>: @unchecked Sendable {
161	return body(&value)	161	return body(&value)
162	}	162	}
163	}	163	}
		164
		165	/// Wait for a condition rather than sleeping a fixed interval. The view
		166	/// models reload through unstructured Tasks, so a fixed sleep is a race:
		167	/// under load the reload has not landed when it expires. Polling is both
		168	/// faster in the common case and deterministic.
		169	@MainActor
		170	func until(
		171	_ timeout: Duration = .seconds(2),
		172	_ condition: () -> Bool
		173	) async {
		174	let deadline = ContinuousClock.now + timeout
		175	while !condition() {
		176	guard ContinuousClock.now < deadline else { return }
		177	try? await Task.sleep(for: .milliseconds(10))
		178	}
		179	}

Commit cc8a97bf86

Commit `cc8a97bf86`