AB#268289 test: make the auth and analytics timing tests deterministic - #150
Merged
k-antipochkin merged 1 commit intoAug 20, 2026
Merged
Conversation
AB#268289 Three tests decided their outcome by racing wall-clock delays, which is what makes them flake on a loaded runner rather than on a broken change. - test_getToken_ignoresProviderCompletionAfterTimeout raced a 0.1s timeout against a 0.3s provider inside a 0.6s wait. The provider now holds its token until the test has observed the timeout, so no scheduling can invert the two, and the test asserts the completion ran exactly once instead of relying on an inverted expectation. - test_number_of_sent_events_with_delays_same_as_tracked slept 0.5s and assumed the first drain had finished. It now waits for the first delivery. - test_failed_network_event_should_be_picked_up_by_subsequent slept 2s and assumed the first send had failed. The mock now reports that. - test_number_of_sent_events_from_background_threads_same_as_tracked checked the count while background queues were still delivering. It still tracks concurrently, but counts once those deliveries are known. Both mocks kept mutable state that the helper's serial queue wrote and the test thread read; that is now behind a lock, so the tests cannot race the mock itself. Every remaining wait is a generous upper bound on something already forced to happen, so load can only make these tests slower, never wrong. Removing the two sleeps also takes the suite from ~9s to ~5.3s. Verified by mutation: dropping AuthManager's completeOnce guard fails with "(2) is not equal to (1) - The token that arrived after the timeout should have been dropped".
There was a problem hiding this comment.
Pull request overview
This PR makes the authentication and analytics timing-related unit tests deterministic by removing reliance on wall-clock sleeps and by synchronizing test mocks that were previously accessed concurrently from multiple threads/queues. It’s a follow-up on the feature/268289-auth line of work and only changes tests (no production code).
Changes:
- Reworks
AnalyticsHelperTeststo sequence phases usingtrackEventcompletion callbacks and explicit expectations instead ofasyncAfter/sleep-based timing. - Adds locking around shared mutable state in
MockKSHttpClientandMockKSHttpClientSingleFailureto prevent races between the helper’s queues and the test thread. - Rewrites
AuthManagerTests.test_getToken_ignoresProviderCompletionAfterTimeoutto enforce the “late provider completion” ordering with a semaphore gate and validates behavior via completion-counting (removing inverted expectations).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| OptimoveSDK/Tests/Sources/Optimobile/AnalyticsHelperTests.swift | Removes timing races in analytics tests and makes HTTP mocks thread-safe via locking. |
| OptimoveCore/Tests/Sources/Auth/AuthManagerTests.swift | Makes the timeout/late-completion test deterministic using enforced ordering and a generous timeout bound. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
eligutovsky
added a commit
that referenced
this pull request
Aug 5, 2026
#134 AB#292773 PR #134 fixes the same flake at the root: `storeUrlOverride` gives each test its own store, which is real isolation rather than deleting a database file that tests happen to share. That is strictly better than what this branch did, and keeping both would only conflict — the two touch the same setUp, tearDown and count assertions. AnalyticsHelperTests goes back to master's version here. The remaining timing work on those tests now lives in #150, on top of #134. SessionHelperTests is unaffected and stays: nothing else covers that sessionDidEnd must not block on tracking.
3 tasks
k-antipochkin
self-requested a review
August 20, 2026 14:13
k-antipochkin
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AB#268289
Targets
feature/268289-auth, notmaster— this is a follow-up to #134, not a separate change.Description of Changes
#134 already found the real cause of the
AnalyticsHelperTestsflake, and thestoreUrlOverrideper-test store is the right fix — better than deleting a shared database file, which is what I had
done independently in #146. This PR does not touch that.
What it changes is the tests that still decide their outcome by racing wall-clock delays. Those
flake when the machine is slow, not when the code is wrong, which is the worst property a test can
have: it fails for a reason unrelated to the change under review, and it passes a regression when
the machine happens to be fast.
test_getToken_ignoresProviderCompletionAfterTimeoutraced a 0.1s timeout against a 0.3sprovider inside a 0.6s wait. Two ways to fail without any bug: the timer slips past 0.3s and the
provider wins, or the whole thing exceeds 0.6s. Widening the margins to 3x makes both less likely
but neither impossible.
The ordering under test — provider answers after the timeout — is now enforced instead of raced.
The provider blocks on a semaphore the test only signals once it has seen the timeout, so no
scheduling can invert the two:
The inverted expectation is gone too. Counting the completions says the same thing and says it
better: an inverted expectation reports "something happened that shouldn't have", a count reports
(2) is not equal to (1).Three more, same class:
..._with_delays_same_as_tracked..._failed_network_event_should_be_picked_up_by_subsequentonFailureDelivered)..._from_background_threads_same_as_trackedBoth mocks were unsynchronised.
MockKSHttpClient.allBatchesandMockKSHttpClientSingleFailure's state are written from the helper's serial queue and read fromthe test thread. A torn read of an array is a corrupt count, not a caught bug, so the tests could
race the mock itself regardless of anything the code under test did. Both are now behind a lock.
Verification
Full suite: 85 tests, 0 failures. No production code changed.
Mutation-checked. Removing
AuthManager.completeOnce'sdidCompleteguard — the exactbehaviour the test exists to protect — fails it, and with a message that names the problem:
The suite got faster: ~9s → ~5.3s, because two real sleeps totalling 2.5s are gone.
What I could not verify, and why the change still stands
I tried to measure whether the flake is gone, and the measurement has no power, so I am not
claiming it. Running
AnalyticsHelperTests8 times under full 11-core CPU load on unfixedmasterpassed 8/8 — the suite slowed from 2.8s to 31s, so the load was biting, but the flakedid not reproduce. It needs something about the GitHub-hosted runner that I cannot reproduce
locally. So my 10/10 on
feature/268289-authsays nothing either, and neither run is evidenceabout #134's fix.
The argument for this PR is structural rather than statistical: a test with no wall-clock margin
in it has no margin to lose. Every wait that remains bounds something already forced to happen, so
load can make these tests slower and nothing else.
The one wait I did not make deterministic is
test_getToken_returnsTimeoutWhenProviderDoesNotComplete— it waits on a real 0.05s timer,because that is the thing under test. Its bound went from 1s to the generous constant.
Breaking Changes
Test-only. No version bump — this rides on #134's 6.9.0.
Release Checklist
Covered by #134; nothing separate to release.
pod lib lint— n/a, no shipped code changedIntegration tests
n/a — no shipped code changed.