-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[go_router] Activate leak testing #7546
base: main
Are you sure you want to change the base?
[go_router] Activate leak testing #7546
Conversation
cc @polina-c |
I didn't modify the version number and the CHANGELOG, should I do it or is it not necessary for this? |
There was a problem hiding this 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
@@ -22,6 +22,7 @@ dev_dependencies: | |||
flutter_test: | |||
sdk: flutter | |||
io: ^1.0.4 | |||
leak_tracker_flutter_testing: any |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.)
FutureOr<void> testExecutable(FutureOr<void> Function() testMain) { | ||
LeakTesting.enable(); | ||
LeakTesting.settings = | ||
LeakTesting.settings.withIgnored(createdByTestHelpers: true); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Thanks for pushing it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -22,6 +22,7 @@ dev_dependencies: | |||
flutter_test: | |||
sdk: flutter | |||
io: ^1.0.4 | |||
leak_tracker_flutter_testing: any |
There was a problem hiding this comment.
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
override reason: test only. |
I am not sure what happen to the ci, can you try to rebase and see if that passes the ci. |
f4912f7
to
0e103fe
Compare
looks like the test was trying to run |
0e103fe
to
6595059
Compare
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. |
I updated the documentation in dart-lang/leak_tracker#245 |
Hm. |
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 |
589b82c
to
90ee2ca
Compare
Thank you for the insights. 90ee2ca made the ci pass |
@chunhtai I see you added the label "waiting for tree to go green"? Is there a bot that will automatically merge this PR once it is ready (like the flutter repository)? Or can I merge it by hand if all the checks are successful? |
I am not sure why it doesn't trigger auto merge, let me update the branch and see if it picks up this time |
5e8517f
to
4598267
Compare
4598267
to
63b1072
Compare
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.