From 85727528463da8bb7d23aee9f3b81dbb243b6173 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 27 May 2025 10:04:08 +1000 Subject: [PATCH 1/5] [coverage] Fix remaining ~0.1% flakiness --- pkgs/coverage/CHANGELOG.md | 6 ++++++ pkgs/coverage/lib/src/isolate_paused_listener.dart | 6 +++++- pkgs/coverage/pubspec.yaml | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkgs/coverage/CHANGELOG.md b/pkgs/coverage/CHANGELOG.md index 2a680adbe..699c56305 100644 --- a/pkgs/coverage/CHANGELOG.md +++ b/pkgs/coverage/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.14.1 + +- 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 - Require Dart ^3.6.0 diff --git a/pkgs/coverage/lib/src/isolate_paused_listener.dart b/pkgs/coverage/lib/src/isolate_paused_listener.dart index 49f17e7a9..9f618f440 100644 --- a/pkgs/coverage/lib/src/isolate_paused_listener.dart +++ b/pkgs/coverage/lib/src/isolate_paused_listener.dart @@ -63,7 +63,11 @@ class IsolatePausedListener { // Resume the main isolate. if (_mainIsolate != null) { - await _service.resume(_mainIsolate!.id!); + try { + await _service.resume(_mainIsolate!.id!); + } catch (RPCError) { + // The VM Service has already shut down, so there's nothing left to do. + } } } diff --git a/pkgs/coverage/pubspec.yaml b/pkgs/coverage/pubspec.yaml index e4a229e7d..7f17a76e4 100644 --- a/pkgs/coverage/pubspec.yaml +++ b/pkgs/coverage/pubspec.yaml @@ -1,5 +1,5 @@ name: coverage -version: 1.14.0 +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 From 25210a3425250af55b7688d82404db01dd06c4d1 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 27 May 2025 10:16:22 +1000 Subject: [PATCH 2/5] Fix analysis --- pkgs/coverage/lib/src/isolate_paused_listener.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/coverage/lib/src/isolate_paused_listener.dart b/pkgs/coverage/lib/src/isolate_paused_listener.dart index 9f618f440..c1b56c223 100644 --- a/pkgs/coverage/lib/src/isolate_paused_listener.dart +++ b/pkgs/coverage/lib/src/isolate_paused_listener.dart @@ -65,7 +65,7 @@ class IsolatePausedListener { if (_mainIsolate != null) { try { await _service.resume(_mainIsolate!.id!); - } catch (RPCError) { + } on RPCError { // The VM Service has already shut down, so there's nothing left to do. } } From b8322cdcf030b86c64514335e797abf5ea5d901e Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 27 May 2025 11:02:12 +1000 Subject: [PATCH 3/5] test --- .../test/isolate_paused_listener_test.dart | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/pkgs/coverage/test/isolate_paused_listener_test.dart b/pkgs/coverage/test/isolate_paused_listener_test.dart index 55d204528..2c4525f4a 100644 --- a/pkgs/coverage/test/isolate_paused_listener_test.dart +++ b/pkgs/coverage/test/isolate_paused_listener_test.dart @@ -487,6 +487,7 @@ void main() { late List received; Future Function(String)? delayTheOnPauseCallback; late bool stopped; + late Set resumeFailures; void startEvent(String id, String groupId, [String? name]) => allEvents.add(event( @@ -524,9 +525,13 @@ void main() { received = []; delayTheOnPauseCallback = null; + resumeFailures = {}; when(service.resume(any)).thenAnswer((invocation) async { final id = invocation.positionalArguments[0]; received.add('Resume $id'); + if (resumeFailures.contains(id)) { + throw RPCError('resume', -32000); + } return Success(); }); @@ -889,5 +894,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(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'); + + // TODO: Not sure how to write this test, since the RPCError is thrown in + // an async handler, and not propagated to this expectation. + // expect(() => endTest(), throwsA(isA())); + }); }); } From 0d8ea03b7903fc29774b6e6472d399c6551805ba Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 27 May 2025 11:52:24 +1000 Subject: [PATCH 4/5] Increase test timeout --- pkgs/coverage/test/isolate_paused_listener_test.dart | 2 +- pkgs/coverage/test/test_util.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/coverage/test/isolate_paused_listener_test.dart b/pkgs/coverage/test/isolate_paused_listener_test.dart index 2c4525f4a..8c29292ad 100644 --- a/pkgs/coverage/test/isolate_paused_listener_test.dart +++ b/pkgs/coverage/test/isolate_paused_listener_test.dart @@ -530,7 +530,7 @@ void main() { final id = invocation.positionalArguments[0]; received.add('Resume $id'); if (resumeFailures.contains(id)) { - throw RPCError('resume', -32000); + throw RPCError('resume', -32000, id); } return Success(); }); diff --git a/pkgs/coverage/test/test_util.dart b/pkgs/coverage/test/test_util.dart index 6fe89d3f5..92fb56148 100644 --- a/pkgs/coverage/test/test_util.dart +++ b/pkgs/coverage/test/test_util.dart @@ -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); Future runTestApp() => TestProcess.start( Platform.resolvedExecutable, From f44b83ea7c54f2440a21e2a947eaecd882af4d77 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Wed, 4 Jun 2025 14:15:20 +1000 Subject: [PATCH 5/5] Fix the error test --- .../test/isolate_paused_listener_test.dart | 79 +++++++++++-------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/pkgs/coverage/test/isolate_paused_listener_test.dart b/pkgs/coverage/test/isolate_paused_listener_test.dart index 8c29292ad..a76998779 100644 --- a/pkgs/coverage/test/isolate_paused_listener_test.dart +++ b/pkgs/coverage/test/isolate_paused_listener_test.dart @@ -483,6 +483,7 @@ void main() { late MockVmService service; late StreamController allEvents; late Future allIsolatesExited; + Object? lastError; late List received; Future Function(String)? delayTheOnPauseCallback; @@ -517,38 +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 = []; - delayTheOnPauseCallback = null; - resumeFailures = {}; - 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); - } - 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 = []; + delayTheOnPauseCallback = null; + resumeFailures = {}; + 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 { @@ -906,6 +917,7 @@ void main() { exitEvent('main', '1'); await endTest(); + expect(lastError, isNull); expect(received, [ 'Pause other. Collect group 2? Yes', @@ -925,9 +937,8 @@ void main() { pauseEvent('main', '1'); exitEvent('main', '1'); - // TODO: Not sure how to write this test, since the RPCError is thrown in - // an async handler, and not propagated to this expectation. - // expect(() => endTest(), throwsA(isA())); + await endTest(); + expect(lastError, isA()); }); }); }