-
Notifications
You must be signed in to change notification settings - Fork 23
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
Ignore test helpers. #196
Ignore test helpers. #196
Conversation
pkgs/leak_tracker/lib/src/leak_tracking/primitives/_test_helper_detector.dart
Outdated
Show resolved
Hide resolved
const IgnoredLeaksSet.byClass(Map<String, int?> byClass) | ||
: this(byClass: byClass); |
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.
why not use this.byClass
directly like before?
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 do not want to duplicate default for this.ignoreAll
/// Is used to test functionality of | ||
/// the leak tracker. |
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.
is this really only used for testing? if so, should this be marked visibleForTesting?
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.
Hm...
Indeed.
I did not know I can use test only members in other packages too.
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.
Ups. It is used for read in production code of leak tracker. And it is complecsity to make just 'write' test-only.
pkgs/leak_tracker_flutter_testing/test/test_infra/test_helpers.dart
Outdated
Show resolved
Hide resolved
}); | ||
|
||
test('Test leak is ignored.', () async { | ||
createTestWidget(); |
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.
how do we ensure this case is not a false negative? If StatelessLeakingWidget
is really a leaking widget, I would think this case would expose a real leak.
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.
Yes, current implementation is not perfect and will miss some leaks, but it seem to create reasonable balance.
In real cases leaks are mostly created after construction, later, when widgets start functioning.
pkgs/leak_tracker/lib/src/leak_tracking/primitives/_test_helper_detector.dart
Outdated
Show resolved
Hide resolved
pkgs/leak_tracker_flutter_testing/test/tests/end_to_end/testing_test.dart
Outdated
Show resolved
Hide resolved
@@ -34,7 +34,7 @@ import 'matchers.dart'; | |||
@immutable | |||
class LeakTesting { | |||
const LeakTesting._({ | |||
this.ignore = true, | |||
this.ignore = false, |
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 we are now using LeakTesting.enabled to set whether leak tracking is enabled, do we even need this ignore
field anymore?
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.
Yes, for some tests we want to turn off leak tracking.
LeakTesting.enabled cannot be turned off because it indicates that leak tracking infrastructure is used for this process in general.
ignore means to ignore just for this test.
makes sense?
pkgs/leak_tracker/lib/src/leak_tracking/primitives/_test_helper_detector.dart
Outdated
Show resolved
Hide resolved
pkgs/leak_tracker_flutter_testing/test/test_infra/memory_leak_tests.dart
Outdated
Show resolved
Hide resolved
…, shelf, tools, webdev Revisions updated by `dart tools/rev_sdk_deps.dart`. async (https://github.com/dart-lang/async/compare/9924570..e83d054): e83d054 2024-01-08 Futuristic Goo Fix typo (dart-archive/async#262) ecosystem (https://github.com/dart-lang/ecosystem/compare/dc44e82..1e2785d): 1e2785d 2024-01-09 Jacob MacDonald fix saving of comment ids to disk (dart-lang/ecosystem#221) 244a28d 2024-01-09 Moritz Update publish.yaml (dart-lang/ecosystem#217) bab9833 2024-01-09 Moritz Fix health commenting (dart-lang/ecosystem#219) f87e6f4 2024-01-08 Moritz Update health workflow (dart-lang/ecosystem#216) a58c7d8 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/ecosystem#214) leak_tracker (https://github.com/dart-lang/leak_tracker/compare/3d4c0d6..4a5b077): 4a5b077 2024-01-09 Polina Cherkasova Enhance scripting. (dart-lang/leak_tracker#204) e7094f4 2024-01-08 Polina Cherkasova Ignore test helpers. (dart-lang/leak_tracker#196) 6591934 2024-01-03 Polina Cherkasova Handle deprecation in Flutter. (dart-lang/leak_tracker#203) markdown (https://github.com/dart-lang/markdown/compare/d2e7903..7fdfa55): 7fdfa55 2024-01-01 dependabot[bot] Bump actions/stale from 8.0.0 to 9.0.0 (dart-lang/markdown#571) 5fab3a7 2023-12-19 Alex Li ✨ Introduce AlertBlockSyntax (dart-lang/markdown#570) native (https://github.com/dart-lang/native/compare/0605d9a..14f6da1): 14f6da1d 2024-01-09 Simon Binder Support `@Native` fields and `addressOf` (dart-lang/native#860) protobuf (https://github.com/dart-lang/protobuf/compare/20ec685..a293fb9): a293fb9 2024-01-08 Ömer Sinan Ağacan Handle deprecated options in grpc services and methods, enum types and values, messages (google/protobuf.dart#908) 9a408a7 2024-01-08 Ömer Sinan Ağacan Generate docs of enums and rpc clients, some refactoring (google/protobuf.dart#909) c4fd596 2024-01-06 Ömer Sinan Ağacan Export GeneratedMessageGenericExtensions in generated files (google/protobuf.dart#907) shelf (https://github.com/dart-lang/shelf/compare/733588f..823966f): 823966f 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/shelf#403) tools (https://github.com/dart-lang/tools/compare/2f59ab4..8ffc077): 8ffc077 2024-01-03 Moritz Fix `labeler.yml` (dart-lang/tools#224) webdev (https://github.com/dart-lang/webdev/compare/b2405cb..c08a65c): c08a65c9 2024-01-09 Elliott Brooks Loosen `vm_service` constraints and prepare DWDS for release to 23.1.1 (dart-lang/webdev#2329) 651bdae6 2024-01-08 Derek Xu Make FrontendServerClient start the frontend server from AOT snapshot by default (dart-lang/webdev#2263) 4d1de266 2024-01-03 Elliott Brooks Prepare DWDS for release to version 23.1.0 (dart-lang/webdev#2328) Change-Id: I4d7fd994cc54ac2d72335c3ebf40710f3bd020e6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345366 Commit-Queue: Konstantin Shcheglov <[email protected]> Auto-Submit: Devon Carew <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
Leaks detected in test helpers create extra toil for users of leak_tracker.
This PR adds functionality to ignore disposables, created in test helpers.
It also adds API to test leak_tracker, to test
testWidgets
and other test engines that use the testing API.PR for Flutter that uses the created testing API: flutter/flutter#140553
Also this PR enables leak tracking by default in LeakTesting.settings, because it is disabled by default in LeakTesting.enabled.