Skip to content
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

Fix flakiness in detection of notDisposed leaks in tests. #149

Merged
merged 10 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ class ObjectRecordSet {
return list.firstWhereOrNull((r) => r.ref.target == object);
}

/// Removes record if it exists in the set.
void remove(ObjectRecord record) {
final list = _records[record.code]!;
final list = _records[record.code];
if (list == null) return;
bool removed = false;
list.removeWhere((r) {
if (r == record) {
Expand All @@ -42,7 +44,6 @@ class ObjectRecordSet {
}
return r == record;
});
assert(removed);
_length--;
if (list.isEmpty) _records.remove(record.code);
}
Expand Down
16 changes: 16 additions & 0 deletions pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ class ObjectTracker implements LeakProvider {
_objects.assertRecordIntegrity(record);
record.setGCed(_gcCounter.gcCount, clock.now());

_declareNotDisposedLeak(record);
}

void _declareNotDisposedLeak(ObjectRecord record) {
if (record.isGCedLateLeak(disposalTime, numberOfGcCycles)) {
_objects.gcedLateLeaks.add(record);
} else if (record.isNotDisposedLeak) {
Expand All @@ -98,6 +102,18 @@ class ObjectTracker implements LeakProvider {
_objects.assertRecordIntegrity(record);
}

/// Declares all not disposed objects as leaks, even if they are not GCed yet.
///
/// Is used to make sure all disposables are disposed by the the end of the test.
void declareAllNotDisposedAsLeaks() {
throwIfDisposed();
final notGCedAndNotDisposed = <ObjectRecord>[];
_objects.notGCed.forEach((record) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: lots of projects have lints recommending that we don't use forEach. Even if we don't have it enabled for this project, consider doing this instead:

for (final record in _objects.notGCed) {
    if (!record.isDisposed) {
      _declareNotDisposedLeak(record);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lint is enabled for this project. notGCed is not iterable though. I will check if it is easy to make it iterable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting. At the very least, I don't think you need noGCedAndNotDisposed, but you could instead do:

_objects.notGCed.forEach((record) {
  if (!record.isDisposed) {
    _declareNotDisposedLeak(record);
  }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks as optimizable. Added comment why I did it:

// We need this temporary storage to avoid errror 'concurrent modification during iteration'
// for internal iterables in `_objects.notGCed`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I checked: Iterable wants 27 methods to be implemented. Seems to be too heavy.

if (!record.isDisposed) notGCedAndNotDisposed.add(record);
});
notGCedAndNotDisposed.forEach(_declareNotDisposedLeak);
}

void dispatchDisposal(
Object object, {
required Map<String, dynamic>? context,
Expand Down
8 changes: 8 additions & 0 deletions pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,12 @@ abstract class LeakTracking {

await (result ?? Future.value());
}

/// Declares all not disposed objects as leaks.
///
/// Should be invoked after test execution, to detect
/// not disposed objects, even if they are not GCed yet.
static void declareNotDisposedObjectsAsLeaks() {
_leakTracker?.objectTracker.declareAllNotDisposedAsLeaks();
}
}
1 change: 1 addition & 0 deletions pkgs/leak_tracker_flutter_testing/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# 1.0.5

* If an object is not disposed by the end of testing, mark it as notDisposed.
* Bump version of SDK to 3.1.2.

# 1.0.4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ void configureLeakTrackingTearDown({
Future<void> _tearDownTestingWithLeakTracking(LeaksCallback? onLeaks) async {
if (!LeakTracking.isStarted) return;
if (!_isPlatformSupported) return;

MemoryAllocations.instance.removeListener(_flutterEventToLeakTracker);

LeakTracking.declareNotDisposedObjectsAsLeaks();
await forceGC(fullGcCycles: _leakTrackingTestSettings.numberOfGcCycles);
// This delay is needed to make sure all disposed and not GCed object are
// declared as leaks, and thus there is no flakiness in tests.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:flutter/material.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

late String twoControllers;

/// Verification for the tests happens in flutter_test_config.dart.
void main() {
testWidgetsWithLeakTracking(
twoControllers = 'Two not disposed controllers results in two leaks.',
(tester) async {
// ignore: unused_local_variable
final TextEditingController controller = TextEditingController();

// ignore: unused_local_variable
final FocusNode focusNode = FocusNode(debugLabel: 'Test Node');
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';

import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker/leak_tracker.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

import 'failure_test.dart';

/// The test configuration for each test library in this directory.
///
/// See https://api.flutter.dev/flutter/flutter_test/flutter_test-library.html.
Future<void> testExecutable(FutureOr<void> Function() testMain) async {
Leaks? leaks;

// This tear down should be set before leak tracking tear down in
// order to happen after it and verify that leaks are found.
tearDownAll(() async {
final theLeaks = leaks;
if (theLeaks == null) throw 'leaks should be detected';

expect(
theLeaks.notDisposed.where((l) => l.phase == twoControllers),
hasLength(2),
);
});

configureLeakTrackingTearDown(
configureOnce: true,
onLeaks: (l) => leaks = l,
);

setUpAll(() {
LeakTracking.warnForUnsupportedPlatforms = false;
});

await testMain();
}