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

[go_router] Activate leak testing #7546

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

[go_router] Activate leak testing #7546

wants to merge 6 commits into from
+21 −0

Conversation

ValentinVignal
Copy link
Contributor

Pre-launch Checklist

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

@ValentinVignal
Copy link
Contributor Author

cc @polina-c

@ValentinVignal
Copy link
Contributor Author

I didn't modify the version number and the CHANGELOG, should I do it or is it not necessary for this?

chunhtai
chunhtai reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

I don't think you need to bump version in this case

packages/go_router/pubspec.yaml
@@ -22,6 +22,7 @@ dev_dependencies:
flutter_test:
sdk: flutter
io: ^1.0.4
leak_tracker_flutter_testing: any
Copy link
Contributor

Choose a reason for hiding this comment

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

can you pin a version instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the documentation recommends not to pin any number: see https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

Do you still want me to pin one after reading this?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i wasn't aware that is the guideline, thanks for pointing it out. LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the pipeline Linux repo_checks is not happy with this. Should it be ignored (and how?) ?
Or is there something else we can do about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should ignore it given that is how leak tracking suggests.

I think you need to modify packages/script/configs/allowed_unpinned_deps.yaml

cc @stuartmorgan

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to modify packages/script/configs/allowed_unpinned_deps.yaml

It looks like the tooling doesn't check for any specifically, which it should, so that should work. (At some point I'll need to fix that and split this exception out into its own file, since in general we don't want to allow any.)

packages/go_router/test/flutter_test_config.dart Outdated
FutureOr<void> testExecutable(FutureOr<void> Function() testMain) {
LeakTesting.enable();
LeakTesting.settings =
LeakTesting.settings.withIgnored(createdByTestHelpers: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we are ignoring this?

Copy link
Contributor Author

@ValentinVignal ValentinVignal Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is what https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md suggests to do.
Maybe @polina-c would have more insights

Copy link

@polina-c polina-c Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If test helpers create a disposable and then do not dispose it, it is not a big deal as it does not affect production code.
This setting helps to setup leak tracking faster for existing projects where test helpers are leaking.
But, it is up to each project if this should be set.
Does it help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @polina-c

@chunhtai test: Don't ignore leaks created by test helpers removes the ignore

@polina-c
Copy link

Thanks for pushing it!
Looks good to me. Will let @chunhtai to stamp.

chunhtai
chunhtai approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai 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/go_router/pubspec.yaml
@@ -22,6 +22,7 @@ dev_dependencies:
flutter_test:
sdk: flutter
io: ^1.0.4
leak_tracker_flutter_testing: any
Copy link
Contributor

Choose a reason for hiding this comment

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

oh i wasn't aware that is the guideline, thanks for pointing it out. LGTM

@chunhtai chunhtai 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 Sep 5, 2024
@chunhtai
Copy link
Contributor

chunhtai commented Sep 5, 2024

override reason: test only.

@chunhtai
Copy link
Contributor

chunhtai commented Sep 5, 2024

I am not sure what happen to the ci, can you try to rebase and see if that passes the ci.

@chunhtai
Copy link
Contributor

chunhtai commented Sep 6, 2024

looks like the test was trying to run
flutter test --color --platform=chrome --web-renderer=canvaskit" in /b/s/w/ir/x/w/packages/packages/go_router

@ValentinVignal
Copy link
Contributor Author

It looks like the signature needs to be

Future<void> testExecutable(FutureOr<void> Function() testMain)

@polina-c do you want me to make a PR to update https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md ?

@polina-c
Copy link

polina-c commented Sep 9, 2024

It looks like the signature needs to be

Future<void> testExecutable(FutureOr<void> Function() testMain)

@polina-c do you want me to make a PR to update https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md ?

Hm, interesting.
Yes, please. Thank you!

@ValentinVignal ValentinVignal mentioned this pull request Sep 9, 2024
Open
1 task
@ValentinVignal
Copy link
Contributor Author

I updated the documentation in dart-lang/leak_tracker#245

@chunhtai
Copy link
Contributor

looks like here is a ci failure https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737284438128899377/+/u/Run_package_tests/pubspec_validation/stdout

@polina-c
Copy link

Hm.
Leak tracker is not recognized as local dependency, while it is part of Flutter.

@stuartmorgan
Copy link
Contributor

Leak tracker is not recognized as local dependency, while it is part of Flutter.

It isn't part of the Flutter SDK, otherwise the dependency would be an SDK dependency and the repo tooling wouldn't flag it. Local means "in this repository", which it also isn't. It's just a random package from pub as far as the repo tooling is concerned. (I'd be happy to take a PR that makes the check cross-reference by querying the pub.dev API for any package that's not SDK and not local, and allowing anything with a Dart or Flutter official publisher.)

Thus https://github.com/flutter/packages/pull/7546/files#r1757634961

@ValentinVignal
Copy link
Contributor Author

Thank you for the insights.

90ee2ca made the ci pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan stuartmorgan stuartmorgan left review comments

@chunhtai chunhtai chunhtai approved these changes

@polina-c polina-c Awaiting requested review from polina-c

Assignees
No one assigned
Labels
override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: go_router waiting for tree to go green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ValentinVignal @polina-c @chunhtai @stuartmorgan