Enable testWidgets to track leaks. by polina-c · Pull Request #124435 · flutter/flutter

polina-c · 2023-04-08T02:31:42Z

…Widgets

goderbauer · 2023-04-12T16:43:25Z

 // ignore: deprecated_member_use
 import 'package:test_api/test_api.dart' as test_package;

+import '_leak_tracking.dart';


why the leading underscore?

Because this is internal library, not exposed in API.

Everything under lib/src is private unless it's exported by something in lib

Some repos use leading underscore to make visibility level visible without checking if the file is exported.

And I see such files exist in this repo too:

Did I misinterpret something?

Our general policy is to export everything. Those underscore libs there are a somewhat special case, they contain the platform-specific implementation (see the io vs web suffix) for certain features and are used with conditional imports. The library that imports these files conditionally do exports the full API.

If you want to keep this private, why not just add it as a private method to the file where it is used?

Reasoning to have code separately can be size of the library widget_tester.dart. We want to keep it reasonable and collect everything related to leak tracking, separately.

Merged 'leak_tracking.dart' and '_leak_tracking.dart'

goderbauer · 2023-04-12T17:07:03Z

+/// The Flutter related enhancements are:
+/// 1. Listens to [MemoryAllocations] events.
+/// 2. Uses `asyncCodeRunner` for async call for leak detection.
+Future<void> withFlutterLeakTracking(


Isn't this method duplicated from elsewhere in the framework now? Can we remove it from there.

Yes, this file will be removed in next PR. Added TODO to that method under flutter/test/foundation.

Can we just fix this up in this PR?

I would love to do it, but I think it is not possible, because the usage of the new method is in different package. So, it needs the method to be checked in, in order to use it.

goderbauer · 2023-04-12T17:23:30Z

One disadvantage of adding this functionality to the testing framework is that we are going to lock people into a particular version of the leak_tracking package. If the leak tracking package evolves, people will not be able to benefit from it until we have upgraded the leak tracking version in Flutter and developers have updated to that new version of Flutter.

This burden is not to be underestimated. Some people have regretted that their package is actually that closely tied to Flutter releases.

cc @dnfield

dnfield · 2023-04-12T17:35:37Z

  vector_math: 2.1.4

+  # Used by testWidgets.
+  leak_tracker: 2.0.1


This is what will become a bit challenging that @goderbauer is referring to.

One option would be to start with this as a dev_dependency until it's been vetted a bit more with the framework tests.

At the same time, we should have a good cycle of time to let this get ready before another stable...

Is it possible to reference something that is imported in lib/src with dev_dependency?

According to this doc, no: https://dart.dev/tools/pub/dependencies

To evaluate this with the framework tests we'd ideally keep it as a dev_dependency on the framework. Not super sure how we can do that with how things are currently setup, though. May need some extra thought.

polina-c · 2023-04-13T18:07:45Z

One disadvantage of adding this functionality to the testing framework is that we are going to lock people into a particular version of the leak_tracking package. If the leak tracking package evolves, people will not be able to benefit from it until we have upgraded the leak tracking version in Flutter and developers have updated to that new version of Flutter.

This burden is not to be underestimated. Some people have regretted that their package is actually that closely tied to Flutter releases.

cc @dnfield

In future, if it starts hurting, we will copy minimal subset of the leak_tracker code to the flutter framework and remove the dependency.

Yes, this means one time the users will need to shift to new version of flutter that does not contain the dependency, to use leak tracker.

…Widgets

kenzieschmoll · 2023-04-17T14:55:21Z

+  /// When to collect stack trace information.
+  ///
+  /// You may need to know call stack to troubleshoot memory leaks.
+  /// Custonize this parameter to collect stack traces when needed.


sp: Customize

kenzieschmoll · 2023-04-17T15:36:41Z

+@Deprecated(_nonBackwardCompatibleApiWarning)
+class LeakTrackingFlutterTestConfig {
+  /// Creates a new instance of [LeakTrackingFlutterTestConfig].
+  @Deprecated(_nonBackwardCompatibleApiWarning)


It feels odd to check in deprecated code intentionally. @goderbauer @dnfield is there anywhere else in the framework where we do this / is this an acceptable pattern? I know in other places we have _kDebug* flags that can be flipped manually or via a service extension.

I would worry that users would ignore the deprecation warnings and check in usages of LeakTrackingFlutterTestConfig to their repos anyway.

This is not a pattern we have elsewhere and we shouldn't establish it. I am afraid, this is just teaching people to ignore deprecation warnings, which is not in our interest. It is also not in line with our breaking change policy: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

General practice would be to evolve the API as a package and then pull it into the framework when it is reasonably stable.

goderbauer · 2023-04-17T17:21:49Z


 typedef LeaksObtainer = void Function(Leaks foundLeaks);

+// TODO(polina-c): remove this file once https://github.com/flutter/flutter/pull/124435


Why not just do this now?

goderbauer · 2023-04-17T17:23:10Z

 // ignore: deprecated_member_use
 import 'package:test_api/test_api.dart' as test_package;

+import '_leak_tracking.dart';


If you want to keep this private, why not just add it as a private method to the file where it is used?

goderbauer · 2023-04-17T17:26:12Z

+@Deprecated(_nonBackwardCompatibleApiWarning)
+class LeakTrackingFlutterTestConfig {
+  /// Creates a new instance of [LeakTrackingFlutterTestConfig].
+  @Deprecated(_nonBackwardCompatibleApiWarning)


This is not a pattern we have elsewhere and we shouldn't establish it. I am afraid, this is just teaching people to ignore deprecation warnings, which is not in our interest. It is also not in line with our breaking change policy: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

goderbauer · 2023-04-17T17:27:24Z

+@Deprecated(_nonBackwardCompatibleApiWarning)
+class LeakTrackingFlutterTestConfig {
+  /// Creates a new instance of [LeakTrackingFlutterTestConfig].
+  @Deprecated(_nonBackwardCompatibleApiWarning)


General practice would be to evolve the API as a package and then pull it into the framework when it is reasonably stable.

goderbauer · 2023-04-17T17:29:15Z

+
+class StatelessLeakingWidget extends StatelessWidget {
+  StatelessLeakingWidget({super.key}) {
+    // ignore: unused_local_variable


https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#comment-all-ignores

goderbauer · 2023-04-17T17:30:34Z

This could just be part of widget_tester_test.dart?

goderbauer · 2023-04-17T17:33:18Z

  vector_math: 2.1.4

+  # Used by testWidgets.
+  leak_tracker: 2.0.1


To evaluate this with the framework tests we'd ideally keep it as a dev_dependency on the framework. Not super sure how we can do that with how things are currently setup, though. May need some extra thought.

flutter-dashboard · 2023-04-18T17:24:11Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

polina-c added 7 commits April 6, 2023 15:40

Create settings.json

cba5470

-

5ecbfa9

-

deb1f8b

-

45c0904

Merge branch 'master' of https://github.com/flutter/flutter into test…

03d9334

…Widgets

-

5d969f7

-

ee7d4c1

polina-c mentioned this pull request Apr 8, 2023

leak-tracker: Detect memory leaks by identifying not disposed or not GCed objects. flutter/devtools#3951

Closed

polina-c added 4 commits April 11, 2023 13:18

-

8c1a859

-

efacb10

-

eb25dbe

Update _leak_tracking.dart

4be7966

goderbauer reviewed Apr 12, 2023

View reviewed changes

dnfield reviewed Apr 12, 2023

View reviewed changes

polina-c added 2 commits April 13, 2023 10:46

Update leak_tracking_utils.dart

92ceff7

-

1ac7508

polina-c added 8 commits April 13, 2023 13:41

-

6e92e52

Update widget_tester_test.dart

3352987

Merge branch 'master' of https://github.com/flutter/flutter into test…

11b7468

…Widgets

Update _leak_tracking.dart

99ca9a5

Update widget_tester.dart

ac2b13c

Update leak_tracking.dart

f713542

Update leak_tracking.dart

84821d8

Update leak_tracking.dart

ee240f6

polina-c marked this pull request as ready for review April 13, 2023 22:57

polina-c added 2 commits April 13, 2023 15:57

Merge branch 'master' of https://github.com/flutter/flutter into test…

f01dddc

…Widgets

Merge branch 'master' of https://github.com/flutter/flutter into test…

41809f1

…Widgets

kenzieschmoll reviewed Apr 17, 2023

View reviewed changes

goderbauer reviewed Apr 17, 2023

View reviewed changes

polina-c mentioned this pull request Apr 18, 2023

Test config and flutter lints. dart-lang/leak_tracker#46

Merged

polina-c marked this pull request as draft April 18, 2023 17:18

-

69c04e9

polina-c closed this Jun 6, 2023


		typedef LeaksObtainer = void Function(Leaks foundLeaks);

		// TODO(polina-c): remove this file once https://github.com/flutter/flutter/pull/124435

Conversation

polina-c commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard Bot commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

polina-c commented Apr 8, 2023 •

edited

Loading

polina-c Apr 13, 2023 •

edited

Loading

goderbauer commented Apr 12, 2023 •

edited

Loading

polina-c commented Apr 13, 2023 •

edited

Loading