-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player]: initial test coverage file() constructor api #3407
[video_player]: initial test coverage file() constructor api #3407
Conversation
9640f73
to
4c5b313
Compare
packages/video_player/video_player/example/integration_test/video_player_test.dart
Show resolved
Hide resolved
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.
Looks reasonable to me with just a question on web platform exclusion (or perhaps the opposite: android ios specific inclusion (in case macos is also not supported but toggled on?). Also a note on simulator not working but maybe that's not for file constructor, unknown. The homepage is dire with all the warnings!
Thanks for the submission! We're always happy to see PRs that increase test coverage. We apologize for the long delay in triaging this PR. We’re in the process of overhauling our PR triage system to respond much more quickly, as well as working through the backlog. If you could resolve the conflicts here, we can get this landed. The version update can actually just be dropped since this is a test-only change, and thus doesn't need to be published. |
packages/video_player/video_player/example/integration_test/video_player_test.dart
Show resolved
Hide resolved
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.
I resolved the conflict; LGTM
This pull request is not suitable for automatic merging in its current state.
|
PR includes a test case for
VideoPlayerController.file()
constructor which doesn't have an integration test currently.There are no issues addressed by this PR.
Pre-launch Checklist
[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.