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.

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

[video_player] Activate leak testing #8379

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ValentinVignal
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

cc @polina-c

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

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

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

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.

@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Jan 7, 2025
@ValentinVignal ValentinVignal force-pushed the video-player/activate-leak-testing branch from b1e7de6 to 19261d2 Compare January 15, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker p: video_player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants