-
Notifications
You must be signed in to change notification settings - Fork 389
Fix auto select main isolate #9756
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
062845e
fcd42b9
45fa504
9c1cf10
8775ece
0f74d38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,20 +166,25 @@ final class IsolateManager with DisposerMixin { | |
| !event.isolate!.isSystemIsolate!) { | ||
| await _registerIsolate(event.isolate!); | ||
| _isolateCreatedController.add(event.isolate); | ||
| // TODO(jacobr): we assume the first isolate started is the main isolate | ||
| // but that may not always be a safe assumption. | ||
| if (_mainIsolate.value == null) { | ||
|
|
||
| // Recompute whenever a new isolate starts so test connections can move | ||
| // from the runner isolate to the user test-suite isolate when available. | ||
| final previousMain = _mainIsolate.value; | ||
| final computedMain = await _computeMainIsolate(); | ||
| if (computedMain != null) { | ||
| _mainIsolate.value = computedMain; | ||
| } else if (_mainIsolate.value == null) { | ||
| _mainIsolate.value = event.isolate; | ||
| if (_shouldReselectMainIsolate) { | ||
| // Assume the main isolate has come back up after a hot restart, so | ||
| // select it. | ||
| _shouldReselectMainIsolate = false; | ||
| _setSelectedIsolate(event.isolate); | ||
| } | ||
| } | ||
|
|
||
| if (_selectedIsolate.value == null) { | ||
| _setSelectedIsolate(event.isolate); | ||
| if (_mainIsolate.value != null && | ||
| (_shouldReselectMainIsolate || | ||
| _selectedIsolate.value == null || | ||
| _selectedIsolate.value == previousMain)) { | ||
| // If the previous main exited and returned (hot restart) or we were | ||
| // following the previous main, follow the newly computed main isolate. | ||
| _shouldReselectMainIsolate = false; | ||
| _setSelectedIsolate(_mainIsolate.value); | ||
| } | ||
| } else if (event.kind == EventKind.kServiceExtensionAdded) { | ||
| // Check to see if there is a new isolate. | ||
|
|
@@ -223,25 +228,71 @@ final class IsolateManager with DisposerMixin { | |
|
|
||
| final service = _service; | ||
| for (final isolateState in _isolateStates.values) { | ||
| if (_selectedIsolate.value == null) { | ||
| final isolate = await isolateState.isolate; | ||
| if (service != _service) return null; | ||
| for (final extensionName in isolate?.extensionRPCs ?? <String>[]) { | ||
| if (extensions.isFlutterExtension(extensionName)) { | ||
| return isolateState.isolateRef; | ||
| } | ||
| final isolate = await isolateState.isolate; | ||
| if (service != _service) return null; | ||
| for (final extensionName in isolate?.extensionRPCs ?? <String>[]) { | ||
| if (extensions.isFlutterExtension(extensionName)) { | ||
| return isolateState.isolateRef; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| final ref = _isolateStates.keys.firstWhereOrNull((IsolateRef ref) { | ||
| // 'foo.dart:main()' | ||
| return ref.name!.contains(':main('); | ||
| return ref.name?.contains(':main(') ?? false; | ||
| }); | ||
|
|
||
| if (ref == null) { | ||
| final rootLibraryTestSuiteRef = | ||
jakemac53 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| await _findTestSuiteByRootLibrary(service); | ||
| if (rootLibraryTestSuiteRef != null) return rootLibraryTestSuiteRef; | ||
|
|
||
| // When connecting to a test run, the test package (package:test_core) | ||
| // spawns each test suite in a separate isolate with a debug name | ||
| // prefixed with 'test_suite:'. DevTools should connect to this isolate | ||
| // rather than the test runner isolate ('main'), since the test suite | ||
| // isolate is where user code actually runs. | ||
| // See: https://github.com/flutter/devtools/issues/9747 | ||
| final testSuiteRef = _isolateStates.keys.firstWhereOrNull( | ||
| (IsolateRef ref) => ref.name?.startsWith('test_suite:') ?? false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this isolate not marked as a system isolate? Ideally it just wouldn't appear in the isolates list.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the actual test, so it's the thing we do want to find. I don't know if we can make the root isolate (test runner) be not marked as a system isolate. But that is the one we would want to ignore if any probably.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that makes sense. If we mark the main isolate as a system isolate, I'm not sure we'll need this change. We already do it for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can achieve the same result that way it would definitely be preferable to matching based on debug name and other heuristics
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the context! Since flutter_tester is a C++ engine binary, marking the runner isolate as isSystemIsolate: true would require a change in flutter/engine. Should I pursue that separately and remove the heuristics from this PR, or would you prefer to merge this as a stopgap for now?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably just go update flutter/engine - both things are released as a part of the flutter SDK so they would reach real users at the same time (which is unfortunately going to be a while now due to release schedules, unless we cherry pick this into the next stable). |
||
| ); | ||
| if (testSuiteRef != null) return testSuiteRef; | ||
| } | ||
|
|
||
| return ref ?? _isolateStates.keys.first; | ||
| } | ||
|
|
||
| Future<IsolateRef?> _findTestSuiteByRootLibrary(VmService? service) async { | ||
| for (final isolateState in _isolateStates.values) { | ||
| final isolate = await isolateState.isolate; | ||
| if (service != _service) return null; | ||
|
|
||
| final rootLibraryUri = isolate?.rootLib?.uri; | ||
| if (rootLibraryUri == null) continue; | ||
|
|
||
| if (_isDartTestRunnerRootLibrary(rootLibraryUri)) continue; | ||
|
|
||
| if (_isLikelyUserTestRootLibrary(rootLibraryUri)) { | ||
| return isolateState.isolateRef; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| bool _isDartTestRunnerRootLibrary(String uri) { | ||
| return uri.contains('dart_test.kernel') || | ||
rishika0212 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| uri.startsWith('package:test_core/') || | ||
| uri.startsWith('package:test_api/'); | ||
| } | ||
|
|
||
| bool _isLikelyUserTestRootLibrary(String uri) { | ||
| return (uri.endsWith('_test.dart') || | ||
| uri.contains('/test/') || | ||
| uri.contains('\\test\\')) && | ||
rishika0212 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| !uri.startsWith('dart:'); | ||
| } | ||
|
|
||
| void _setSelectedIsolate(IsolateRef? ref) { | ||
| _selectedIsolate.value = ref; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,252 @@ | ||
| // Copyright 2026 The Flutter Authors | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. | ||
|
|
||
| import 'dart:async'; | ||
|
|
||
| import 'package:devtools_app_shared/src/service/isolate_manager.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:vm_service/vm_service.dart'; | ||
|
|
||
| /// Minimal fake VmService for IsolateManager tests. | ||
| class _FakeVmService extends Fake implements VmService { | ||
rishika0212 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| _FakeVmService(this.isolates); | ||
|
|
||
| /// Map of isolate id -> Isolate to return from getIsolate(). | ||
| final Map<String, Isolate> isolates; | ||
|
|
||
| final _isolateEventController = StreamController<Event>.broadcast(); | ||
|
|
||
| @override | ||
| Stream<Event> get onIsolateEvent => _isolateEventController.stream; | ||
|
|
||
| @override | ||
| Stream<Event> get onDebugEvent => const Stream.empty(); | ||
|
|
||
| @override | ||
| Future<Isolate> getIsolate(String isolateId) async { | ||
| return isolates[isolateId] ?? | ||
| Isolate.parse({ | ||
| 'id': isolateId, | ||
| 'runnable': true, | ||
| 'extensionRPCs': <String>[], | ||
| })!; | ||
| } | ||
|
|
||
| @override | ||
| Future<Success> resume(String isolateId, {String? step, int? frameIndex}) => | ||
| Future.value(Success()); | ||
|
|
||
| Future<void> emitIsolateStart(IsolateRef isolateRef) async { | ||
| _isolateEventController.add( | ||
| Event.parse({ | ||
| 'type': 'Event', | ||
| 'kind': EventKind.kIsolateStart, | ||
| 'isolate': { | ||
| 'type': '@Isolate', | ||
| 'id': isolateRef.id, | ||
| 'name': isolateRef.name, | ||
| 'isSystemIsolate': isolateRef.isSystemIsolate, | ||
| }, | ||
| })!, | ||
| ); | ||
| await Future<void>.delayed(Duration.zero); | ||
| } | ||
|
|
||
| @override | ||
| Future<void> dispose() async { | ||
| await _isolateEventController.close(); | ||
| } | ||
| } | ||
|
|
||
| /// Creates a minimal runnable [Isolate] for a given [IsolateRef]. | ||
| Isolate _makeIsolate(IsolateRef ref, {String? rootLibraryUri}) { | ||
| final json = <String, Object?>{ | ||
| 'id': ref.id, | ||
| 'name': ref.name, | ||
| 'type': '@Isolate', | ||
| 'runnable': true, | ||
| 'extensionRPCs': <String>[], | ||
| if (rootLibraryUri != null) | ||
| 'rootLib': { | ||
| 'type': '@Library', | ||
| 'id': 'libraries/0', | ||
| 'uri': rootLibraryUri, | ||
| }, | ||
| }; | ||
|
|
||
| return Isolate.parse(json)!; | ||
| } | ||
|
|
||
| /// Creates an [IsolateRef] with the given name and id. | ||
| IsolateRef _makeRef(String name, String id) { | ||
| return IsolateRef.parse({'name': name, 'id': id, 'isSystemIsolate': false})!; | ||
| } | ||
|
|
||
| void main() { | ||
| group('IsolateManager._computeMainIsolate', () { | ||
| late IsolateManager manager; | ||
| final fakeServices = <_FakeVmService>[]; | ||
|
|
||
| setUp(() { | ||
| manager = IsolateManager(); | ||
| }); | ||
|
|
||
| tearDown(() { | ||
| manager.handleVmServiceClosed(); | ||
| for (final fakeService in fakeServices) { | ||
| unawaited(fakeService.dispose()); | ||
| } | ||
| fakeServices.clear(); | ||
| }); | ||
|
|
||
| test( | ||
| 'selects test_suite isolate instead of test runner when running tests', | ||
| () async { | ||
| // Simulates the isolate list seen when connecting to a test run: | ||
| // - 'main' is the test runner isolate (wrong choice) | ||
| // - 'test_suite:...' is where user code actually runs (correct choice) | ||
| // - 'vm-service' is infrastructure | ||
| final testRunnerRef = _makeRef('main', 'isolates/1'); | ||
| final testSuiteRef = _makeRef( | ||
| 'test_suite:file:///tmp/dart_test.kernel.dill', | ||
| 'isolates/2', | ||
| ); | ||
| final vmServiceRef = _makeRef('vm-service', 'isolates/3'); | ||
|
|
||
| final fakeService = _FakeVmService({ | ||
| 'isolates/1': _makeIsolate(testRunnerRef), | ||
rishika0212 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 'isolates/2': _makeIsolate(testSuiteRef), | ||
| 'isolates/3': _makeIsolate(vmServiceRef), | ||
| }); | ||
| fakeServices.add(fakeService); | ||
|
|
||
| manager.vmServiceOpened(fakeService); | ||
| await manager.init([testRunnerRef, testSuiteRef, vmServiceRef]); | ||
|
|
||
| expect( | ||
| manager.selectedIsolate.value?.name, | ||
| equals('test_suite:file:///tmp/dart_test.kernel.dill'), | ||
| reason: | ||
| 'Should auto-select the test_suite isolate, not the test runner', | ||
| ); | ||
| expect( | ||
| manager.mainIsolate.value?.name, | ||
| equals('test_suite:file:///tmp/dart_test.kernel.dill'), | ||
| reason: 'Main isolate should also resolve to the test_suite isolate', | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| test('selects main isolate for normal (non-test) app runs', () async { | ||
| final mainRef = _makeRef('main', 'isolates/1'); | ||
| final vmServiceRef = _makeRef('vm-service', 'isolates/2'); | ||
|
|
||
| final fakeService = _FakeVmService({ | ||
| 'isolates/1': _makeIsolate(mainRef), | ||
rishika0212 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 'isolates/2': _makeIsolate(vmServiceRef), | ||
| }); | ||
| fakeServices.add(fakeService); | ||
|
|
||
| manager.vmServiceOpened(fakeService); | ||
| await manager.init([mainRef, vmServiceRef]); | ||
|
|
||
| expect( | ||
| manager.selectedIsolate.value?.name, | ||
| equals('main'), | ||
| reason: 'Should select the main isolate for normal app runs', | ||
| ); | ||
| }); | ||
|
|
||
| test('selects isolate containing :main( for dart scripts', () async { | ||
| final scriptRef = _makeRef('foo.dart:main()', 'isolates/1'); | ||
|
|
||
| final fakeService = _FakeVmService({ | ||
| 'isolates/1': _makeIsolate(scriptRef), | ||
| }); | ||
| fakeServices.add(fakeService); | ||
|
|
||
| manager.vmServiceOpened(fakeService); | ||
| await manager.init([scriptRef]); | ||
|
|
||
| expect( | ||
| manager.selectedIsolate.value?.name, | ||
| equals('foo.dart:main()'), | ||
| ); | ||
| }); | ||
|
|
||
| test( | ||
| 'selects test isolate by root library when test_suite prefix is absent', | ||
| () async { | ||
| final testRunnerRef = _makeRef('main', 'isolates/1'); | ||
| final userTestRef = _makeRef('isolate-2', 'isolates/2'); | ||
| final vmServiceRef = _makeRef('vm-service', 'isolates/3'); | ||
|
|
||
| final fakeService = _FakeVmService({ | ||
| 'isolates/1': _makeIsolate( | ||
| testRunnerRef, | ||
| rootLibraryUri: 'file:///tmp/dart_test.kernel.abcd/test.dart', | ||
| ), | ||
| 'isolates/2': _makeIsolate( | ||
| userTestRef, | ||
| rootLibraryUri: 'package:my_app/foo_test.dart', | ||
| ), | ||
| 'isolates/3': _makeIsolate( | ||
| vmServiceRef, | ||
| rootLibraryUri: 'dart:developer', | ||
| ), | ||
| }); | ||
| fakeServices.add(fakeService); | ||
|
|
||
| manager.vmServiceOpened(fakeService); | ||
| await manager.init([testRunnerRef, userTestRef, vmServiceRef]); | ||
|
|
||
| expect( | ||
| manager.selectedIsolate.value?.name, | ||
| equals('isolate-2'), | ||
| reason: 'Should choose user test isolate using root library metadata', | ||
| ); | ||
| expect( | ||
| manager.mainIsolate.value?.name, | ||
| equals('isolate-2'), | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| test( | ||
| 'promotes main isolate from test runner to test suite on isolate start', | ||
| () async { | ||
| final testRunnerRef = _makeRef('main', 'isolates/1'); | ||
| final testSuiteRef = _makeRef( | ||
| 'test_suite:file:///tmp/dart_test.kernel.dill', | ||
| 'isolates/2', | ||
| ); | ||
|
|
||
| final fakeService = _FakeVmService({ | ||
| 'isolates/1': _makeIsolate(testRunnerRef), | ||
| 'isolates/2': _makeIsolate(testSuiteRef), | ||
| }); | ||
| fakeServices.add(fakeService); | ||
|
|
||
| manager.vmServiceOpened(fakeService); | ||
| await manager.init(const []); | ||
|
|
||
| await fakeService.emitIsolateStart(testRunnerRef); | ||
| expect(manager.selectedIsolate.value?.name, equals('main')); | ||
| expect(manager.mainIsolate.value?.name, equals('main')); | ||
|
|
||
| await fakeService.emitIsolateStart(testSuiteRef); | ||
| expect( | ||
| manager.selectedIsolate.value?.name, | ||
| equals('test_suite:file:///tmp/dart_test.kernel.dill'), | ||
| reason: | ||
| 'Should switch selection to test_suite isolate once it starts', | ||
| ); | ||
| expect( | ||
| manager.mainIsolate.value?.name, | ||
| equals('test_suite:file:///tmp/dart_test.kernel.dill'), | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.