-
Notifications
You must be signed in to change notification settings - Fork 405
Fix one more flaky test #5939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix one more flaky test #5939
Conversation
…nterDoesNotFail()` test
|
@RCGitBot please test |
| await Task.yield() | ||
| _ = await (task0.value, task1.value) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a very reliable way to make these kinds of tests more stable without needing to wait for the tasks.
The trick is to use a lower priority on a different task needing to yield.
I made a helper for that a while ago XCTestCase+Yield.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see usages of it in the. FileRepository tests
func yield() async {
await Task(priority: .low) {
await Task.yield()
}.value
}
// 👇 this Task ensures the file repository has a user initiated priority
// when it dispatches its own tasks
await Task(priority: .userInitiated) {
// 👇 this dispatches its own task
sut.fileRepository.prefetch(urls: [someURL])
}.value
// 👇 this yields on a low priority thread forcing the structured concurrency
// system to pick up a higher priority task
await yield()
// because Tasks inherit the current scope's priority, we have a stable test without needing to explicitly wait for the dispatch nested taskThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense. Thanks for the tip! ❤️
The test
EventsManagerIntegrationTests.testPostingCustomerCenterDoesNotFail()was sometimes failing. E.g. here (even though it passed after 3 retries).This PR makes the test more deterministic by waiting for the execution of
Purchases.shared.track(customerCenterEvent: )to complete before checking the logs.In addition, the logs check is also more strict now, checking for the right event name + event identifier.
More details
I suspect the reason of the random failures is that
Task.yield()would not always wait for the track eventTasks to complete, resulting in the test checks happening before the expected logs actually happened.