Skip to content

Conversation

matanlurey
Copy link
Contributor

Closes flutter/flutter#156424.

Also improved an error message that came up as a result.

Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just a question about a removed test.

asset: _videoAssetKey,
)))!;

testWidgets('can be paused', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add back a test for pausing like this one that was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F. Yup, that was a booboo - re-added. Thanks for calling this out! Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there was an additional call to await tester.pumpAndSettle(_playDuration) after pause was called in the original test. Maybe that explains the slight difference in player.getPosition(textureId) and pausedDuration causing the test failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let's try it :)

@matanlurey matanlurey requested a review from camsim99 October 15, 2024 20:25
@matanlurey
Copy link
Contributor Author

Thanks @camsim99! PTAL again

Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but there's a test failure. Left a comment on maybe why

asset: _videoAssetKey,
)))!;

testWidgets('can be paused', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there was an additional call to await tester.pumpAndSettle(_playDuration) after pause was called in the original test. Maybe that explains the slight difference in player.getPosition(textureId) and pausedDuration causing the test failure?

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2024
@auto-submit auto-submit bot merged commit a35f02d into main Oct 15, 2024
76 checks passed
@auto-submit auto-submit bot deleted the video_player_android-e2e branch October 15, 2024 22:59
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 16, 2024
flutter/packages@bf751e6...a35f02d

2024-10-15 [email protected] Convert use of `mini_player` in the `video_player_android` test to `AndroidVideoPlayer`. (flutter/packages#7847)
2024-10-15 [email protected] [interactive_media_ads] Adds internal wrapper for Android native `UniversalAdId` (flutter/packages#7833)

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-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
Lurchfresser pushed a commit to Lurchfresser/flutter that referenced this pull request Oct 17, 2024
…r#156983)

flutter/packages@bf751e6...a35f02d

2024-10-15 [email protected] Convert use of `mini_player` in the `video_player_android` test to `AndroidVideoPlayer`. (flutter/packages#7847)
2024-10-15 [email protected] [interactive_media_ads] Adds internal wrapper for Android native `UniversalAdId` (flutter/packages#7833)

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-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor video_player_android/example/integration_test to _not_ use MiniController
2 participants