[video_player] Activate leak testing #8379

ValentinVignal · 2025-01-07T13:12:05Z

The package manipulates disposable objects. This PR activates leak testing to make sure disposable objects are correctly disposed.

It also fixes the memory leak warnings from the tests

See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ValentinVignal · 2025-01-07T13:12:16Z

Since I modified only the tests, I think this PR is eligible for the tags override: no versioning needed and override: no changelog needed

ValentinVignal · 2025-01-07T13:12:22Z

cc @polina-c

ValentinVignal · 2025-01-07T13:15:21Z

packages/video_player/video_player/test/video_player_test.dart

@@ -698,6 +723,7 @@ void main() {
        expect(controller.value.isPlaying, isTrue);

        await controller.pause();
+        await tester.runAsync(controller.dispose);


For those ones, I didn't manage to make the test pass without using runAsync.

If I use

addTearDown(() { controller.dispose(); });

or

// end of test unawaited(controller.dispose()); // a lot of tester.pump(); tester.pump();

it doesn't fix the memory leak.

If I use

addTearDown(controller.dispose());

or

// end of test await controller.dispose();

the test hangs and times out.

Do you have an idea on how to do it better?

It looks smelly for me that dispose is async. But, it is test only class, so it is ok.
And thus runAsync makes perfect sense.

Are these tests doing anything to test the real implementation of this class (VideoPlayerController)? In the actual usage of dispose there are asynchronous happenings. I'm not super familiar with how the leak testing works.

From what I remember, they do. Those controller were created and I had issues making the tests wait for the dispose methods to finish

ValentinVignal · 2025-01-18T05:30:17Z

Any thoughts on this PR @tarrinneal ?

tarrinneal · 2025-01-21T22:14:25Z

packages/video_player/video_player/pubspec.yaml

@@ -33,6 +33,7 @@ dependencies:
 dev_dependencies:
  flutter_test:
    sdk: flutter
+  leak_tracker_flutter_testing: any


I'm not sure what our version policies are for dev_deps. @stuartmorgan is there guidance on this?

For more context, here is the documentation: https://prudential-ps.atlassian.net/browse/GTP-2844

Put any instead of version, because the version is defined by your Flutter SDK.

Yes, generally this would be a very bad idea, but it's specifically needed for the case of this package (IIUC because it's not in the SDK, so can't use sdk:, but is pinned by Flutter).

tarrinneal · 2025-01-21T22:31:02Z

packages/video_player/video_player/test/video_player_test.dart

@@ -698,6 +723,7 @@ void main() {
        expect(controller.value.isPlaying, isTrue);

        await controller.pause();
+        await tester.runAsync(controller.dispose);


Are these tests doing anything to test the real implementation of this class (VideoPlayerController)? In the actual usage of dispose there are asynchronous happenings. I'm not super familiar with how the leak testing works.

tarrinneal · 2025-01-22T21:57:04Z

@stuartmorgan another question on this one. I think it should be exempt from changelog and versioning as it is only changing tests and dev-dependencies.

tarrinneal

lgtm sans response from Stuart about versioning.

stuartmorgan-g · 2025-01-23T01:21:52Z

Version/changelog override: dev-only changes are exempt

stuartmorgan-g · 2025-01-23T01:23:22Z

@stuartmorgan another question on this one. I think it should be exempt from changelog and versioning as it is only changing tests and dev-dependencies.

That's correct; the version-check tooling just hasn't been taught to do analysis deeper than file path level, so it can't currently distinguish between pubspec.yaml chages that need publishing, and those that only affect dev_dependencies. I think I filed it at some point, but it hasn't been a priority since it's easy to override.

tarrinneal · 2025-01-23T01:57:56Z

@stuartmorgan another one for your list

stuartmorgan-g

This shouldn't need a second approval, but LGTM

flutter/packages@3d3ab7b...258f6dc 2025-01-24 adsonpleal@gmail.com [shared_preferences] Add shared preferences devtool (flutter/packages#8494) 2025-01-24 tarrinneal@gmail.com [shared_preferences] update List<String> encode/decode (flutter/packages#8335) 2025-01-24 engine-flutter-autoroll@skia.org Manual roll Flutter from c1561a4 to c1ffaa9 (21 revisions) (flutter/packages#8498) 2025-01-24 stuartmorgan@google.com [ios_platform_images] Switch to `loadImage` (flutter/packages#8216) 2025-01-24 mchudy@users.noreply.github.com [camera] Remove OCMock from CameraExposureTests and CameraFocusTests (flutter/packages#8351) 2025-01-24 tarrinneal@gmail.com [shared_preferences] Tool for migrating from legacy shared_preferences to shared_preferences_async (flutter/packages#8229) 2025-01-23 stuartmorgan@google.com Revert "[shared_preferences] Add shared preferences devtool" (flutter/packages#8493) 2025-01-23 20254485+kaboc@users.noreply.github.com [go_router] Fix return type of current state getter to be non-nullable (flutter/packages#8173) 2025-01-23 engine-flutter-autoroll@skia.org Manual roll Flutter from b2f515f to c1561a4 (18 revisions) (flutter/packages#8491) 2025-01-23 tarrinneal@gmail.com [pigeon] fixes event channel dart instance name usage and adds test (flutter/packages#8483) 2025-01-23 51104750+AffanShaikhsurab@users.noreply.github.com [go _route] fragment parameter added (flutter/packages#8232) 2025-01-23 mchudy@users.noreply.github.com [in_app_purchase] Update in_app_purchase_android version in in_app_purchase (flutter/packages#8463) 2025-01-23 stuartmorgan@google.com [image_picker] Reference alternate macOS implementations (flutter/packages#8487) 2025-01-23 32538273+ValentinVignal@users.noreply.github.com [rfw] Activate leak testing (flutter/packages#8370) 2025-01-23 32538273+ValentinVignal@users.noreply.github.com [video_player] Activate leak testing (flutter/packages#8379) 2025-01-23 engine-flutter-autoroll@skia.org Manual roll Flutter from b9e86a5 to b2f515f (42 revisions) (flutter/packages#8482) 2025-01-23 olli.helenius@codemate.com [camera] Add API support query for image streaming (app-facing) (flutter/packages#8422) 2025-01-23 engine-flutter-autoroll@skia.org Manual roll Flutter from b9e86a5 to eb6af3d (13 revisions) (flutter/packages#8473) 2025-01-23 adsonpleal@gmail.com [shared_preferences] Add shared preferences devtool (flutter/packages#8322) 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 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

The package manipulates disposable objects. This PR activates leak testing to make sure disposable objects are correctly disposed. It also fixes the memory leak warnings from the tests See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

ValentinVignal requested a review from tarrinneal as a code owner January 7, 2025 13:12

github-actions bot added the p: video_player label Jan 7, 2025

ValentinVignal commented Jan 7, 2025

View reviewed changes

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Jan 7, 2025

ValentinVignal added 2 commits January 15, 2025 21:48

Activate leak testing

8decf40

Fix the tests

19261d2

ValentinVignal force-pushed the video-player/activate-leak-testing branch from b1e7de6 to 19261d2 Compare January 15, 2025 13:48

leak_tracker_flutter_testing as dev dependency

f84d92b

tarrinneal reviewed Jan 21, 2025

View reviewed changes

tarrinneal approved these changes Jan 22, 2025

View reviewed changes

stuartmorgan-g added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Jan 23, 2025

tarrinneal requested a review from stuartmorgan-g January 23, 2025 01:57

stuartmorgan-g approved these changes Jan 23, 2025

View reviewed changes

ValentinVignal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2025

auto-submit bot merged commit 4e53528 into flutter:main Jan 23, 2025
77 checks passed

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Packages from 3d3ab7bcb07e to 4e5352869f4b (5 revisions) flutter/flutter#162079

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 23, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

7787a40

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Packages from 3d3ab7bcb07e to 61aeda5abaac (6 revisions) flutter/flutter#162084

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 23, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

0349871

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Packages from 3d3ab7bcb07e to ebb373e8e1f0 (7 revisions) flutter/flutter#162090

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 23, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

a8d960d

engine-flutter-autoroll mentioned this pull request Jan 24, 2025

Roll Packages from 3d3ab7bcb07e to 73857c68c1ce (15 revisions) flutter/flutter#162160

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

d9177d8

engine-flutter-autoroll mentioned this pull request Jan 24, 2025

Roll Packages from 3d3ab7bcb07e to 73857c68c1ce (15 revisions) flutter/flutter#162169

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

f173188

engine-flutter-autoroll mentioned this pull request Jan 24, 2025

Roll Packages from 3d3ab7bcb07e to 1833b326a1d0 (16 revisions) flutter/flutter#162175

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 24, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

ce0edb9

engine-flutter-autoroll mentioned this pull request Jan 27, 2025

Roll Packages from 3d3ab7bcb07e to 258f6dce6a49 (19 revisions) flutter/flutter#162254

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2025

4e5352869 [video_player] Activate leak testing (flutter/packages#8379)

860e77e

[video_player] Activate leak testing #8379

[video_player] Activate leak testing #8379

Uh oh!

Conversation

ValentinVignal commented Jan 7, 2025

Pre-launch Checklist

Uh oh!

ValentinVignal commented Jan 7, 2025

Uh oh!

ValentinVignal commented Jan 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Jan 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarrinneal commented Jan 22, 2025

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jan 23, 2025

Uh oh!

stuartmorgan-g commented Jan 23, 2025

Uh oh!

tarrinneal commented Jan 23, 2025

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!