Repository navigation
iOS: Migrate VSyncClient tests to Swift Testing - #190054
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates VSyncClientTest.swift from XCTest to the Swift Testing framework, converting the test class to a struct, adopting @Test annotations, and replacing XCTAssert assertions with #expect macros. Feedback suggests removing redundant local declarations of threadTaskRunner in deallocatesWithoutExplicitInvalidation and deallocatesAfterRegistrationCompletes that shadow the newly introduced struct-level instance property.
7730de3 to
e193546
Compare
| weak var weakClient: VSyncClient? | ||
|
|
||
| autoreleasepool { | ||
| let vsyncExpectation = expectation(description: "vsync") | ||
| let vsyncSignal = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
SwiftTesting has a confirmation API designed this kind of stuff without blocking the thread.
pseudo code:
autoreleasepool {
await confirmation("vsync triggered") { confirm in
let client = VSyncClient(...) { _, _ in
confirm()
}
weakClient = client
threadTaskRunner.postTask {
client.await()
}
}
weakClient?.invalidate()
}
#expect(weakClient == nil)
There was a problem hiding this comment.
I'll update the PR description, but the reason we use a semaphore here is because we can't use this approach. This code runs on a separate thread. We're using makeTaskRunnerWithLabel: which creates a task runner that owns its own fml::Thread. So the client.await() and the vsync callback both run on a dedicated background thread with its own run loop that is completely independent of this test (and the main thread).
confirmation isn't a synchronisation primitive like a semaphore is. There's no wait/signal, it just runs the body and when it returns it asserts that confirm() got called some expected number of times.
Looking at the pseudocode, you set up the client, then you post a task with client.await() -- this will eventually trigger the callback (containing the confirm()) on a background thread. But since we're never blocking to wait on that background thread, this approach would be racy. But also since we've dropped the semaphore there's no more memory fence forcing synchronisation, so our thread local state isn't guaranteed to be synchronised, and even if the dealloc did happen on the other thead, there's no guarantee that state is reflected on the test main thread -- not really a race since without some kind of fence instruction, there's no guarantee the state ever syncs.
There was a problem hiding this comment.
nit: using AsyncStream feels a bit more idiomatic than semaphores (which is basically a withCheckedContinuation but doesn't have the 1 event limitation.
There was a problem hiding this comment.
Sounds good; as discussed over chat, since this is just a straight port to Swift Testing, I'll land this with the minimal changes just keeping the existing pattern nearly identically then follow up with a refactoring to nicer swift concurrency APIs.
Swift Testing has no equivalent of XCTest's expectation / waitForExpectations, so the object-lifetime tests have have been updated to use DispatchSemaphore, which is what we do in ResizeSynchronizerTest. No semantic changes; this is just migrating the tests.
e193546 to
d70686f
Compare
LongCatIsLooong
left a comment
There was a problem hiding this comment.
LGTM but I think withCheckedContinuation would be more idiomatic in all but one place, and AsyncStream can be used instead of withCheckedContinuation, when VSyncClient can fire multiple times. Also the @MainActor doesn't seem to be needed.
| threadTaskRunner = nil | ||
| super.tearDown() | ||
| } | ||
| @MainActor |
There was a problem hiding this comment.
The @MainActor doesn't seem to be necessary (at least for now).
| XCTAssertFalse(callbackTargetTime.isNaN) | ||
| XCTAssertFalse(callbackTargetTime.isInfinite) | ||
| #expect(abs((callbackTargetTime - callbackStartTime) - 1.0 / 60.0) <= 0.0001) | ||
| #expect(callbackTargetTime.isNaN == false) |
There was a problem hiding this comment.
| #expect(callbackTargetTime.isNaN == false) | |
| #expect(!callbackTargetTime.isNaN) | |
| #expect(!callbackTargetTime.isInfinite) |
| weak var weakClient: VSyncClient? | ||
|
|
||
| autoreleasepool { | ||
| let vsyncExpectation = expectation(description: "vsync") | ||
| let vsyncSignal = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
nit: using AsyncStream feels a bit more idiomatic than semaphores (which is basically a withCheckedContinuation but doesn't have the 1 event limitation.
| } | ||
|
|
||
| let backgroundThreadFlushed = expectation(description: "Background thread flushed") | ||
| let backgroundThreadFlushed = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
withCheckedContinuation works here
| let registerExpectation = expectation(description: "Wait for display link registration") | ||
| threadTaskRunner.postTask { registerExpectation.fulfill() } | ||
| waitForExpectations(timeout: 1.0, handler: nil) | ||
| let registered = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
nit: use withCheckedContinuation?
…12325) Manual roll requested by stuartmorgan@google.com flutter/flutter@c83f80b...2a230d1 2026-07-29 zerouali.bardai.omar@gmail.com Clarify CustomScrollView use cases (flutter/flutter#189692) 2026-07-29 brunocorona.alcantar@gmail.com Fix TreeSliver first node clipping during expand/collapse animation (flutter/flutter#188626) 2026-07-29 chris@bracken.jp iOS: Migrate VSyncClient tests to Swift Testing (flutter/flutter#190054) 2026-07-29 32538273+ValentinVignal@users.noreply.github.com Remove no shuffle from flutter_driver extension_test.dart and mock flutter.process channel (flutter/flutter#187559) 2026-07-29 52160996+FMorschel@users.noreply.github.com Adds missing await on `instantiateImageCodecFromBuffer` (flutter/flutter#188910) 2026-07-29 1961493+harryterkelsen@users.noreply.github.com [web] Provide Content-Length header for CanvasKit files in flutter test (flutter/flutter#190155) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Remove @mainactor. These tests run VSyncClient on a dedicated worker thread and synchronize with it directly. Nothing in them touches the main thread or its run loop, so the main-actor isolation serves no purpose. This was spotted by @LongCatIsLooong in the review of flutter#190054. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one PR that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Swift Testing has no equivalent of XCTest's expectation / waitForExpectations, so the object-lifetime tests have have been updated to use DispatchSemaphore, which is what we do in ResizeSynchronizerTest and what most closely resembles the previous code, plus provides the wait semantics and thread synchronisation we need.
confirmation()/confirm()can't be used since it provides no wait/signal semantics.confirmationreturns as soon as its block returns (whether or notconfirmwas called). Ref: Validate asynchronous behaviors section.withCheckedContinuationcan't be used since its docs clearly state"you must invoke the continuation’s resume method exactly once", which isn't viable with a CADisplayLink vsync callback. Ref: Discussion section.
No semantic changes; this is just migrating the tests.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.