lightbox test: Add video player regression tests #694

rajveermalviya · 2024-05-21T16:32:32Z

Adds tests mentioned in #587 (comment)

gnprice · 2024-05-22T19:15:12Z

Let's start using the "buddy review" system here, now that the start of the official GSoC period is imminent and it seems like everyone's actively engaged again.

@sm-sayedi, would you do the first review on this PR?

As a reminder since we're just starting to use this system: when you get a buddy review request, please prioritize it right after important bugs and your regressions, ahead of most of your own PRs. (In particular I don't think anyone is currently working on an issue that would come ahead of doing buddy reviews.)

sm-sayedi · 2024-05-23T20:58:28Z

Seems pretty decent to me. The tests are written cleverly. Good job!

Just one small comment below.

sm-sayedi · 2024-05-23T21:00:17Z

test/widgets/lightbox_test.dart


      check(FakeVideoPlayerPlatform.initialized).isTrue();
      check(FakeVideoPlayerPlatform.isPlaying).isTrue();

      await tester.ensureVisible(find.byType(VideoPlayer));
+
+      FakeVideoPlayerPlatform.cancelTimer();


I think this line is not necessary to add to this and its neighboring tests as it will be called inside FakeVideoPlayerPlatform.reset through addTearDown anyway.

Yeah generally one would think that tearDown callback should run before the timer leak checks, but unfortunately that isn't the case, which is understandable - see flutter/flutter#24166

So, in this test case, the timer is created when the page is initialized and generally should cancel out when page is disposed, but since we don't have that setup (for simplicity of the test) the dispose isn't called on the VideoPlayerController, leading to a "timer leak".

Anyway, I think pushing the lightbox page in a route and pop-ing it before the end of each test (just like how cancelTimer is called currently), would avoid this "hacky" looking cancelTimer function, so I'll change the tests behavior to that.

(Also would like to get to know a better way of doing this, if I am missing something here)

So, turns out dispose is being called, and my tentative workaround (of temporary page route) was completely unnecessary, good to know.

Also after debugging the tests, the timer leak checking actually is triggered in the dispose call, it happens exactly here which is surprising and seems like a magic behaviour (since it's implementation is internal to dart vm).

Anyway after leading to nowhere, I replaced the timer implementation to instead use stopwatch. And everything seems to be correct now.

Cool, the stopwatch seems like a good solution.

Also after debugging the tests, the timer leak checking actually is triggered in the dispose call, it happens exactly here which is surprising and seems like a magic behaviour (since it's implementation is internal to dart vm).

I don't quite follow this part. That line says:

await _eventSubscription?.cancel();

That seems like it's just calling a function, one that doesn't look particularly concerned with a timer leak check. What did you observe that pointed at that being the line where the check happens?

TBH, I am not sure too, debugger does lead to that call and then jumps to an assert that checks timer leaks in TestBinding. Maybe looks like debugger skips/misses some call frames.

It can be tested with this diff and then debugging the first test ('shows a VideoPlayer, and video is playing') with breakpoint set on _controller?.dispose() call in _VideoLightboxPageState.

--- a/test/widgets/lightbox_test.dart +++ b/test/widgets/lightbox_test.dart @@ -39,6 +39,8 @@ class FakeVideoPlayerPlatform extends Fake static bool get hasError => _hasError; static bool get isPlaying => _isPlaying; + static Timer? timer; + static Duration get position { _updatePosition(); return _position; @@ -106,6 +108,8 @@ class FakeVideoPlayerPlatform extends Fake return null; } + timer = Timer.periodic(const Duration(milliseconds: 10), (timer) {}); + _stopwatch = clock.stopwatch(); _initialized = true; _streamController.add(VideoEvent(

Ah, I see. I'm guessing the assert you mean is this one in AutomatedTestWidgetsFlutterBinding._verifyInvariants:

assert(!timersPending, 'A Timer is still pending even after the widget tree was disposed.');

That's called from TestWidgetsFlutterBinding._runTestBody:

// run the test await testBody(); asyncBarrier(); // drains the microtasks in `flutter test` mode (when using AutomatedTestWidgetsFlutterBinding) if (_pendingExceptionDetails == null) { // We only try to clean up and verify invariants if we didn't already // fail. If we got an exception already, then we instead leave everything // alone so that we don't cause more spurious errors. runApp(Container(key: UniqueKey(), child: _postTestMessage)); // Unmount any remaining widgets. await pump(); if (registerTestTextInput) { _testTextInput.unregister(); } invariantTester(); _verifyAutoUpdateGoldensUnset(autoUpdateGoldensBeforeTest && !isBrowser); _verifyReportTestExceptionUnset(reportTestExceptionBeforeTest); _verifyErrorWidgetBuilderUnset(errorWidgetBuilderBeforeTest); _verifyShouldPropagateDevicePointerEventsUnset(shouldPropagateDevicePointerEventsBeforeTest); _verifyInvariants();

I suspect what's happening here is:

After running the test body, it drains microtasks. That should get it through any await foo(); lines where foo isn't waiting on something like a timer.

But at that point, it hasn't done anything to cause the test's widgets to get unmounted.

Only after that does it call runApp with a small const widget, in order to unmount the test's widgets. This is the point where dispose methods get called.

And then it just does an await pump(). That'll get through one round of await foo(); lines, but not a whole series of them.

So, the dispose method you linked to got past the line await _creatingCompleter!.future;, the first await in the method, thanks to that await pump();. That gets it to the line await _eventSubscription?.cancel();, but not past that line.

Probably there should be another asyncBarrier after that await pump(). That'd be a fun change to try upstream. It'd be helpful for tests like the original revision here, where the dispose methods do cleanup that's needed in order to satisfy the test framework's invariants… but it'd also sometimes be helpful in causing tests to fail, catching bugs, if there's some exception that gets thrown within a dispose method but only after a couple of await steps.

gnprice · 2024-05-23T22:11:45Z

@sm-sayedi Thanks for that review!

@sumanthvrao I think this is ready for your review as @rajveermalviya's GSoC mentor, now that it's had a first round of buddy review and there's only one small comment outstanding. Please take a look!

Then @sm-sayedi after the thread above is resolved and buddy review is complete, please remove the "buddy review" label and add "maintainer review", and request a review from @chrisbobbe.

rajveermalviya · 2024-05-24T13:05:45Z

@sm-sayedi Thanks for the review, I pushed a new revision which as mentioned in #694 (comment) uses Stopwatch to track play/pause time instead of a Timer which entirely removes the concern of manually cancelling the timers in each test.

sm-sayedi

Thanks, @rajveermalviya for the revision! LGTM.

Just a small change below.

sm-sayedi · 2024-05-24T15:51:16Z

test/widgets/lightbox_test.dart

+  static List<String> get callLogs => _callLogs;
+  static bool get initialized => _initialized;
+  static bool get hasError => _hasError;
+  static bool get isPlaying => _isPlaying;


Suggested change

static bool get isPlaying => _isPlaying;

static bool get isPlaying => _stopwatch?.isRunning ?? false;

I think instead of using a separate field, _isPlaying for keeping track of the player state, we can replace it with the preceding code. What do you say?

rajveermalviya · 2024-05-24T19:12:31Z

Thanks for the review @sm-sayedi, new revision pushed!

sm-sayedi

Thanks @rajveermalviya ! I am satisfied with this revision for the buddy review. Let's move on to the maintainer review with @chrisbobbe .

chrisbobbe

Thanks @rajveermalviya! Small comments below.

chrisbobbe · 2024-05-29T02:05:49Z

test/widgets/lightbox_test.dart

+      final positionLabel = tester.widget(
+        find.byKey(VideoPositionSliderControl.kCurrentPositionLabelKey)) as VideoDurationLabel;


~~We can simplify by not adding kCurrentPositionLabelKey, and instead doing find.byType(VideoDurationLabel) here, right?~~

Oh I see; this is used to distinguish this VideoDurationLabel (for the current position) from the other VideoDurationLabel (for the total video duration). I don't think I've used key purely to help with widget testing, but this approach is documented, so it seems fine.

chrisbobbe · 2024-06-05T21:35:01Z

test/widgets/lightbox_test.dart

+  static Duration get position {
+    _updatePosition();
+    return _position;
+  }


It looks like this getter is doing substantially more than accessing and returning a value. The side effects of stopping a stopwatch and sending a VideoEvent seem particularly important. Can it be written as a method instead? The Dart style guide says getters should "not have user-visible side effects".

Or maybe updatePosition could be made public, for callers to call?

chrisbobbe · 2024-06-05T21:42:49Z

test/widgets/lightbox_test.dart

+
+    testWidgets('unsupported video shows an error dialog', (tester) async {
+      await setupPage(tester, videoSrc: Uri.parse(FakeVideoPlayerPlatform.kTestUnsupportedVideoUrl));
+      await tester.ensureVisible(find.text("Unable to play the video"));


Suggested change

await tester.ensureVisible(find.text("Unable to play the video"));

await tester.tap(find.byWidget(checkErrorDialog(tester,

expectedTitle: zulipLocalizations.errorDialogTitle,

expectedMessage: zulipLocalizations.errorVideoPlayerFailed)));

(where zulipLocalizations is GlobalLocalizations.zulipLocalizations)

chrisbobbe · 2024-06-05T21:51:38Z

test/widgets/lightbox_test.dart

+      verifySliderPosition(
+        tester, FakeVideoPlayerPlatform.kTestVideoDuration - const Duration(milliseconds: 500));


nit: could put the boring argument, tester, on the first line, leaving the interesting argument on its own line

Suggested change

verifySliderPosition(

tester, FakeVideoPlayerPlatform.kTestVideoDuration - const Duration(milliseconds: 500));

verifySliderPosition(tester,

FakeVideoPlayerPlatform.kTestVideoDuration - const Duration(milliseconds: 500));

chrisbobbe · 2024-06-05T21:57:52Z

test/widgets/lightbox_test.dart

@@ -109,6 +189,16 @@ void main() {
  TestZulipBinding.ensureInitialized();

  group("VideoLightboxPage", () {
+    void verifySliderPosition(WidgetTester tester, Duration duration) {


This is checking more than just the Slider widget; it also checks the position label. So a more general name might be better, and also I think we usually use the word "check" instead of "verify"—how about checkPosition?

chrisbobbe · 2024-06-05T22:13:17Z

test/widgets/lightbox_test.dart

+      check(positionLabel.duration)
+        .equals(duration);


This checks that the VideoDurationLabel widget gets the intended Duration value, but we don't yet check that VideoDurationLabel correctly renders that Duration into the hh:mm:ss format. That would be good to do, either here or in some separate unit tests.

chrisbobbe · 2024-06-06T16:18:37Z

Thanks, LGTM! Labeling for Greg's review.

gnprice

Thanks @rajveermalviya, and thanks @sm-sayedi and @chrisbobbe for the helpful previous reviews! Comments below.

gnprice · 2024-06-06T20:35:28Z

test/widgets/lightbox_test.dart

+  group('VideoDurationLabel', () {
+    const cases = [
+      (Duration(milliseconds: 1),    '00:00',     '1ms'),
+      (Duration(milliseconds: 900),  '00:00',     '900ms'),
+      (Duration(milliseconds: 1000), '00:01',     '1000ms'),


Cool, yeah, these unit tests make a good reason to expose VideoDurationLabel.

gnprice · 2024-06-06T20:39:08Z

lib/widgets/lightbox.dart

@@ -248,18 +248,41 @@ class _ImageLightboxPageState extends State<_ImageLightboxPage> {
  }
 }

-class _VideoPositionSliderControl extends StatefulWidget {
+class VideoDurationLabel extends StatelessWidget {


Nit in commit structure: in this first commit:
349b2be lightbox [nfc]: Factor out VideoDurationLabel for testing visibility

the VideoDurationLabel group of tests can be squashed in, and then call it a lightbox test: commit.

That reduces the number of things that are going on in the main commit, and it also provides some motivation right within this commit to explain why it needs to be a public widget.

gnprice · 2024-06-06T23:04:46Z

test/widgets/lightbox_test.dart

+      final positionLabel = tester.widget<VideoDurationLabel>(
+        find.byKey(VideoPositionSliderControl.kCurrentPositionLabelKey));


Hmm, I don't love the idea of finding this position label by key:

The key isn't something the user sees, so this creates some divergence between what the test is checking and what we ultimately care about delivering.

The key is extra logic, and runtime behavior, that we're adding to the production code in order to provide this hook to the tests.

Typically we solve this problem by searching for some text, or an icon. Those solutions don't immediately apply here. Searching for text could work; it just needs to handle the wrinkle that there's also the "00:10" text showing the total video length.

Probably the cleanest solution here would be find.bySemanticsLabel. If there isn't anything suitable in the semantics tree, then that's an issue it'd be good to fix for its own sake, because that's what screen-readers will use.

gnprice · 2024-06-06T23:09:38Z

test/widgets/lightbox_test.dart


+  static const int _kTextureId = 0xffffffff;
+
+  static final List<String> _callLogs = [];


nit: this is one log, so _callLog would be a better name. A log is a place where a series of entries get written over time to record things that happen. So this list itself is a log, and its elements are entries in the log.

gnprice · 2024-06-06T23:15:54Z

test/widgets/lightbox_test.dart

+  static List<String> get callLogs => _callLogs;
+  static bool get initialized => _initialized;


nit: getters like these should go just above the underlying variable

See:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense
and that's the convention we've followed in our codebase too.

gnprice · 2024-06-06T23:49:51Z

test/widgets/lightbox_test.dart

+      await tester.pump(halfTime);
+      FakeVideoPlayerPlatform.pumpEvents();
+      checkPosition(tester, halfTime);


static void pumpEvents() { if (kTestVideoDuration.compareTo(position) <= 0 && isPlaying) { _pause(); } }

Huh — what's the reason this pumpEvents function is needed? This is a piece of API that the real app code won't be calling; what happens in the real app that takes care of pausing when the video finishes?

In a real app, these events are sent from platform bindings, for example here's android.

The initial revisions of this PR used Timer.periodic instead of Stopwatch, and in that implementation these events were dispatched from the Timer.periodic callback, but with Stopwatch there's no such hook thus the need for an explicit pumpEvents function.

(In this revision; Removed the usage from tests which don't rely on it.)

In a real app, these events are sent from platform bindings, for example here's android.

Hmm, I see.

That explanation should go in a comment, then, including the link to that code. That helps a lot in letting the reader see what assumptions this test scaffolding is making, and compare the scaffold code to the corresponding native code it's mimicking.

The initial revisions of this PR used Timer.periodic instead of Stopwatch, and in that implementation these events were dispatched from the Timer.periodic callback, but with Stopwatch there's no such hook thus the need for an explicit pumpEvents function.

This could still be handled with Timer. Whenever the video starts playing, it's predictable in advance when it should finish, so a timer can be set for it then. (In real life it's not so predictable because it depends on the video's later contents continuing to load from the server, etc., but this test isn't simulating any of that complexity. And the position.compareTo(kTestVideoDuration) >= 0 condition makes basically the same assumption of predictability.)

gnprice · 2024-06-06T23:52:44Z

test/widgets/lightbox_test.dart

+      // At exactly the end of the video.
+      await tester.pump(const Duration(milliseconds: 500));
+      FakeVideoPlayerPlatform.pumpEvents();
+      checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration);
+      check(FakeVideoPlayerPlatform.position).equals(FakeVideoPlayerPlatform.kTestVideoDuration);
+      check(FakeVideoPlayerPlatform.isPlaying).isFalse(); // stopped playing


In particular it seems like this check on isPlaying doesn't really do its job as a test. The reason isPlaying is false at this line is that FakeVideoPlayerPlatform.pumpEvents set it to false (via its call to _pause), which is just part of the test scaffolding itself and not something that happens when running in the app.

replied in #694 (comment)

gnprice · 2024-06-07T00:00:24Z

test/widgets/lightbox_test.dart

+      await gesture.moveBy(Offset(twentyPercent, 0.0));
+      FakeVideoPlayerPlatform.pumpEvents();
+      await tester.pump();
+      checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration * 0.2);


nit:

Suggested change

checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration * 0.2);

checkPosition(tester, 0.2 * FakeVideoPlayerPlatform.kTestVideoDuration);

That's the standard convention for mathematicians and physicists — one would write $0.2 \cdot T$ or $0.2 \, T$, and rarely to never $T \cdot 0.2$ or $T \, 0.2$.

Here, it's helpful also just by foregrounding the part that's more interesting, so it isn't inconspicuously off at the end of a longish line.

Unfortunately, I was unable to get this working with operator overloading extensions, local operator overloads on num/double won't take precedence over the upstream ones.
dart-lang/language#966

Oh I see, I forgot that this is a Duration and not a number. So be it, then.

In any case, for now I've reduced the character count:

- checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration * 0.4); + checkPosition(tester, kTestVideoDuration * 0.4);

gnprice · 2024-06-07T00:03:44Z

test/widgets/lightbox_test.dart

+      await gesture.moveBy(Offset(twentyPercent, 0.0));
+      FakeVideoPlayerPlatform.pumpEvents();
+      await tester.pump();
+      checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration * 0.4);
+      check(FakeVideoPlayerPlatform.position).equals(Duration.zero);
+
+      await gesture.moveBy(Offset(twentyPercent, 0.0));


This test can be made a bit stronger by dragging the slider backward, as well as forward, before finally letting go. One could imagine bugs where either switching directions, or moving backward, triggers some different and wrong behavior.

(I'm getting this idea from just rereading my list at #587 (comment) .)

gnprice · 2024-06-07T00:09:42Z

test/widgets/lightbox_test.dart

+      // Verify that after dragging ends, video position is at the
+      // halfway point, and after that it starts advancing as the time
+      // passes.
+      check(FakeVideoPlayerPlatform.position).equals(halfTime);
+
+      const waitTime = Duration(seconds: 1);
+      await tester.pump(waitTime);
+      FakeVideoPlayerPlatform.pumpEvents();
+      checkPosition(tester, halfTime + (waitTime * 1));
+      check(FakeVideoPlayerPlatform.position).equals(halfTime + (waitTime * 1));


Would this be effective as a regression test for the potential bug it's meant to catch?

I.e., quoting my original request:

a regression test for the bug that the _isSliderDragging and _sliderValue logic is avoiding as explained in your comment quoted at #587 (comment) .

The regression test I suggested there was:

Drag slider from one point to another. Check the slider appears at the place you dragged it to now, and at every frame until it starts advancing again because the video is playing.

Where "every frame" is key, because the bug you describe in that comment is a single-frame glitch. It seems like skipping over a whole second could bypass such a glitch.

While debugging this, I found that it was possible to correctly mitigate flickering of slider rather than the previous workaround of delaying the redraw when listeners were called.

The first commit applies the correct fix: 819118f

Also, making the wait interval lower than 500ms wasn't working because video_player plugin runs a Timer.periodic with 500ms interval for dispatching position updates via the ValueNotifier:
https://github.com/flutter/packages/blob/260102b64c0fac9c66b7574035421fa6c09f5f89/packages/video_player/video_player/lib/video_player.dart#L566

video_player plugin runs a Timer.periodic with 500ms interval for dispatching position updates

Huh indeed — wacky. That'll work fine when the video is like 10 minutes long so that 500ms is no more than a couple of pixels, but seems annoying for videos that are like 5 seconds long.

And yeah, that behavior is very conspicuous when I look for it. E.g. here on this 12-second video:
https://chat.zulip.org/#narrow/stream/9-issues/topic/Spoiler.20collapse.20.2F.20uncollapse.20is.20unreliable/near/1823188

but really even on this 3-minute video it's noticeable:
https://chat.zulip.org/#narrow/stream/7-test-here/topic/video.20test/near/1776292

Certainly out of scope for this PR, though.

rajveermalviya · 2024-06-11T21:36:07Z

Thanks for the review @gnprice, pushed a new revision, PTAL :)

gnprice · 2024-06-12T20:19:47Z

lib/widgets/lightbox.dart

-
-            // The toggling back of '_isSliderDragging' is omitted here intentionally,
-            // see '_handleVideoControllerUpdates'.
+            await widget.controller.seekTo(durationValue);


Hmm I see! This does feel cleaner than the previous solution.

I guess this is an example that would have been caught by the lint rule ~~unawaited_futures~~ actually I guess its twin, discarded_futures. I've just filed #731 to track us enabling those.

In the commit message:

lightbox: Apply correct fix for slider flickering This changes makes it so that the async function `seekTo` is awaited before the `setState` in `Slider.onChangeEnd`, thus first invoking the `ValueNotifier.value` setter which as a side-effect calls the `setState` in `_handleVideoControllerUpdate`, and then the `setState` in `Slider.onChangeEnd`. Resulting in both setState calls to be coalesced in a single redraw. Whereas previously the unawaited `seekTo` call would delay the `ValueNotifier.value` setter call in a microtask.

I don't know what you're referring to when you mention "the ValueNotifier.value setter". Can you expand on that? For example, which object here is the ValueNotifier in question?

It refers to seekTo's implementation which calls into platforms bindings, waits for it's Future to complete and then calls _updatePosition which dispatches position update via ValueNotifier.value setter – resulting in our _handleVideoControllerUpdate to be called.

OK, so the ValueNotifier this is referring to is the controller? In that case I think a helpful rewording would be to replace "invoking the ValueNotifier.value setter" with "setting the controller's value".

gnprice · 2024-06-12T20:37:24Z

test/widgets/lightbox_test.dart

+    final positionMicros = (_stopwatch!.elapsed + _lastSetPosition).inMicroseconds;
+    return Duration(microseconds: math.min(
+      positionMicros, kTestVideoDuration.inMicroseconds));


Breaking these durations down into microseconds here is a bit ugly, and it's reaching into a bit of a detail of the definition of Duration which is that it's in microseconds.

I think it'd be cleaner to keep this in terms of Durations, and just write out the definition of min in terms of <=. That's one of those functions that is so simple a judge could write it 🙂

gnprice · 2024-06-12T20:50:57Z

test/widgets/lightbox_test.dart

+  void reset() {
    _streamController.close();
    _streamController = StreamController<VideoEvent>();
-    initialized = false;
-    isPlaying = false;
+    _callLog.clear();
+    _initialized = false;
+    _lastSetPosition = Duration.zero;
+    _stopwatch?.stop();
+    _stopwatch?.reset();


nit: put these in the same order as the fields are declared — helps with scanning the two lists to compare them, and being sure we're not missing any and the initializers all agree

gnprice · 2024-06-12T21:11:29Z

test/widgets/lightbox_test.dart

+      // Verify that after dragging ends, video position is at the
+      // halfway point, and after that it starts advancing as the time
+      // passes.
+      check(FakeVideoPlayerPlatform.position).equals(halfTime);
+
+      const waitTime = Duration(seconds: 1);
+      await tester.pump(waitTime);
+      FakeVideoPlayerPlatform.pumpEvents();
+      checkPosition(tester, halfTime + (waitTime * 1));
+      check(FakeVideoPlayerPlatform.position).equals(halfTime + (waitTime * 1));


video_player plugin runs a Timer.periodic with 500ms interval for dispatching position updates

Huh indeed — wacky. That'll work fine when the video is like 10 minutes long so that 500ms is no more than a couple of pixels, but seems annoying for videos that are like 5 seconds long.

And yeah, that behavior is very conspicuous when I look for it. E.g. here on this 12-second video:
https://chat.zulip.org/#narrow/stream/9-issues/topic/Spoiler.20collapse.20.2F.20uncollapse.20is.20unreliable/near/1823188

but really even on this 3-minute video it's noticeable:
https://chat.zulip.org/#narrow/stream/7-test-here/topic/video.20test/near/1776292

Certainly out of scope for this PR, though.

rajveermalviya · 2024-06-13T04:18:38Z

Thanks for the review @gnprice, pushed a new revision, PTAL.

gnprice

OK, and here's the rest of a round of review!

I haven't yet read your new revision of a couple of hours ago — I was pulled AFK after writing most of this and just sat back down a few minutes ago. But I think this is mostly on different parts of the code than I commented on above.

gnprice · 2024-06-12T21:17:23Z

test/widgets/lightbox_test.dart

+      // Periodic timer interval at which video_player plugin notifies
+      // of position events.
+      const interval = Duration(milliseconds: 500);
+
+      await tester.pump(interval);
+      checkPosition(tester, halfTime + (interval * 1));
+      check(platform.position).equals(halfTime + (interval * 1));


Continuing the thread #694 (comment) :

I think even with the odd polling-every-500ms behavior that video_player has, that shouldn't be an obstacle to writing a check here that would make this an effective regression test for the potential bug discussed above.

In particular the test I suggested:

Drag slider from one point to another. Check the slider appears at the place you dragged it to now, and at every frame until it starts advancing again because the video is playing.

seems like it describes what happens in the real app today, despite that polling behavior.

When you release the drag, the slider stays there.

It stays there for multiple frames, until the 500ms timer fires.

Then it starts advancing again — i.e. it moves again for the first time, and that movement is forward.

gnprice · 2024-06-12T23:49:30Z

test/widgets/lightbox_test.dart

+      await tester.pump(halfTime);
+      FakeVideoPlayerPlatform.pumpEvents();
+      checkPosition(tester, halfTime);


In a real app, these events are sent from platform bindings, for example here's android.

Hmm, I see.

That explanation should go in a comment, then, including the link to that code. That helps a lot in letting the reader see what assumptions this test scaffolding is making, and compare the scaffold code to the corresponding native code it's mimicking.

The initial revisions of this PR used Timer.periodic instead of Stopwatch, and in that implementation these events were dispatched from the Timer.periodic callback, but with Stopwatch there's no such hook thus the need for an explicit pumpEvents function.

This could still be handled with Timer. Whenever the video starts playing, it's predictable in advance when it should finish, so a timer can be set for it then. (In real life it's not so predictable because it depends on the video's later contents continuing to load from the server, etc., but this test isn't simulating any of that complexity. And the position.compareTo(kTestVideoDuration) >= 0 condition makes basically the same assumption of predictability.)

gnprice · 2024-06-13T00:03:59Z

test/widgets/lightbox_test.dart

+      final positionLabel = tester.widget<VideoDurationLabel>(
+        find.byWidgetPredicate((widget) => widget is VideoDurationLabel
+          && widget.semanticsLabel == "Current position"));
+      check(positionLabel.duration).equals(duration);


Definitely nicer! (Continuing from #694 (comment) .)

But we can go farther and make this entirely in terms of what the user sees:

Suggested change

final positionLabel = tester.widget<VideoDurationLabel>(

find.byWidgetPredicate((widget) => widget is VideoDurationLabel

&& widget.semanticsLabel == "Current position"));

check(positionLabel.duration).equals(duration);

check(tester.widget<RichText>(

find.descendant(of: find.bySemanticsLabel('Current position'),

matching: find.byType(RichText))).text.toPlainText())

.equals(VideoDurationLabel.formatDuration(duration));

gnprice · 2024-06-13T00:15:16Z

test/widgets/lightbox_test.dart

+      testWidgets('with $title shows $expected', (tester) async {
+        await tester.pumpWidget(MaterialApp(home: VideoDurationLabel(duration)));
+        final text = tester.widget<Text>(find.byType(Text));
+        check(text.data).equals(expected);


To go along with the revision just below/above this one (depending on which page you read it on), this can validate that VideoDurationLabel.formatDuration does what one hopes:

Suggested change

check(text.data).equals(expected);

check(text.data)

..equals(VideoDurationLabel.formatDuration(duration))

..equals(expected);

That's not something that needs checking for its own sake — for testing VideoDurationLabel itself, the end-to-end test you already have here is perfect. But it's useful for then using VideoDurationLabel.formatDuration as an ingredient in another test.

gnprice · 2024-06-13T00:15:38Z

lib/widgets/lightbox.dart

+  final Duration duration;
+  final String? semanticsLabel;
+
+  static String formatDuration(Duration value) {


Oh speaking of which, this should get a @visibleForTesting.

gnprice · 2024-06-13T00:33:28Z

test/widgets/lightbox_test.dart

+      final rect = tester.getRect(find.byType(Slider));
+      final trackWidth = rect.width - padding - padding;
+      final trackStartPos = rect.centerLeft + const Offset(padding, 0);
+      final twentyPercent = trackWidth * 0.2; // 20% increments


This variable is best just inlined where it's used — makes things more transparent, and not many more characters.

gnprice · 2024-06-13T00:33:58Z

test/widgets/lightbox_test.dart

+      checkPosition(tester, Duration.zero);
+      check(platform.position).equals(Duration.zero);
+
+      await gesture.moveBy(Offset(twentyPercent, 0.0));


This could be simplified a bit by having an Offset variable:

Suggested change

await gesture.moveBy(Offset(twentyPercent, 0.0));

await gesture.moveBy(trackLength * 0.2);

gnprice · 2024-06-13T00:37:43Z

test/widgets/lightbox_test.dart

+      checkPosition(tester, kTestVideoDuration * 0.4);
+      check(platform.position).equals(Duration.zero);


These pairs of checks can be pulled into a local helper:

Suggested change

checkPosition(tester, kTestVideoDuration * 0.4);

check(platform.position).equals(Duration.zero);

checkPositions(slider: 0.4, video: 0.0);

That cuts out almost all the repetitive bits, which otherwise get in the way of seeing the parts that are changing. As a bonus it makes room to be explicit about the two different kinds of position involved here, as discussed in a comment above.

gnprice · 2024-06-13T05:50:03Z

test/widgets/lightbox_test.dart

+      await gesture.moveBy(Offset(twentyPercent, 0.0));
+      await tester.pump();
+      checkPosition(tester, kTestVideoDuration * 0.6);
+      check(platform.position).equals(Duration.zero);
+
+      await gesture.moveBy(Offset(-twentyPercent, 0.0));
+      await tester.pump();
+      checkPosition(tester, kTestVideoDuration * 0.4);
+      check(platform.position).equals(Duration.zero);
+
+      await gesture.moveBy(Offset(-twentyPercent, 0.0));
+      await tester.pump();
+      checkPosition(tester, kTestVideoDuration * 0.2);
+      check(platform.position).equals(Duration.zero);


This test has more dragging steps than it needs; it's doing now 8 separate gesture.moveBy calls. It could tell the story just as well with two or three steps, like:

0 to 20%

then on to 60%

then back to 40%, and release there

That's enough to show that

it doesn't seek after the first drag;

it doesn't seek after turning around, or moving backward.

And conversely it'd help make the test have fewer things in it to read.

If you want to tell a story saying also that it doesn't seek more than once even after a large number of drag steps, then a loop would be a good way to express that, while keeping it concise. But I think that isn't a scenario I'd worry about; I think if it doesn't seek every time, it's not going to seek after the 10th time just because there've been 10 of them.

gnprice · 2024-06-13T05:53:55Z

test/widgets/lightbox_test.dart

+      const padding = 24.0;
+      final rect = tester.getRect(find.byType(Slider));
+      final trackWidth = rect.width - padding - padding;
+      final trackStartPos = rect.centerLeft + const Offset(padding, 0);


This part would be good to pull out as a little helper function that these two tests can both call.

gnprice

Thanks for the revision! Comments below on the changes there. Then I think my previous round above (#694 (review)), on other parts of the code, is still open for you to act on.

gnprice · 2024-06-14T02:50:17Z

test/widgets/lightbox_test.dart

+    if (pos.compareTo(kTestVideoDuration) >= 0) {
+      return kTestVideoDuration;
+    } else {
+      return pos;
+    }


Suggested change

if (pos.compareTo(kTestVideoDuration) >= 0) {

return kTestVideoDuration;

} else {

return pos;

}

return pos <= kTestVideoDuration ? pos : kTestVideoDuration;

Or is there a subtle difference between that and compareTo which I'm missing?

gnprice · 2024-06-14T02:51:57Z

test/widgets/lightbox_test.dart

+    _stopwatch?.reset();
+    _callLog.clear();
+    _initialized = false;
+    _isCompleted = false;


I see putting these in order was helpful 🙂 (as the _hasError and _isCompleted lines are new in this revision)

gnprice · 2024-06-14T02:56:38Z

lib/widgets/lightbox.dart

-
-            // The toggling back of '_isSliderDragging' is omitted here intentionally,
-            // see '_handleVideoControllerUpdates'.
+            await widget.controller.seekTo(durationValue);


OK, so the ValueNotifier this is referring to is the controller? In that case I think a helpful rewording would be to replace "invoking the ValueNotifier.value setter" with "setting the controller's value".

rajveermalviya · 2024-06-14T05:02:35Z

Thanks for the review @gnprice, pushed a new revision!

This changes makes it so that the async function `seekTo` is awaited before the `setState` in `Slider.onChangeEnd`, thus first setting the controller's value which as a side-effect calls the `setState` in `_handleVideoControllerUpdate`, and then the `setState` in `Slider.onChangeEnd`. Resulting in both setState calls to be coalesced in a single redraw. Whereas previously the unawaited `seekTo` call would delay setting the controller's value in a microtask.

Also introduce various tests for VideoDurationLabel.

Introduce tests for each of the cases mentioned here: zulip#587 (comment)

In particular "A helper to" can be left out; and the fact that the two positions can be expected to differ can be seen from the function's call sites, some of which have them differing.

This makes some of these test cases quite a bit shorter, without losing any information that's relevant to what the test is about. Partly that comes by removing some tokens (like `kTestVideoDuration`) that were repetitive and not the interesting aspect of the test. Partly it comes by then as a result being able to collapse three lines into one line at each of these steps. That brings the different parts of the test closer together and makes them easier to see all at once.

…links

gnprice

Thanks @rajveermalviya for all your work on this! Merging now.

There were a couple of remaining items where I figured it'd be better at this point to demonstrate what I have in mind rather than try to explain it. So I've added a few commits on top:
1e2143c lightbox test [nfc]: Tighten up checkPositions doc
97109a0 lightbox test [nfc]: Tighten test cases using a checkPositionsRelative
b9065e8 lightbox test [nfc]: Extract findSliderPosition
ff029af lightbox test: Revise no-flicker-upon-drag test, and add explanatory links

The second commit completes the changes I was going for in #694 (comment) ; the version in your latest revision is helpful, but doesn't get all of the benefits of concision that I had in mind.

The last commit follows up on #694 (comment), and the next-to-last is just a small refactor to enable that one.

The first commit is a stylistic revision to the handy new doc comment on checkPositions. For background on the changes, see:
https://dart.dev/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph
https://dart.dev/effective-dart/documentation#prefer-starting-function-or-method-comments-with-third-person-verbs

In general all the advice on that page (about documentation style) is worth reading, as is the corresponding section of the Flutter style guide:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#documentation-dartdocs-javadocs-etc

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label May 21, 2024

gnprice added buddy review GSoC buddy review needed. and removed maintainer review PR ready for review by Zulip maintainers labels May 22, 2024

gnprice requested a review from sm-sayedi May 22, 2024 19:15

rajveermalviya force-pushed the video-player-extra-tests branch from 9938edb to f28e841 Compare May 22, 2024 19:53

sm-sayedi reviewed May 23, 2024

View reviewed changes

gnprice added maintainer review PR ready for review by Zulip maintainers and removed buddy review GSoC buddy review needed. labels May 23, 2024

gnprice added buddy review GSoC buddy review needed. mentor review GSoC mentor review needed. and removed maintainer review PR ready for review by Zulip maintainers labels May 23, 2024

gnprice requested a review from sumanthvrao May 23, 2024 22:33

rajveermalviya force-pushed the video-player-extra-tests branch from f28e841 to bdf531f Compare May 24, 2024 12:51

rajveermalviya requested a review from sm-sayedi May 24, 2024 13:05

sm-sayedi reviewed May 24, 2024

View reviewed changes

rajveermalviya force-pushed the video-player-extra-tests branch from bdf531f to 75f365e Compare May 24, 2024 19:11

rajveermalviya requested a review from sm-sayedi May 24, 2024 19:12

sm-sayedi reviewed May 24, 2024

View reviewed changes

sm-sayedi requested a review from chrisbobbe May 24, 2024 19:28

sm-sayedi added maintainer review PR ready for review by Zulip maintainers and removed buddy review GSoC buddy review needed. labels May 24, 2024

rajveermalviya force-pushed the video-player-extra-tests branch from 75f365e to 089bba3 Compare May 31, 2024 11:07

chrisbobbe reviewed Jun 5, 2024

View reviewed changes

rajveermalviya force-pushed the video-player-extra-tests branch 2 times, most recently from 9e3f10e to 7e14787 Compare June 6, 2024 09:38

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jun 6, 2024

gnprice reviewed Jun 7, 2024

View reviewed changes

rajveermalviya force-pushed the video-player-extra-tests branch 3 times, most recently from a5ef118 to c6372be Compare June 11, 2024 21:29

rajveermalviya requested a review from gnprice June 11, 2024 21:36

gnprice reviewed Jun 12, 2024

View reviewed changes

rajveermalviya force-pushed the video-player-extra-tests branch 2 times, most recently from da38c86 to 8454a8c Compare June 13, 2024 04:17

rajveermalviya requested a review from gnprice June 13, 2024 04:18

gnprice reviewed Jun 13, 2024

View reviewed changes

gnprice reviewed Jun 14, 2024

View reviewed changes

rajveermalviya force-pushed the video-player-extra-tests branch from 8454a8c to 68bf211 Compare June 14, 2024 05:01

rajveermalviya requested a review from gnprice June 14, 2024 05:02

rajveermalviya and others added 8 commits June 14, 2024 22:55

lightbox test: Factor out VideoDurationLabel for testing visibility

44c734c

Also introduce various tests for VideoDurationLabel.

lightbox test [nfc]: Refactor FakeVideoPlayerPlatform

71d679e

lightbox test: Add video player regression tests

8f018d4

Introduce tests for each of the cases mentioned here: zulip#587 (comment)

lightbox test [nfc]: Tighten up checkPositions doc

1e2143c

In particular "A helper to" can be left out; and the fact that the two positions can be expected to differ can be seen from the function's call sites, some of which have them differing.

lightbox test [nfc]: Extract findSliderPosition

b9065e8

lightbox test: Revise no-flicker-upon-drag test, and add explanatory …

ff029af

…links

gnprice force-pushed the video-player-extra-tests branch from 68bf211 to ff029af Compare June 15, 2024 06:08

gnprice merged commit ff029af into zulip:main Jun 15, 2024

gnprice reviewed Jun 15, 2024

View reviewed changes

rajveermalviya deleted the video-player-extra-tests branch June 15, 2024 06:14

	static bool get isPlaying => _isPlaying;
	static bool get isPlaying => _stopwatch?.isRunning ?? false;

		final positionLabel = tester.widget(
		find.byKey(VideoPositionSliderControl.kCurrentPositionLabelKey)) as VideoDurationLabel;

-      await tester.ensureVisible(find.text("Unable to play the video"));
+      await tester.tap(find.byWidget(checkErrorDialog(tester,
+        expectedTitle: zulipLocalizations.errorDialogTitle,
+        expectedMessage: zulipLocalizations.errorVideoPlayerFailed)));

		verifySliderPosition(
		tester, FakeVideoPlayerPlatform.kTestVideoDuration - const Duration(milliseconds: 500));

		final positionLabel = tester.widget<VideoDurationLabel>(
		find.byKey(VideoPositionSliderControl.kCurrentPositionLabelKey));


		static const int _kTextureId = 0xffffffff;

		static final List<String> _callLogs = [];

		static List<String> get callLogs => _callLogs;
		static bool get initialized => _initialized;

	checkPosition(tester, FakeVideoPlayerPlatform.kTestVideoDuration * 0.2);
	checkPosition(tester, 0.2 * FakeVideoPlayerPlatform.kTestVideoDuration);

	await gesture.moveBy(Offset(twentyPercent, 0.0));
	await gesture.moveBy(trackLength * 0.2);

		checkPosition(tester, kTestVideoDuration * 0.4);
		check(platform.position).equals(Duration.zero);

	checkPosition(tester, kTestVideoDuration * 0.4);
	check(platform.position).equals(Duration.zero);
	checkPositions(slider: 0.4, video: 0.0);

lightbox test: Add video player regression tests #694

lightbox test: Add video player regression tests #694

Conversation

rajveermalviya commented May 21, 2024

gnprice commented May 22, 2024

sm-sayedi commented May 23, 2024

Choose a reason for hiding this comment

rajveermalviya May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented May 23, 2024 • edited Loading

rajveermalviya commented May 24, 2024

sm-sayedi left a comment • edited Loading

Choose a reason for hiding this comment

sm-sayedi May 24, 2024 • edited Loading

Choose a reason for hiding this comment

rajveermalviya commented May 24, 2024 • edited Loading

sm-sayedi left a comment

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 6, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jun 13, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jun 14, 2024

rajveermalviya May 24, 2024 •

edited

Loading

gnprice commented May 23, 2024 •

edited

Loading

sm-sayedi left a comment •

edited

Loading

sm-sayedi May 24, 2024 •

edited

Loading

rajveermalviya commented May 24, 2024 •

edited

Loading