From 4e8014bf76c14759b35353565075ab55d263c268 Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Tue, 11 Jul 2023 17:41:14 -0700 Subject: [PATCH] Enable not GCed leak tracking. (#130159) --- .../test/foundation/leak_tracking.dart | 10 +---- .../test/foundation/leak_tracking_test.dart | 39 ++++++++++++------- .../flutter/test/material/about_test.dart | 20 +++++++++- .../test/widgets/widget_inspector_test.dart | 29 ++------------ 4 files changed, 49 insertions(+), 49 deletions(-) diff --git a/packages/flutter/test/foundation/leak_tracking.dart b/packages/flutter/test/foundation/leak_tracking.dart index 26a3dbd7a0..37b169cda0 100644 --- a/packages/flutter/test/foundation/leak_tracking.dart +++ b/packages/flutter/test/foundation/leak_tracking.dart @@ -54,13 +54,13 @@ void testWidgetsWithLeakTracking( bool semanticsEnabled = true, TestVariant variant = const DefaultTestVariant(), dynamic tags, - LeakTrackingTestConfig leakTrackingConfig = const LeakTrackingTestConfig(), + LeakTrackingTestConfig leakTrackingTestConfig = const LeakTrackingTestConfig(), }) { Future wrappedCallback(WidgetTester tester) async { await _withFlutterLeakTracking( () async => callback(tester), tester, - leakTrackingConfig, + leakTrackingTestConfig, ); } @@ -169,12 +169,6 @@ class LeakCleaner { /// Returns true if [leak] should be reported as failure. bool _shouldReportLeak(LeakType leakType, LeakReport leak, Map<(String, LeakType), int> countByClassAndType) { - // Tracking for non-GCed is temporarily disabled. - // TODO(polina-c): turn on tracking for non-GCed after investigating existing leaks. - if (leakType != LeakType.notDisposed) { - return false; - } - final String leakingClass = leak.type; final (String, LeakType) classAndType = (leakingClass, leakType); diff --git a/packages/flutter/test/foundation/leak_tracking_test.dart b/packages/flutter/test/foundation/leak_tracking_test.dart index fc71814d26..e9a66480c0 100644 --- a/packages/flutter/test/foundation/leak_tracking_test.dart +++ b/packages/flutter/test/foundation/leak_tracking_test.dart @@ -18,7 +18,7 @@ Leaks _leaksOfAllTypes() => Leaks(> { }); Future main() async { - test('Trivial $LeakCleaner returns only non-disposed leaks.', () { + test('Trivial $LeakCleaner returns all leaks.', () { final LeakCleaner leakCleaner = LeakCleaner(const LeakTrackingTestConfig()); final Leaks leaks = _leaksOfAllTypes(); final int leakTotal = leaks.total; @@ -26,7 +26,7 @@ Future main() async { final Leaks cleanedLeaks = leakCleaner.clean(leaks); expect(leaks.total, leakTotal); - expect(cleanedLeaks.total, 1); + expect(cleanedLeaks.total, 3); }); test('$LeakCleaner catches extra leaks', () { @@ -48,7 +48,7 @@ Future main() async { (WidgetTester tester) async { await tester.pumpWidget(_StatelessLeakingWidget()); }, - leakTrackingConfig: LeakTrackingTestConfig( + leakTrackingTestConfig: LeakTrackingTestConfig( notDisposedAllowList: {_leakTrackedClassName: null}, notGCedAllowList: {_leakTrackedClassName: null}, ), @@ -59,7 +59,7 @@ Future main() async { (WidgetTester tester) async { await tester.pumpWidget(_StatelessLeakingWidget()); }, - leakTrackingConfig: LeakTrackingTestConfig( + leakTrackingTestConfig: LeakTrackingTestConfig( notDisposedAllowList: {_leakTrackedClassName: 1}, notGCedAllowList: {_leakTrackedClassName: 1}, ), @@ -76,7 +76,7 @@ Future main() async { await tester.pumpWidget(_StatelessLeakingWidget()); await tester.pumpWidget(_StatelessLeakingWidget()); }, - leakTrackingConfig: LeakTrackingTestConfig( + leakTrackingTestConfig: LeakTrackingTestConfig( onLeaks: (Leaks theLeaks) { leaks = theLeaks; }, @@ -85,7 +85,7 @@ Future main() async { ), ); - tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 2)); + tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 2, expectedNotGCed: 2, shouldContainDebugInfo: false)); }); group('respects notGCed allow lists', () { @@ -98,7 +98,7 @@ Future main() async { (WidgetTester tester) async { await tester.pumpWidget(_StatelessLeakingWidget()); }, - leakTrackingConfig: LeakTrackingTestConfig( + leakTrackingTestConfig: LeakTrackingTestConfig( onLeaks: (Leaks theLeaks) { leaks = theLeaks; }, @@ -107,7 +107,7 @@ Future main() async { ), ); - tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 1)); + tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 1, shouldContainDebugInfo: false)); }); group('catches that', () { @@ -120,7 +120,7 @@ Future main() async { (WidgetTester tester) async { await tester.pumpWidget(_StatelessLeakingWidget()); }, - leakTrackingConfig: LeakTrackingTestConfig( + leakTrackingTestConfig: LeakTrackingTestConfig( onLeaks: (Leaks theLeaks) { leaks = theLeaks; }, @@ -128,7 +128,7 @@ Future main() async { ), ); - tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 1)); + tearDown(() => _verifyLeaks(leaks, expectedNotDisposed: 1, expectedNotGCed: 1, shouldContainDebugInfo: false)); }); }, skip: isBrowser); // [intended] Leak detection is off for web. @@ -140,7 +140,12 @@ Future main() async { } /// Verifies [leaks] contains expected number of leaks for [_LeakTrackedClass]. -void _verifyLeaks(Leaks leaks, { int expectedNotDisposed = 0, int expectedNotGCed = 0 }) { +void _verifyLeaks( + Leaks leaks, { + int expectedNotDisposed = 0, + int expectedNotGCed = 0, + required bool shouldContainDebugInfo, +}) { const String linkToLeakTracker = 'https://github.com/dart-lang/leak_tracker'; expect( @@ -152,14 +157,20 @@ void _verifyLeaks(Leaks leaks, { int expectedNotDisposed = 0, int expectedNotGC ), ); - _verifyLeakList(leaks.notDisposed, expectedNotDisposed); - _verifyLeakList(leaks.notGCed, expectedNotGCed); + _verifyLeakList(leaks.notDisposed, expectedNotDisposed, shouldContainDebugInfo); + _verifyLeakList(leaks.notGCed, expectedNotGCed, shouldContainDebugInfo); } -void _verifyLeakList(List list, int expectedCount){ +void _verifyLeakList(List list, int expectedCount, bool shouldContainDebugInfo){ expect(list.length, expectedCount); for (final LeakReport leak in list) { + if (shouldContainDebugInfo) { + expect(leak.context, isNotEmpty); + } else { + expect(leak.context ?? {}, isEmpty); + } + expect(leak.trackedClass, contains(_LeakTrackedClass.library)); expect(leak.trackedClass, contains(_leakTrackedClassName)); } diff --git a/packages/flutter/test/material/about_test.dart b/packages/flutter/test/material/about_test.dart index 29cfdf5a37..3c15de4ebb 100644 --- a/packages/flutter/test/material/about_test.dart +++ b/packages/flutter/test/material/about_test.dart @@ -201,7 +201,15 @@ void main() { await tester.tap(find.text('Another package')); await tester.pumpAndSettle(); expect(find.text('Another license'), findsOneWidget); - }); + }, + leakTrackingTestConfig: const LeakTrackingTestConfig( + // TODO(polina-c): remove after fixing + // https://github.com/flutter/flutter/issues/130354 + notGCedAllowList: { + 'ValueNotifier<_OverlayEntryWidgetState?>':2, + 'ValueNotifier': 1, + }, + )); testWidgetsWithLeakTracking('LicensePage control test with all properties', (WidgetTester tester) async { const FlutterLogo logo = FlutterLogo(); @@ -278,7 +286,15 @@ void main() { await tester.tap(find.text('Another package')); await tester.pumpAndSettle(); expect(find.text('Another license'), findsOneWidget); - }); + }, + leakTrackingTestConfig: const LeakTrackingTestConfig( + // TODO(polina-c): remove after fixing + // https://github.com/flutter/flutter/issues/130354 + notGCedAllowList: { + 'ValueNotifier<_OverlayEntryWidgetState?>':2, + 'ValueNotifier': 1, + }, + )); testWidgetsWithLeakTracking('_PackageLicensePage title style without AppBarTheme', (WidgetTester tester) async { LicenseRegistry.addLicense(() { diff --git a/packages/flutter/test/widgets/widget_inspector_test.dart b/packages/flutter/test/widgets/widget_inspector_test.dart index 9f18ec1630..a75293e832 100644 --- a/packages/flutter/test/widgets/widget_inspector_test.dart +++ b/packages/flutter/test/widgets/widget_inspector_test.dart @@ -10,7 +10,6 @@ library; import 'dart:convert'; -import 'dart:developer'; import 'dart:math'; import 'dart:ui' as ui; @@ -20,6 +19,7 @@ import 'package:flutter/gestures.dart' show DragStartBehavior; import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:leak_tracker/leak_tracker.dart'; import 'widget_inspector_test_utils.dart'; @@ -241,29 +241,6 @@ extension TextFromString on String { } } -/// Forces garbage collection by aggressive memory allocation. -/// -/// Copied from internal code of -/// https://github.com/dart-lang/leak_tracker -Future _forceGC() async { - const int gcCycles = 3; // 1 should be enough, but we do 3 to make sure test is not flaky. - final int barrier = reachabilityBarrier; - - final List> storage = >[]; - - void allocateMemory() { - storage.add(Iterable.generate(10000, (_) => DateTime.now()).toList()); - if (storage.length > 100) { - storage.removeAt(0); - } - } - - while (reachabilityBarrier < barrier + gcCycles) { - await Future.delayed(Duration.zero); - allocateMemory(); - } -} - final List _weakValueTests = [1, 1.0, 'hello', true, false, Object(), [3, 4], DateTime(2023)]; void main() { @@ -331,7 +308,9 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService { final WeakReference ref = WeakReference(someObject); someObject = null; - await _forceGC(); + + // 1 should be enough for [fullGcCycles], but it is 3 to make sure tests are not flaky. + await forceGC(fullGcCycles: 3); expect(ref.target, null); });