packages
packages copied to clipboard
flutter
[go_router] Use `leak_tracker_flutter_testing`
Activate leak_tracker_flutter_testing in the tests to detect memory leak issues.
cc @polina-c
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [ ] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [x] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required.
So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.
But leak_tracker_flutter_testing ^2.0.1 is the version available on stabe. @polina-c how can I make it work?
Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required. So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.But
leak_tracker_flutter_testing ^2.0.1is the version available on stabe. @polina-c how can I make it work?
Will investigate. Thank you. Converted the PR to draft for now.
Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required. So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.But
leak_tracker_flutter_testing ^2.0.1is the version available on stabe. @polina-c how can I make it work?Will investigate. Thank you. Converted the PR to draft for now.
Okay, it should be leak_tracker_flutter_testing: any in pubspec.yaml to pick up the version of leak tracker from sdk. Thanks for raising this. I will update documentation.
I see tests are failing. And they are failing because of leaks. And many of them are originated by flutter itself.
For example, leaking "_NotAnnounced" was cleaned up this week in Flutter master.
It seems packages that use stable should not be early adopters of leak tracker. So, let's shelve this PR till leak_tracker is released and recommended for stable.
Thank you for the experiment. It helped:
- To understand go_router has leaks.
- To document that version should be
any.
Converting this to draft.
@chunhtai what do you want me to do? Maybe I can remove leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart but keep the changes to prepare for when leak_tracker_flutter_testing can be used ?
@chunhtai what do you want me to do? Maybe I can remove
leak_tracker_flutter_testingfrom thepubspec.yamland revert theflutter_test_config.dartbut keep the changes to prepare for whenleak_tracker_flutter_testingcan be used ?
Yes, I like it: to merge fixes, while postponing regression testing. @chunhtai, how does it sound for you?
@chunhtai what do you want me to do? Maybe I can remove
leak_tracker_flutter_testingfrom thepubspec.yamland revert theflutter_test_config.dartbut keep the changes to prepare for whenleak_tracker_flutter_testingcan be used ?Yes, I like it: to merge fixes, while postponing regression testing. @chunhtai, how does it sound for you?
chunhtai is unavailable at the moment I think we can proceed with merging the change.
I removed the leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart in test: Disable leak tracking
'Linux analyze_legacy N-2' is failing because HeroController did not have method dispose yet.
@johnpryan, is this legacy job fixed forever or we can upgrade it for newer versions of flutter?
'Linux analyze_legacy N-2' is failing because HeroController did not have method dispose yet.
@johnpryan, is this legacy job fixed forever or we can upgrade it for newer versions of flutter?
More information: https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md
@ValentinVignal, invoked dispose does not exist for this job, that is on Flutter 2.0.6. What is minimal version of Flutter that has this method? We either need to increase oldest supported version of flutter, or not to add this missing dispose (that may cause memory leaks)
@stuartmorgan, are you right person to help here? Is it ok to stop supporting 2.0.6 and increase minimal supported version a little?
@stuartmorgan, are you right person to help here? Is it ok to stop supporting 2.0.6 and increase minimal supported version a little?
The earliest version of Flutter that any package in this repo is allowed to support (enforced by CI) is 3.13, so I don't really understand the question.
If N-2 is failing, then the PR doesn't work in 3.13.9. It's fine to drop 3.13 per the wiki, but that's unrelated to 2.0.6.
Thanks.
I took 2.0.6 from https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md, but now i see it is just very first supported version.
I'm a bit unsure of what I should do, should I stop using HeroController.dispose or should I make a change to the pipeline?
I took 2.0.6 from https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md, but now i see it is just very first supported version.
That's actually unrelated to analyze_legacy; it's for the legacy native build step in android_build_all_packages.
I'm a bit unsure of what I should do, should I stop using
HeroController.disposeor should I make a change to the pipeline?
To choose between these two options we need to find out what pipeline change is needed. Then we will decide if this change makes sense or if it is better not to use dispose for now and instead create tech debt issue and leave TODO to dispose when all supported versions of flutter have the dispose. Does it help?
@polina-c After looking at it, it is because HeroControlle.dispose was released in flutter 3.16.x
- https://github.com/flutter/flutter/pull/133951
- https://docs.flutter.dev/release/release-notes/release-notes-3.16.0
So I had to increase the minimum flutter version of flutter to
^3.16.0
Is that okay? Or is it too soon?