Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[video_player] Try to address test flake #6899

Merged
merged 3 commits into from
Jun 10, 2024
Merged

[video_player] Try to address test flake #6899

merged 3 commits into from
Jun 10, 2024

Conversation

stuartmorgan
Copy link
Contributor

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

Pre-launch Checklist

@stuartmorgan
[video_player] Try to address test flake
9369341 
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
@stuartmorgan
Copy link
Contributor Author

Since this is speculative I'll re-run the presubmit tests a few times at least to get better signal.

bparrishMines
bparrishMines approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

packages/video_player/video_player/lib/video_player.dart Outdated
// 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.compareTo(value.duration) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would this not be easier to read as position > value.duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, for some reason I thought that didn't work. Yes, definitely much better that way.

@stuartmorgan stuartmorgan mentioned this pull request Jun 10, 2024
Open
@stuartmorgan stuartmorgan added autosubmit Merge PR when tree becomes green via auto submit App warning: land on red to fix tree breakage Override tree-status signal (land even with closed tree), combine with the autosubmit label. labels Jun 10, 2024
@stuartmorgan
Copy link
Contributor Author

I'm going to land this on red because so much of the red today has been one of these two tests flaking.

@auto-submit auto-submit bot merged commit 61ed839 into flutter:main Jun 10, 2024
73 of 74 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 11, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2024
@engine-flutter-autoroll
61ed839d4 [video_player] Try to address test flake (flutter/packages#…
fb3b3fa 
…6899)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 11, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2024
@engine-flutter-autoroll
61ed839d4 [video_player] Try to address test flake (flutter/packages#…
d8ae56a 
…6899)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 12, 2024
@engine-flutter-autoroll
Roll Packages from e95fe4a0cd3a to 8c24fd4ec947 (7 revisions) (#150051)
9983ced 
flutter/packages@e95fe4a...8c24fd4

2024-06-11 engine-flutter-autoroll@skia.org Roll Flutter from 32081aa to 14df7be (21 revisions) (flutter/packages#6907)
2024-06-11 luthien256@gmail.com Ensure each code block specified in the markdown uses its own ScrollController. (flutter/packages#6904)
2024-06-10 stuartmorgan@google.com [video_player] Try to address test flake (flutter/packages#6899)
2024-06-10 stuartmorgan@google.com [ci] Allow `platform` references (flutter/packages#6903)
2024-06-10 magder@google.com Change CODEOWNERS for metrics_center (flutter/packages#6892)
2024-06-10 mit@google.com Remove package:platform source (flutter/packages#6898)
2024-06-10 engine-flutter-autoroll@skia.org Roll Flutter from fc19ecf to 32081aa (9 revisions) (flutter/packages#6896)

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 flutter-ecosystem@google.com,rmistry@google.com 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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@stuartmorgan @arc-yong
[video_player] Try to address test flake (flutter#6899)
c4cf920 
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
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll @victorsanni
Roll Packages from e95fe4a0cd3a to 8c24fd4ec947 (7 revisions) (flutte…
9b82fb9 
…r#150051)

flutter/packages@e95fe4a...8c24fd4

2024-06-11 engine-flutter-autoroll@skia.org Roll Flutter from 32081aa to 14df7be (21 revisions) (flutter/packages#6907)
2024-06-11 luthien256@gmail.com Ensure each code block specified in the markdown uses its own ScrollController. (flutter/packages#6904)
2024-06-10 stuartmorgan@google.com [video_player] Try to address test flake (flutter/packages#6899)
2024-06-10 stuartmorgan@google.com [ci] Allow `platform` references (flutter/packages#6903)
2024-06-10 magder@google.com Change CODEOWNERS for metrics_center (flutter/packages#6892)
2024-06-10 mit@google.com Remove package:platform source (flutter/packages#6898)
2024-06-10 engine-flutter-autoroll@skia.org Roll Flutter from fc19ecf to 32081aa (9 revisions) (flutter/packages#6896)

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 flutter-ecosystem@google.com,rmistry@google.com 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
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll @victorsanni
Roll Packages from e95fe4a0cd3a to 8c24fd4ec947 (7 revisions) (flutte…
db55345 
…r#150051)

flutter/packages@e95fe4a...8c24fd4

2024-06-11 engine-flutter-autoroll@skia.org Roll Flutter from 32081aa to 14df7be (21 revisions) (flutter/packages#6907)
2024-06-11 luthien256@gmail.com Ensure each code block specified in the markdown uses its own ScrollController. (flutter/packages#6904)
2024-06-10 stuartmorgan@google.com [video_player] Try to address test flake (flutter/packages#6899)
2024-06-10 stuartmorgan@google.com [ci] Allow `platform` references (flutter/packages#6903)
2024-06-10 magder@google.com Change CODEOWNERS for metrics_center (flutter/packages#6892)
2024-06-10 mit@google.com Remove package:platform source (flutter/packages#6898)
2024-06-10 engine-flutter-autoroll@skia.org Roll Flutter from fc19ecf to 32081aa (9 revisions) (flutter/packages#6896)

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 flutter-ecosystem@google.com,rmistry@google.com 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
@reidbaker reidbaker mentioned this pull request Jul 2, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparrishMines bparrishMines bparrishMines approved these changes

@tarrinneal tarrinneal Awaiting requested review from tarrinneal tarrinneal is a code owner

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: video_player warning: land on red to fix tree breakage Override tree-status signal (land even with closed tree), combine with the autosubmit label.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[video_player] integration test is very flaky on iOS
2 participants
@stuartmorgan @bparrishMines