Enable misc leak tracking #176992
Enable misc leak tracking #176992
Conversation
github-actions
bot
added
a: tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new CI target, Windows framework_tests_misc_leak_tracking, to enable leak tracking for the misc subshard. It also adds a flutter_test_config.dart file, presumably to configure leak tracking for tests related to WidgetTester. My review focuses on the correctness of the CI configuration and the new test configuration file. I've identified a critical issue with the location of the new config file which will prevent it from being used, and a couple of medium-severity issues related to maintainability in both the CI config and the Dart code.
| @@ -0,0 +1,44 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
This flutter_test_config.dart file appears to be in the wrong location. For the test runner to pick it up, it must be placed inside a directory that contains tests. For the flutter_test package, this would be packages/flutter_test/test/. In its current location at packages/flutter_test/, it will not be discovered and will have no effect.
| bool _isLeakTrackingEnabled() { | ||
| if (kIsWeb) { | ||
| return false; | ||
| } | ||
| // The values can be different, one is compile time, another is run time. | ||
| return const bool.fromEnvironment('LEAK_TRACKING') || | ||
| (bool.tryParse(Platform.environment['LEAK_TRACKING'] ?? '') ?? false); | ||
| } |
There was a problem hiding this comment.
This _isLeakTrackingEnabled function is a duplicate of the one in packages/flutter/test/flutter_test_config.dart. To improve maintainability and avoid code duplication, consider extracting this logic into a shared utility function that both configuration files can use.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hey @dkwingsmt is this change still on your radar? |
|
I still want to finish this and am hoping to discuss with @/jtmcdole , who has been busy towards the end of the year. I'll keep following up. |
3 participants
This PR adds a new subshard that runs leak tracking tests on flutter/misc.
This is used to address a problem faced by PR #176519, which fixes a leak in
WidgetTesterbut is unable to write a unit test.I'm not certain this is the best approach so I'm open to other suggestions.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.