Skip to content

Commit 48bf6a4

Browse files
committed
address review feedback on _findTestSuiteByRootLibrary
1 parent 7aa34ce commit 48bf6a4

2 files changed

Lines changed: 55 additions & 7 deletions

File tree

packages/devtools_app_shared/lib/src/service/isolate_manager.dart

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,19 @@ final class IsolateManager with DisposerMixin {
264264
}
265265

266266
Future<IsolateRef?> _findTestSuiteByRootLibrary(VmService? service) async {
267+
String? projectRootUri;
268+
if (service != null) {
269+
try {
270+
final vm = await service.getVM();
271+
if (service != _service) return null;
272+
// `rootUri` is not a typed field on `VM` in the current vm_service
273+
// version, so read it from the raw JSON response.
274+
projectRootUri = vm.json?['rootUri'] as String?;
275+
} catch (_) {
276+
// If getVM() fails, proceed without project-root scoping.
277+
}
278+
}
279+
267280
for (final isolateState in _isolateStates.values) {
268281
final isolate = await isolateState.isolate;
269282
if (service != _service) return null;
@@ -273,25 +286,48 @@ final class IsolateManager with DisposerMixin {
273286

274287
if (_isDartTestRunnerRootLibrary(rootLibraryUri)) continue;
275288

276-
if (_isLikelyUserTestRootLibrary(rootLibraryUri)) {
289+
if (_isLikelyUserTestRootLibrary(
290+
rootLibraryUri,
291+
projectRootUri: projectRootUri,
292+
)) {
277293
return isolateState.isolateRef;
278294
}
279295
}
280296

281297
return null;
282298
}
283299

300+
/// Returns true if [uri] looks like the root library of a dart:test runner
301+
/// isolate (as opposed to the user's test suite isolate).
302+
///
303+
/// Note: 'dart_test.kernel', 'package:test_core/', and 'package:test_api/'
304+
/// are implementation details of package:test that DevTools depends on.
305+
/// Changing these in package:test will break this logic.
284306
bool _isDartTestRunnerRootLibrary(String uri) {
285307
return uri.contains('dart_test.kernel') ||
286308
uri.startsWith('package:test_core/') ||
287309
uri.startsWith('package:test_api/');
288310
}
289311

290-
bool _isLikelyUserTestRootLibrary(String uri) {
291-
return (uri.endsWith('_test.dart') ||
292-
uri.contains('/test/') ||
293-
uri.contains('\\test\\')) &&
294-
!uri.startsWith('dart:');
312+
/// Returns true if [uri] looks like the root library of a user test isolate.
313+
///
314+
/// When [projectRootUri] is provided, `file://` URIs must be under it to
315+
/// avoid false positives from other packages in the workspace that happen to
316+
/// contain '/test/' or end with '_test.dart'.
317+
bool _isLikelyUserTestRootLibrary(String uri, {String? projectRootUri}) {
318+
if (uri.startsWith('dart:')) return false;
319+
320+
// Scope file:// URIs to the current project to avoid matching test files
321+
// from other packages in a workspace.
322+
if (projectRootUri != null &&
323+
uri.startsWith('file:') &&
324+
!uri.startsWith(projectRootUri)) {
325+
return false;
326+
}
327+
328+
return uri.endsWith('_test.dart') ||
329+
uri.contains('/test/') ||
330+
uri.contains('\\test\\');
295331
}
296332

297333
void _setSelectedIsolate(IsolateRef? ref) {

packages/devtools_app_shared/test/service/isolate_manager_test.dart

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,11 +178,15 @@ void main() {
178178

179179
/// Minimal fake VmService for IsolateManager tests.
180180
class _FakeVmService extends Fake implements VmService {
181-
_FakeVmService(this.isolates);
181+
_FakeVmService(this.isolates, {this.vmRootUri});
182182

183183
/// Map of isolate id -> Isolate to return from getIsolate().
184184
final Map<String, Isolate> isolates;
185185

186+
/// Optional root URI returned by [getVM], used to scope test library
187+
/// detection to the current project.
188+
final String? vmRootUri;
189+
186190
final _isolateEventController = StreamController<Event>.broadcast();
187191

188192
@override
@@ -191,6 +195,14 @@ class _FakeVmService extends Fake implements VmService {
191195
@override
192196
Stream<Event> get onDebugEvent => const Stream.empty();
193197

198+
@override
199+
Future<VM> getVM() async {
200+
return VM.parse({
201+
'type': 'VM',
202+
if (vmRootUri != null) 'rootUri': vmRootUri,
203+
})!;
204+
}
205+
194206
@override
195207
Future<Isolate> getIsolate(String isolateId) async {
196208
return isolates[isolateId] ??

0 commit comments

Comments
 (0)