Skip to content

[video_player] Activate leak testing #8379

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

Merged
merged 3 commits into from
Jan 23, 2025
Merged

[video_player] Activate leak testing #8379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8decf40
Activate leak testing
ValentinVignal Jan 7, 2025
19261d2
Fix the tests
ValentinVignal Jan 7, 2025
f84d92b
leak_tracker_flutter_testing as dev dependency
ValentinVignal Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/video_player/video_player/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ dependencies:
dev_dependencies:
flutter_test:
sdk: flutter
leak_tracker_flutter_testing: any
Copy link
Contributor

@tarrinneal tarrinneal Jan 21, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ValentinVignal ValentinVignal Jan 22, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 22, 2025

Choose a reason for hiding this comment

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

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).


topics:
- video
Expand Down
13 changes: 13 additions & 0 deletions packages/video_player/video_player/test/flutter_test_config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

Future<void> testExecutable(FutureOr<void> Function() testMain) async {
LeakTesting.enable();
LeakTracking.warnForUnsupportedPlatforms = false;
await testMain();
}
41 changes: 41 additions & 0 deletions packages/video_player/video_player/test/video_player_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ void main() {

testWidgets('update texture', (WidgetTester tester) async {
final FakeController controller = FakeController();
addTearDown(controller.dispose);
await tester.pumpWidget(VideoPlayer(controller));
expect(find.byType(Texture), findsNothing);

Expand All @@ -146,6 +147,7 @@ void main() {

testWidgets('update controller', (WidgetTester tester) async {
final FakeController controller1 = FakeController();
addTearDown(controller1.dispose);
controller1.textureId = 101;
await tester.pumpWidget(VideoPlayer(controller1));
expect(
Expand All @@ -155,6 +157,7 @@ void main() {
findsOneWidget);

final FakeController controller2 = FakeController();
addTearDown(controller2.dispose);
controller2.textureId = 102;
await tester.pumpWidget(VideoPlayer(controller2));
expect(
Expand All @@ -169,6 +172,7 @@ void main() {
final FakeController controller = FakeController.value(
const VideoPlayerValue(
duration: Duration.zero, rotationCorrection: 180));
addTearDown(controller.dispose);
controller.textureId = 1;
await tester.pumpWidget(VideoPlayer(controller));
final Transform actualRotationCorrection =
Expand All @@ -189,6 +193,7 @@ void main() {
(WidgetTester tester) async {
final FakeController controller =
FakeController.value(const VideoPlayerValue(duration: Duration.zero));
addTearDown(controller.dispose);
controller.textureId = 1;
await tester.pumpWidget(VideoPlayer(controller));
expect(find.byType(Transform), findsNothing);
Expand Down Expand Up @@ -315,6 +320,7 @@ void main() {
VideoPlayerController.networkUrl(
Uri.parse('https://127.0.0.1'),
);
addTearDown(controller.dispose);
await controller.initialize();
await controller.play();
verifyPlayStateRespondsToLifecycle(controller,
Expand All @@ -334,6 +340,7 @@ void main() {
test('network url', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(Uri.parse('https://127.0.0.1'));
addTearDown(controller.dispose);
await controller.initialize();

expect(
Expand All @@ -356,6 +363,7 @@ void main() {
Uri.parse('https://127.0.0.1'),
formatHint: VideoFormat.dash,
);
addTearDown(controller.dispose);
await controller.initialize();

expect(
Expand All @@ -378,6 +386,7 @@ void main() {
Uri.parse('https://127.0.0.1'),
httpHeaders: <String, String>{'Authorization': 'Bearer token'},
);
addTearDown(controller.dispose);
await controller.initialize();

expect(
Expand All @@ -401,6 +410,7 @@ void main() {

final VideoPlayerController controller =
VideoPlayerController.networkUrl(invalidUrl);
addTearDown(controller.dispose);

late Object error;
fakeVideoPlayerPlatform.forceInitError = true;
Expand Down Expand Up @@ -464,6 +474,7 @@ void main() {
() async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

fakeVideoPlayerPlatform.forceInitError = true;
await controller.initialize().catchError((dynamic e) {});
Expand All @@ -485,6 +496,7 @@ void main() {
test('dispose', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

expect(
controller.textureId, VideoPlayerController.kUninitializedTextureId);
Expand All @@ -500,6 +512,7 @@ void main() {
test('calling dispose() on disposed controller does not throw', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
await controller.dispose();
Expand All @@ -510,6 +523,7 @@ void main() {
test('play', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(Uri.parse('https://127.0.0.1'));
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.isPlaying, isFalse);
Expand All @@ -529,6 +543,7 @@ void main() {
test('play before initialized does not call platform', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

expect(controller.value.isInitialized, isFalse);

Expand All @@ -540,6 +555,7 @@ void main() {
test('play restarts from beginning if video is at end', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
const Duration nonzeroDuration = Duration(milliseconds: 100);
Expand All @@ -557,6 +573,7 @@ void main() {
test('setLooping', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.isLooping, isFalse);
Expand All @@ -568,6 +585,7 @@ void main() {
test('pause', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
await controller.play();
Expand All @@ -583,6 +601,7 @@ void main() {
test('works', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(await controller.position, Duration.zero);
Expand All @@ -595,6 +614,7 @@ void main() {
test('before initialized does not call platform', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

expect(controller.value.isInitialized, isFalse);

Expand All @@ -606,6 +626,7 @@ void main() {
test('clamps values that are too high or low', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(await controller.position, Duration.zero);
Expand All @@ -622,6 +643,7 @@ void main() {
test('works', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.volume, 1.0);
Expand All @@ -635,6 +657,7 @@ void main() {
test('clamps values that are too high or low', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.volume, 1.0);
Expand All @@ -651,6 +674,7 @@ void main() {
test('works', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.playbackSpeed, 1.0);
Expand All @@ -664,6 +688,7 @@ void main() {
test('rejects negative values', () async {
final VideoPlayerController controller =
VideoPlayerController.networkUrl(_localhostUri);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.playbackSpeed, 1.0);
Expand Down Expand Up @@ -698,6 +723,7 @@ void main() {
expect(controller.value.isPlaying, isTrue);

await controller.pause();
await tester.runAsync(controller.dispose);
Copy link
Contributor Author

@ValentinVignal ValentinVignal Jan 7, 2025

Choose a reason for hiding this comment

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

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?

Copy link

@polina-c polina-c Jan 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tarrinneal tarrinneal Jan 21, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ValentinVignal ValentinVignal Jan 22, 2025

Choose a reason for hiding this comment

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

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

});

testWidgets('does not restart when dragging to end',
Expand All @@ -723,6 +749,7 @@ void main() {

expect(controller.value.position, controller.value.duration);
expect(controller.value.isPlaying, isFalse);
await tester.runAsync(controller.dispose);
});
});

Expand All @@ -733,6 +760,7 @@ void main() {
_localhostUri,
closedCaptionFile: _loadClosedCaption(),
);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.value.position, Duration.zero);
Expand Down Expand Up @@ -766,6 +794,7 @@ void main() {
_localhostUri,
closedCaptionFile: _loadClosedCaption(),
);
addTearDown(controller.dispose);

await controller.initialize();
controller.setCaptionOffset(const Duration(milliseconds: 100));
Expand Down Expand Up @@ -803,6 +832,7 @@ void main() {
_localhostUri,
closedCaptionFile: _loadClosedCaption(),
);
addTearDown(controller.dispose);

await controller.initialize();
controller.setCaptionOffset(const Duration(milliseconds: -100));
Expand Down Expand Up @@ -842,6 +872,7 @@ void main() {
VideoPlayerController.networkUrl(
_localhostUri,
);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.closedCaptionFile, null);
Expand All @@ -859,6 +890,7 @@ void main() {
_localhostUri,
closedCaptionFile: _loadClosedCaption(),
);
addTearDown(controller.dispose);

await controller.initialize();
expect(
Expand Down Expand Up @@ -893,6 +925,7 @@ void main() {

expect(controller.value.isPlaying, isFalse);
expect(controller.value.position, nonzeroDuration);
await tester.runAsync(controller.dispose);
});

testWidgets('playback status', (WidgetTester tester) async {
Expand All @@ -917,6 +950,7 @@ void main() {
));
await tester.pumpAndSettle();
expect(controller.value.isPlaying, isFalse);
await tester.runAsync(controller.dispose);
});

testWidgets('buffering status', (WidgetTester tester) async {
Expand Down Expand Up @@ -953,6 +987,7 @@ void main() {
.add(VideoEvent(eventType: VideoEventType.bufferingEnd));
await tester.pumpAndSettle();
expect(controller.value.isBuffering, isFalse);
await tester.runAsync(controller.dispose);
});
});
});
Expand Down Expand Up @@ -1155,6 +1190,7 @@ void main() {
_localhostUri,
videoPlayerOptions: VideoPlayerOptions(mixWithOthers: true),
);
addTearDown(controller.dispose);

await controller.initialize();
expect(controller.videoPlayerOptions!.mixWithOthers, true);
Expand All @@ -1167,6 +1203,7 @@ void main() {
allowBackgroundPlayback: true,
),
);
addTearDown(controller.dispose);

await controller.initialize();
await controller.play();
Expand All @@ -1181,6 +1218,7 @@ void main() {
_localhostUri,
videoPlayerOptions: VideoPlayerOptions(),
);
addTearDown(controller.dispose);

await controller.initialize();
await controller.play();
Expand Down Expand Up @@ -1211,6 +1249,7 @@ void main() {
_localhostUri,
videoPlayerOptions: VideoPlayerOptions(),
);
addTearDown(controller.dispose);

await controller.initialize();

Expand Down Expand Up @@ -1238,6 +1277,7 @@ void main() {
_localhostUri,
videoPlayerOptions: VideoPlayerOptions(),
);
addTearDown(controller.dispose);

await controller.initialize();

Expand Down Expand Up @@ -1275,6 +1315,7 @@ void main() {
_localhostUri,
videoPlayerOptions: VideoPlayerOptions(),
);
addTearDown(controller.dispose);

await controller.initialize();

Expand Down
Loading