Skip to content

Commit

Permalink
[video_player] Try to address test flake (flutter#6899)
Browse files Browse the repository at this point in the history
This attempts to address two sources of flake:
- A test that playing doesn't continue past the duration flakily fails, at least on Android, with a position a small amount past the duration. This seems like an unexpected library behavior that we wouldn't want to expose to clients, so rather than change the test, this makes the logic that updates the `Value` clamp the position to the duration.
- A test that asset videos play has been restructured to actually wait for the future that should start playing to complete before checking whether it's playing. The test was previously not actually waiting for anything other than animations to complete, and there's no reason the placeholder layout couldn't have completed before the asset loaded. The fact that the test was already disabled for iOS is strong evidence that the flake we are seeing on Android is a problem with the test itself, so hopefully this addresses both platforms.

Fixes flutter/flutter#86915
  • Loading branch information
stuartmorgan authored Jun 10, 2024
1 parent 80f0e16 commit 61ed839
Showing 4 changed files with 14 additions and 5 deletions.
3 changes: 2 additions & 1 deletion packages/video_player/video_player/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 2.8.7

* Ensures that `value.position` never reports a value larger than `value.duration`.
* Updates minimum supported SDK version to Flutter 3.16/Dart 3.2.

## 2.8.6
Original file line number Diff line number Diff line change
@@ -194,9 +194,11 @@ void main() {

testWidgets('test video player view with local asset',
(WidgetTester tester) async {
final Completer<void> loaded = Completer<void>();
Future<bool> started() async {
await controller.initialize();
await controller.play();
loaded.complete();
return true;
}

@@ -221,12 +223,12 @@ void main() {
),
));

await loaded.future;
await tester.pumpAndSettle();
expect(controller.value.isPlaying, true);
},
skip: kIsWeb || // Web does not support local assets.
// Extremely flaky on iOS: https://github.com/flutter/flutter/issues/86915
defaultTargetPlatform == TargetPlatform.iOS);
// Web does not support local assets.
skip: kIsWeb);
});

group('file-based videos', () {
6 changes: 6 additions & 0 deletions packages/video_player/video_player/lib/video_player.dart
Original file line number Diff line number Diff line change
@@ -743,6 +743,12 @@ class VideoPlayerController extends ValueNotifier<VideoPlayerValue> {
}

void _updatePosition(Duration position) {
// The underlying native implementation on some platforms sometimes reports
// a position slightly past the reported max duration. Clamp to the duration
// to insulate clients from this behavior.
if (position > value.duration) {
position = value.duration;
}
value = value.copyWith(
position: position,
caption: _getCaptionAt(position),
2 changes: 1 addition & 1 deletion packages/video_player/video_player/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@ description: Flutter plugin for displaying inline video with other Flutter
widgets on Android, iOS, and web.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.8.6
version: 2.8.7

environment:
sdk: ">=3.2.3 <4.0.0"

0 comments on commit 61ed839

Please sign in to comment.