Repository navigation
Define testWidgetsWithLeakTracking. - #125063
Conversation
flutter-dashboard
Bot
added
p: material_ui
| if (!_webWarningPrinted) { | ||
| _webWarningPrinted = true; | ||
| debugPrint('Leak tracking is not supported on web platform.'); | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
Made actionable.
| // Using [testWidgets] instead of [testWidgetsWithLeakTracking] because | ||
| // [tooltipState] is hold after disposal by the test | ||
| // and thus is not garbage collected. |
There was a problem hiding this comment.
This makes me question that turning on leak tracking by default is actually feasible. This is a fairly common pattern. If people are confronted with a non-actionable leak warnings every time they obtain a state in a test, they'll likely get frustrated with these warnings and stop paying attention to the legitimate ones.
There was a problem hiding this comment.
Good question.
Added allowlist for such leaks.
Having leak tracking on by default will force people to comment the allow list entries explaining what is going on, that I think, is good thing as it builds intuition about leak prone situations and shared meaning of what to expect from garbage collector.
There was a problem hiding this comment.
We talked about this a bit offline but unfortunately without @goderbauer - this test is a little strange in that it's poking at a disposed object after the object has been disposed.
I think a reasonable API would be to somehow add that object as "intentionally leaked", e.g. tester.ignoredLeakedObject(tooltipState). WDYT @goderbauer ?
There was a problem hiding this comment.
this test is a little strange in that it's poking at a disposed object after the object has been disposed.
For my own understanding: If you'd remove the last expect(tooltipState.ensureTooltipVisible(), false); from the test, the leak tracker would not complain because there is no interaction with a disposed object? Or would holding the disposed tooltipState in that variable still constitute a leak? I believe, doing the latter is somewhat common...
I think a reasonable API would be to somehow add that object as "intentionally leaked",
Again, I think to answer this question we need to know how common this situation is, i.e. how often would test authors get confused about a false leak warning? For that, I would check how often this situation happens in our current test code base...
There was a problem hiding this comment.
I collected some statistics. Out of 142 tests, there is just 1 that holds state after disposal.
Yes, it counts as a leak, because an object is expected to be GCed soon after disposal.
Looking forward to discuss details on a meeting.
flutter-dashboard
Bot
added
a: animation
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM after comments are addressed.
| /// | ||
| /// The objects are referenced by hash codes and can duplicate with low probability. | ||
| @visibleForTesting | ||
| class WeakSet { |
There was a problem hiding this comment.
Looks like this is also unused now and should have been removed?
3 participants
contributes to: flutter/devtools#3951