Skip to content

[coverage] Fix remaining ~0.1% flakiness #2102

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

Merged
merged 8 commits into from
Jun 4, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 4 additions & 1 deletion pkgs/coverage/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
## 1.14.1-wip
## 1.14.1

- Remove dependency on `package:pubspec_parse`.
- Silence a rare error that can occur when trying to resume the main isolate
because the VM service has already shut down. This was responsible for a ~0.1%
flakiness, and is safe to ignore.

## 1.14.0

Expand Down
6 changes: 5 additions & 1 deletion pkgs/coverage/lib/src/isolate_paused_listener.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ class IsolatePausedListener {

// Resume the main isolate.
if (_mainIsolate != null) {
await _service.resume(_mainIsolate!.id!);
try {
await _service.resume(_mainIsolate!.id!);
} on RPCError {
// The VM Service has already shut down, so there's nothing left to do.
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkgs/coverage/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: coverage
version: 1.14.1-wip
version: 1.14.1
description: Coverage data manipulation and formatting
repository: https://github.com/dart-lang/tools/tree/main/pkgs/coverage
issue_tracker: https://github.com/dart-lang/tools/issues?q=is%3Aissue+is%3Aopen+label%3Apackage%3Acoverage
Expand Down
105 changes: 78 additions & 27 deletions pkgs/coverage/test/isolate_paused_listener_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,12 @@ void main() {
late MockVmService service;
late StreamController<Event> allEvents;
late Future<void> allIsolatesExited;
Object? lastError;

late List<String> received;
Future<void> Function(String)? delayTheOnPauseCallback;
late bool stopped;
late Set<String> resumeFailures;

void startEvent(String id, String groupId, [String? name]) =>
allEvents.add(event(
Expand Down Expand Up @@ -516,34 +518,48 @@ void main() {
}

setUp(() {
(service, allEvents) = createServiceAndEventStreams();

// Backfill was tested above, so this test does everything using events,
// for simplicity. No need to report any isolates.
when(service.getVM()).thenAnswer((_) async => VM());

received = <String>[];
delayTheOnPauseCallback = null;
when(service.resume(any)).thenAnswer((invocation) async {
final id = invocation.positionalArguments[0];
received.add('Resume $id');
return Success();
});

stopped = false;
allIsolatesExited = IsolatePausedListener(
service,
(iso, isLastIsolateInGroup) async {
expect(stopped, isFalse);
received.add('Pause ${iso.id}. Collect group ${iso.isolateGroupId}? '
'${isLastIsolateInGroup ? 'Yes' : 'No'}');
if (delayTheOnPauseCallback != null) {
await delayTheOnPauseCallback!(iso.id!);
received.add('Pause done ${iso.id}');
Zone.current.fork(
specification: ZoneSpecification(
handleUncaughtError: (Zone self, ZoneDelegate parent, Zone zone,
Object error, StackTrace stackTrace) {
lastError = error;
},
),
).runGuarded(() {
(service, allEvents) = createServiceAndEventStreams();

// Backfill was tested above, so this test does everything using events,
// for simplicity. No need to report any isolates.
when(service.getVM()).thenAnswer((_) async => VM());

received = <String>[];
delayTheOnPauseCallback = null;
resumeFailures = <String>{};
when(service.resume(any)).thenAnswer((invocation) async {
final id = invocation.positionalArguments[0];
received.add('Resume $id');
if (resumeFailures.contains(id)) {
throw RPCError('resume', -32000, id);
}
},
(message) => received.add(message),
).waitUntilAllExited();
return Success();
});

stopped = false;
allIsolatesExited = IsolatePausedListener(
service,
(iso, isLastIsolateInGroup) async {
expect(stopped, isFalse);
received
.add('Pause ${iso.id}. Collect group ${iso.isolateGroupId}? '
'${isLastIsolateInGroup ? 'Yes' : 'No'}');
if (delayTheOnPauseCallback != null) {
await delayTheOnPauseCallback!(iso.id!);
received.add('Pause done ${iso.id}');
}
},
(message) => received.add(message),
).waitUntilAllExited();
});
});

test('ordinary flows', () async {
Expand Down Expand Up @@ -889,5 +905,40 @@ void main() {
// Don't try to resume B, because the VM service is already shut down.
]);
});

test('throw when resuming main isolate is ignored', () async {
resumeFailures = {'main'};

startEvent('main', '1');
startEvent('other', '2');
pauseEvent('other', '2');
exitEvent('other', '2');
pauseEvent('main', '1');
exitEvent('main', '1');

await endTest();
expect(lastError, isNull);

expect(received, [
'Pause other. Collect group 2? Yes',
'Resume other',
'Pause main. Collect group 1? Yes',
'Resume main',
]);
});

test('throw when resuming other isolate is not ignored', () async {
resumeFailures = {'other'};

startEvent('main', '1');
startEvent('other', '2');
pauseEvent('other', '2');
exitEvent('other', '2');
pauseEvent('main', '1');
exitEvent('main', '1');

await endTest();
expect(lastError, isA<RPCError>());
});
});
}
2 changes: 1 addition & 1 deletion pkgs/coverage/test/test_util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import 'package:test_process/test_process.dart';

final String testAppPath = p.join('test', 'test_files', 'test_app.dart');

const Duration timeout = Duration(seconds: 30);
const Duration timeout = Duration(seconds: 60);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the timeout increase necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not directly related to this PR, but I've seen a few flakes of the integration tests on GitHub CI due to timing out


Future<TestProcess> runTestApp() => TestProcess.start(
Platform.resolvedExecutable,
Expand Down