Move some service manager tests to a proper integration test (flutter…

…#5916)
kenzieschmoll · Jun 14, 2023 · 36b39ac · 36b39ac
1 parent 3debabb
commit 36b39ac
Show file tree

Hide file tree

Showing 10 changed files with 132 additions and 79 deletions.
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<DevToolsJsonFile> sampleData = const [],
   List<DevToolsScreen>? 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
@@ -21,13 +21,15 @@ void main() {
 }
 
 void externalRunDevTools({
+  bool integrationTestMode = false,
   bool shouldEnableExperiments = false,
   List<DevToolsJsonFile> sampleData = const [],
 }) {
   // Set the extension points global.
   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
@@ -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<VmServiceWrapper> _connectWithSse(
@@ -27,6 +28,7 @@ Future<VmServiceWrapper> _connectWithSse(
     stream,
     client.sink!.add,
     uri,
+    trackFutures: integrationTestMode,
   );
 
   unawaited(
@@ -57,6 +59,7 @@ Future<VmServiceWrapper> _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
@@ -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
@@ -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
@@ -76,6 +76,7 @@ Future<void> 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,