From 36b39acbd24ab27abf6f92cb6b5197745072787f Mon Sep 17 00:00:00 2001 From: Kenzie Davisson <43759233+kenzieschmoll@users.noreply.github.com> Date: Wed, 14 Jun 2023 14:04:06 -0700 Subject: [PATCH] Move some service manager tests to a proper integration test (#5916) --- .../service_connection_test.dart | 87 +++++++++++++++++++ .../test_infra/run/_io_utils.dart | 10 ++- .../test_infra/run/_test_app_driver.dart | 2 +- .../test_infra/run/run_test.dart | 3 +- packages/devtools_app/lib/initialization.dart | 28 ++++-- packages/devtools_app/lib/main.dart | 2 + .../devtools_app/lib/src/service/service.dart | 3 + .../devtools_app/lib/src/shared/globals.dart | 7 ++ .../test/shared/service_manager_test.dart | 68 --------------- .../integration_test_utils.dart | 1 + 10 files changed, 132 insertions(+), 79 deletions(-) create mode 100644 packages/devtools_app/integration_test/test/live_connection/service_connection_test.dart diff --git a/packages/devtools_app/integration_test/test/live_connection/service_connection_test.dart b/packages/devtools_app/integration_test/test/live_connection/service_connection_test.dart new file mode 100644 index 00000000000..fbf0bcad2fe --- /dev/null +++ b/packages/devtools_app/integration_test/test/live_connection/service_connection_test.dart @@ -0,0 +1,87 @@ +// Copyright 2023 The Chromium Authors. 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:devtools_app/devtools_app.dart'; +import 'package:devtools_test/devtools_integration_test.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:integration_test/integration_test.dart'; + +// To run: +// dart run integration_test/run_tests.dart --target=integration_test/test/live_connection/service_connection_test.dart + +void main() { + IntegrationTestWidgetsFlutterBinding.ensureInitialized(); + + late TestApp testApp; + + setUpAll(() { + testApp = TestApp.fromEnvironment(); + expect(testApp.vmServiceUri, isNotNull); + }); + + tearDown(() async { + await resetHistory(); + }); + + testWidgets('initial service connection state', (tester) async { + await pumpAndConnectDevTools(tester, testApp); + + // Await a delay to ensure the service extensions have had a chance to + // be called. This delay may be able to be shortened if doing so does + // not cause bot flakiness. + await tester.pump(longDuration); + + // Ensure all futures are completed before running checks. + await serviceManager.service!.allFuturesCompleted; + + logStatus('verify the number of vm service calls on connect'); + final vmServiceCallCount = serviceManager.service!.vmServiceCallCount; + expect( + // Use a range instead of an exact number because service extension + // calls are not consistent. This will still catch any spurious calls + // that are unintentionally added at start up. + const Range(50, 60).contains(vmServiceCallCount), + isTrue, + reason: 'Unexpected number of vm service calls upon connection: ' + '$vmServiceCallCount. If this is expected, please update this test ' + 'to the new expected number of calls. Here are the calls for this ' + 'test run:\n ${serviceManager.service!.vmServiceCalls.toString()}', + ); + // Check the ordering of the vm service calls we can expect to occur + // in a stable order. + expect( + serviceManager.service!.vmServiceCalls + // Filter out unawaited streamListen calls. + .where((call) => call != 'streamListen') + .toList() + .sublist(0, 5), + equals([ + 'getSupportedProtocols', + 'getVersion', + 'getFlagList', + 'getVM', + 'getIsolate', + ]), + ); + + expect( + serviceManager.service!.vmServiceCalls + .where((call) => call == 'streamListen') + .toList() + .length, + equals(10), + ); + + logStatus('verify managers have all been initialized'); + expect(serviceManager.isolateManager, isNotNull); + expect(serviceManager.serviceExtensionManager, isNotNull); + expect(serviceManager.vmFlagManager, isNotNull); + expect(serviceManager.isolateManager.isolates.value, isNotEmpty); + expect(serviceManager.vmFlagManager.flags.value, isNotNull); + + if (serviceManager.isolateManager.selectedIsolate.value == null) { + await whenValueNonNull(serviceManager.isolateManager.selectedIsolate); + } + }); +} diff --git a/packages/devtools_app/integration_test/test_infra/run/_io_utils.dart b/packages/devtools_app/integration_test/test_infra/run/_io_utils.dart index 8ab36c969d2..ae297915967 100644 --- a/packages/devtools_app/integration_test/test_infra/run/_io_utils.dart +++ b/packages/devtools_app/integration_test/test_infra/run/_io_utils.dart @@ -23,10 +23,14 @@ mixin IOMixin { void listenToProcessOutput( Process process, { - void Function(String) printCallback = _defaultPrintCallback, void Function(String)? onStdout, void Function(String)? onStderr, + void Function(String)? printCallback, + String printTag = '', }) { + printCallback = + printCallback ?? (line) => _defaultPrintCallback(line, tag: printTag); + streamSubscriptions.addAll([ transformToLines(process.stdout).listen((String line) { onStdout?.call(line); @@ -52,7 +56,7 @@ mixin IOMixin { streamSubscriptions.clear(); } - static void _defaultPrintCallback(String line) { - print(line); + static void _defaultPrintCallback(String line, {String tag = ''}) { + print(tag.isNotEmpty ? '$tag - $line' : line); } } diff --git a/packages/devtools_app/integration_test/test_infra/run/_test_app_driver.dart b/packages/devtools_app/integration_test/test_infra/run/_test_app_driver.dart index 658f4563804..9f49b1ca05c 100644 --- a/packages/devtools_app/integration_test/test_infra/run/_test_app_driver.dart +++ b/packages/devtools_app/integration_test/test_infra/run/_test_app_driver.dart @@ -300,7 +300,7 @@ abstract class _TestApp with IOMixin { msg.length > maxLength ? '${msg.substring(0, maxLength)}...' : msg; _allMessages.add(truncatedMsg); if (_printDebugOutputToStdOut) { - print(truncatedMsg); + print('_TestApp - $truncatedMsg'); } return msg; } diff --git a/packages/devtools_app/integration_test/test_infra/run/run_test.dart b/packages/devtools_app/integration_test/test_infra/run/run_test.dart index beff7486bc0..0fc179371a6 100644 --- a/packages/devtools_app/integration_test/test_infra/run/run_test.dart +++ b/packages/devtools_app/integration_test/test_infra/run/run_test.dart @@ -103,7 +103,7 @@ class ChromeDriver with IOMixin { '--port=4444', ], ); - listenToProcessOutput(_process); + listenToProcessOutput(_process, printTag: 'ChromeDriver'); } void kill() { @@ -153,6 +153,7 @@ class TestRunner with IOMixin { listenToProcessOutput( process, + printTag: 'FlutterDriveProcess', onStdout: (line) { if (line.startsWith(_TestResult.testResultPrefix)) { final testResultJson = line.substring(line.indexOf('{')); diff --git a/packages/devtools_app/lib/initialization.dart b/packages/devtools_app/lib/initialization.dart index 489a56384d0..fcf4392abeb 100644 --- a/packages/devtools_app/lib/initialization.dart +++ b/packages/devtools_app/lib/initialization.dart @@ -29,6 +29,7 @@ import 'src/shared/primitives/utils.dart'; /// If the initialization is specific to running Devtools in google3 or /// externally, then it should be added to that respective main.dart file. void runDevTools({ + bool integrationTestMode = false, bool shouldEnableExperiments = false, List sampleData = const [], List? screens, @@ -44,12 +45,10 @@ void runDevTools({ usePathUrlStrategy(); - // This may be set to true from our Flutter integration tests. Since we call - // [runDevTools] from Dart code, we cannot set the 'enable_experiments' - // environment variable before calling [runDevTools]. - if (shouldEnableExperiments) { - setEnableExperiments(); - } + _maybeInitForIntegrationTestMode( + integrationTestMode: integrationTestMode, + enableExperiments: shouldEnableExperiments, + ); // Initialize the framework before we do anything else, otherwise the // StorageController won't be initialized and preferences won't be loaded. @@ -79,6 +78,23 @@ void runDevTools({ }); } +/// Initializes some DevTools global fields for our Flutter integration tests. +/// +/// Since we call [runDevTools] from Dart code, we cannot set environment +/// variables before calling [runDevTools], and therefore have to pass in these +/// values manually to [runDevTools]. +void _maybeInitForIntegrationTestMode({ + required bool integrationTestMode, + required bool enableExperiments, +}) { + if (!integrationTestMode) return; + + setIntegrationTestMode(); + if (enableExperiments) { + setEnableExperiments(); + } +} + /// Checks if the request is for a legacy URL and if so, redirects to the new /// equivalent. /// diff --git a/packages/devtools_app/lib/main.dart b/packages/devtools_app/lib/main.dart index 4bffbcb1b19..fe2dc2df785 100644 --- a/packages/devtools_app/lib/main.dart +++ b/packages/devtools_app/lib/main.dart @@ -21,6 +21,7 @@ void main() { } void externalRunDevTools({ + bool integrationTestMode = false, bool shouldEnableExperiments = false, List sampleData = const [], }) { @@ -28,6 +29,7 @@ void externalRunDevTools({ setGlobal(DevToolsExtensionPoints, ExternalDevToolsExtensionPoints()); runDevTools( + integrationTestMode: integrationTestMode, shouldEnableExperiments: shouldEnableExperiments, sampleData: sampleData, ); diff --git a/packages/devtools_app/lib/src/service/service.dart b/packages/devtools_app/lib/src/service/service.dart index d793b425dd6..147a1c413dd 100644 --- a/packages/devtools_app/lib/src/service/service.dart +++ b/packages/devtools_app/lib/src/service/service.dart @@ -8,6 +8,7 @@ import 'package:vm_service/utils.dart'; import 'package:web_socket_channel/web_socket_channel.dart'; import '../shared/config_specific/sse/sse_shim.dart'; +import '../shared/globals.dart'; import 'vm_service_wrapper.dart'; Future _connectWithSse( @@ -27,6 +28,7 @@ Future _connectWithSse( stream, client.sink!.add, uri, + trackFutures: integrationTestMode, ); unawaited( @@ -57,6 +59,7 @@ Future _connectWithWebSocket( ws.sink.add(message); }, uri, + trackFutures: integrationTestMode, ); if (ws.closeCode != null) { diff --git a/packages/devtools_app/lib/src/shared/globals.dart b/packages/devtools_app/lib/src/shared/globals.dart index 774d817e283..d5b0e415fe8 100644 --- a/packages/devtools_app/lib/src/shared/globals.dart +++ b/packages/devtools_app/lib/src/shared/globals.dart @@ -59,3 +59,10 @@ EvalService get evalService => globals[EvalService] as EvalService; void setGlobal(Type clazz, Object instance) { globals[clazz] = instance; } + +/// Whether DevTools is being run in integration test mode. +bool get integrationTestMode => _integrationTestMode; +bool _integrationTestMode = false; +void setIntegrationTestMode() { + _integrationTestMode = true; +} diff --git a/packages/devtools_app/test/shared/service_manager_test.dart b/packages/devtools_app/test/shared/service_manager_test.dart index 620d8233eb4..2ac73c08cd0 100644 --- a/packages/devtools_app/test/shared/service_manager_test.dart +++ b/packages/devtools_app/test/shared/service_manager_test.dart @@ -36,74 +36,6 @@ void main() { await env.tearDownEnvironment(force: true); }); - test( - 'verify number of vm service calls on connect', - () async { - await env.setupEnvironment(); - // Await a delay to ensure the service extensions have had a chance to - // be called. This delay may be able to be shortened if doing so does - // not cause bot flakiness. - await Future.delayed(const Duration(seconds: 10)); - // Ensure all futures are completed before running checks. - await serviceManager.service!.allFuturesCompleted; - expect( - // Use a range instead of an exact number because service extension - // calls are not consistent. This will still catch any spurious calls - // that are unintentionally added at start up. - const Range(15, 35) - .contains(serviceManager.service!.vmServiceCallCount), - isTrue, - reason: - 'Unexpected number of vm service calls upon connection. If this ' - 'is expected, please update this test to the new expected number ' - 'of calls. Here are the calls for this test run:\n' - '${serviceManager.service!.vmServiceCalls.toString()}', - ); - // Check the ordering of the vm service calls we can expect to occur - // in a stable order. - expect( - serviceManager.service!.vmServiceCalls - // Filter out unawaited streamListen calls. - .where((call) => call != 'streamListen') - .toList() - .sublist(0, 3), - equals([ - 'getFlagList', - 'getVM', - 'getIsolate', - ]), - ); - - expect( - serviceManager.service!.vmServiceCalls - .where((call) => call == 'streamListen') - .toList() - .length, - equals(8), - ); - - await env.tearDownEnvironment(); - }, - timeout: const Timeout.factor(4), - ); - - test('vmServiceOpened', () async { - await env.setupEnvironment(); - - expect(serviceManager.service, equals(env.service)); - expect(serviceManager.isolateManager, isNotNull); - expect(serviceManager.serviceExtensionManager, isNotNull); - expect(serviceManager.vmFlagManager, isNotNull); - expect(serviceManager.isolateManager.isolates.value, isNotEmpty); - expect(serviceManager.vmFlagManager.flags.value, isNotNull); - - if (serviceManager.isolateManager.selectedIsolate.value == null) { - await whenValueNonNull(serviceManager.isolateManager.selectedIsolate); - } - - await env.tearDownEnvironment(); - }); - test( 'invalid setBreakpoint throws exception', () async { diff --git a/packages/devtools_test/lib/src/integration_test/integration_test_utils.dart b/packages/devtools_test/lib/src/integration_test/integration_test_utils.dart index 3d94217abf5..6951c357b4d 100644 --- a/packages/devtools_test/lib/src/integration_test/integration_test_utils.dart +++ b/packages/devtools_test/lib/src/integration_test/integration_test_utils.dart @@ -76,6 +76,7 @@ Future pumpDevTools(WidgetTester tester) async { // Error when reading 'org-dartlang-app:/test_infra/shared.dart': File not found const shouldEnableExperiments = bool.fromEnvironment('enable_experiments'); app.externalRunDevTools( + integrationTestMode: true, // ignore: avoid_redundant_argument_values, by design shouldEnableExperiments: shouldEnableExperiments, sampleData: _sampleData,