-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[video_player] Convert iOS unit tests to Swift #10989
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?
[video_player] Convert iOS unit tests to Swift #10989
Conversation
| private(set) nonisolated(unsafe) var lastSeekTime: CMTime = .invalid | ||
|
|
||
| override func seek( | ||
| to time: CMTime, toleranceBefore: CMTime, toleranceAfter: CMTime, |
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.
nit: separate lines per style guide -> "Line-Wrapping" section.
Filed: flutter/flutter#182175
|
|
||
| /// An AVPlayer subclass that records method call parameters for inspection. | ||
| // TODO(stuartmorgan): Replace with a protocol like the other classes. | ||
| @MainActor class InspectableAVPlayer: AVPlayer { |
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.
nit: final class (here and rest of this file)
| @MainActor class InspectableAVPlayer: AVPlayer { | ||
| private(set) nonisolated(unsafe) var beforeTolerance: NSNumber? | ||
| private(set) nonisolated(unsafe) var afterTolerance: NSNumber? | ||
| private(set) nonisolated(unsafe) var lastSeekTime: CMTime = .invalid |
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.
Interesting. AVPlayer is main actor, but seek is explicitly marked as non-isolated, meaning that Apple likely has some internal synchronization to make seek safe to be called from different threads.
We could make these 3 thread-safe too, but since it's testing only and we don't access it from different threads, using nonisolated(unsafe) is fine here
| } | ||
|
|
||
| class StubEventListener: NSObject, FVPVideoEventListener { | ||
| var initializationContinuation: CheckedContinuation<Void, Never>? |
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.
Having continuation as ivar is non-standard practice, as it's very easy to misuse (e.g. call resume twice). How about:
class StubEventListener... {
var onInitialized: (() -> Void)?
...
}
@Test func fooTest() {
// ...
await withCheckedContinuation { continuation in
listener.onInitialized = { continuation.resume() }
}
}
| let mp4TestURI = "https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4" | ||
| let hlsTestURI = "https://flutter.github.io/assets-for-api-docs/assets/videos/hls/bee.m3u8" | ||
| let mp3AudioTestURI = "https://flutter.github.io/assets-for-api-docs/assets/audio/rooster.mp3" | ||
| let hlsAudioTestURI = |
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.
nit: private static let
| } | ||
|
|
||
| // Temporary test adapter until the player implementation is converted to use async. | ||
| private func asyncSeekTo(player: FVPVideoPlayer, time: Int) async { |
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.
Swift auto-generate the async/await API based on completion API. We should already be able to call
let error = await player.seek(to: time)
|
|
||
| let listener = StubEventListener() | ||
| await withCheckedContinuation { initialized in | ||
| listener.initializationContinuation = initialized |
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.
This and other places in this file: if videoPlayerDidInitialize is not called, the test suite would get stuck (rather than marking this test as failure and continuing).
SwiftTesting has a time limit trait: https://developer.apple.com/documentation/testing/trait/timelimit(_:), but it's iOS 16+ only.
Google search sends me to https://www.donnywals.com/implementing-task-timeout-with-swift-concurrency. I cleaned up the code and updated it for Swift 6:
enum TestError: Error {
case timeout
}
func withTimeout(seconds: TimeInterval, operation: @escaping @Sendable () async throws -> Void) async throws {
try await withThrowingTaskGroup(of: Void.self) { group in
group.addTask { try await operation() }
group.addTask {
try await Task.sleep(nanoseconds: UInt64(seconds * 1_000_000_000))
throw TestError.timeout
}
defer { group.cancelAll() }
try await group.next()!
}
}
Then in the test:
try await withTimeout(seconds: 5) {
await withCheckedContinuation { continuation in
listener.onInitialized {
continuation.resume()
}
}
}
I verified it working by adding a sleep:
try await withTimeout(seconds: 5) {
try await Task.sleep(nanoseconds: 10 * 1_000_000_000)
// ...
}
Converts all of the native unit tests in
video_player_avfoundationto Swift:To minimize churn, I deliberately didn't address other technical debt in the tests (e.g., testing of player instances by making an entire plugin instance just to make a player). Some changes were necessary though:
NS_ASSUME_NONNULL_BEGINannotations to headers, to avoid having to deal with optional values in the Swift version of tests that couldn't ever actually be null.AVAssetTrackinstead ofAVAssetTrack. The tests for that worked by subclassingAVAssetTrackeven though it has no public initializers, which was extremely sketchy. It worked okay in Obj-C because Obj-C will let you get away with almost anything, but it doesn't compile in Swift.AVPlayerItemto actually work in Swift, and rather than try to fix that when it wasn't great test design anyway, I just changed to using something that wasn't an internal detail anyway.Part of flutter/flutter#119105
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3