Fix broken integration tests (flutter#8299)

Sarsilmaz02 · Sep 6, 2024 · 3127636 · 3127636
1 parent 2879e93
commit 3127636
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 37 deletions.
diff --git a/packages/devtools_app/integration_test/run_tests.dart b/packages/devtools_app/integration_test/run_tests.dart
@@ -18,17 +18,17 @@ import 'test_infra/run/run_test.dart';
 const _testDirectory = 'integration_test/test';
 const _offlineIndicator = 'integration_test/test/offline';
 
-/// The key in [_skipTestsForDevice] that will hold a set of tests that should
+/// The key in [_disabledTestsForDevice] that will hold a set of tests that should
 /// be skipped for all test devices.
 const _testDeviceAll = 'all';
 
-/// The set of tests that should be skipped for each type of test target.
+/// The set of tests that are temporarily disabled for each type of test device.
 ///
 /// This list should be empty most of the time, but may contain a broken test
 /// while a fix being worked on.
 ///
 /// Format: `'my_example_test.dart'`.
-final _skipTestsForDevice = <String, Set<String>>{
+final _disabledTestsForDevice = <String, Set<String>>{
   _testDeviceAll: {
     // https://github.com/flutter/devtools/issues/6592
     'eval_and_browse_test.dart',
@@ -67,14 +67,17 @@ Future<void> _runTest(
   final testTarget = testRunnerArgs.testTarget!;
   final testDevice = testRunnerArgs.testAppDevice.name;
 
-  final skipAll = _skipTestsForDevice[_testDeviceAll]!;
-  final skipForDevice = _skipTestsForDevice[testDevice] ?? {};
-  final shouldSkip =
-      {...skipAll, ...skipForDevice}.any((t) => testTarget.endsWith(t));
-  if (shouldSkip) return;
+  final disabledForAllDevices = _disabledTestsForDevice[_testDeviceAll]!;
+  final disabledForDevice = _disabledTestsForDevice[testDevice] ?? {};
+  final disabled = {...disabledForAllDevices, ...disabledForDevice}
+      .any((t) => testTarget.endsWith(t));
+  if (disabled) {
+    debugLog('Disabled test - skipping $testTarget for $testDevice.');
+    return;
+  }
 
   if (!testRunnerArgs.testAppDevice.supportsTest(testTarget)) {
-    // Skip test, since it is not supported for device.
+    debugLog('Unsupported test - skipping $testTarget for $testDevice.');
     return;
   }
 

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
@@ -28,24 +28,24 @@ Future<void> runFlutterIntegrationTest(
 
   if (!offline) {
     if (testRunnerArgs.testAppUri == null) {
-      debugLog('Starting a test application');
       // Create the test app and start it.
       try {
         if (testRunnerArgs.testAppDevice == TestAppDevice.cli) {
           debugLog(
-            'Creating a TestDartCliApp with path ${testFileArgs.appPath}',
+            'creating a TestDartCliApp with path ${testFileArgs.appPath}',
           );
           testApp = TestDartCliApp(appPath: testFileArgs.appPath);
         } else {
           debugLog(
-            'Creating a TestFlutterApp with path ${testFileArgs.appPath} and '
+            'creating a TestFlutterApp with path ${testFileArgs.appPath} and '
             'device ${testRunnerArgs.testAppDevice}',
           );
           testApp = TestFlutterApp(
             appPath: testFileArgs.appPath,
             appDevice: testRunnerArgs.testAppDevice,
           );
         }
+        debugLog('starting the test app');
         await testApp.start();
       } catch (e) {
         // ignore: avoid-throw-in-catch-block, by design
@@ -63,8 +63,10 @@ Future<void> runFlutterIntegrationTest(
     final testArgs = <String, Object>{
       if (!offline) 'service_uri': testAppUri,
     };
+    final testTarget = testRunnerArgs.testTarget!;
+    debugLog('starting test run for $testTarget');
     await testRunner.run(
-      testRunnerArgs.testTarget!,
+      testTarget,
       testDriver: 'test_driver/integration_test.dart',
       headless: testRunnerArgs.headless,
       dartDefineArgs: [

diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md
@@ -13,6 +13,7 @@
 * Deprecate `surveyActionTakenPropertyName`.
 * Deprecate `apiGetSurveyShownCount` in favor of `SurveyApi.getSurveyShownCount`.
 * Deprecate `apiIncrementSurveyShownCount` in favor of `SurveyApi.incrementSurveyShownCount`.
+* Support Chrome's new headless mode in the integration test runner.
 
 # 10.0.2
 * Update dependency `web_socket_channel: '>=2.4.0 <4.0.0'`.

diff --git a/packages/devtools_shared/lib/src/test/chrome_driver.dart b/packages/devtools_shared/lib/src/test/chrome_driver.dart
@@ -16,14 +16,15 @@ class ChromeDriver with IOMixin {
   // https://github.com/flutter/flutter/blob/master/docs/contributing/testing/Running-Flutter-Driver-tests-with-Web.md#web-installers-repo.
   Future<void> start({bool debugLogging = false}) async {
     try {
+      const chromedriverExe = 'chromedriver';
+      const chromedriverArgs = ['--port=4444'];
       if (debugLogging) {
         print('starting the chromedriver process');
+        print('> $chromedriverExe ${chromedriverArgs.join(' ')}');
       }
       final process = _process = await Process.start(
-        'chromedriver',
-        [
-          '--port=4444',
-        ],
+        chromedriverExe,
+        chromedriverArgs,
       );
       listenToProcessOutput(process, printTag: 'ChromeDriver');
     } catch (e) {

diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart
@@ -35,23 +35,34 @@ class IntegrationTestRunner with IOMixin {
     }
 
     Future<void> runTest({required int attemptNumber}) async {
+      debugLog('starting attempt #$attemptNumber for $testTarget');
       debugLog('starting the flutter drive process');
-      final process = await Process.start(
-        'flutter',
-        [
-          'drive',
-          // Debug outputs from the test will not show up in profile mode. Since
-          // we rely on debug outputs for detecting errors and exceptions from the
-          // test, we cannot run this these tests in profile mode until this issue
-          // is resolved.  See https://github.com/flutter/flutter/issues/69070.
-          // '--profile',
-          '--driver=$testDriver',
-          '--target=$testTarget',
-          '-d',
-          headless ? 'web-server' : 'chrome',
-          for (final arg in dartDefineArgs) '--dart-define=$arg',
+
+      final flutterDriveArgs = [
+        'drive',
+        // Debug outputs from the test will not show up in profile mode. Since
+        // we rely on debug outputs for detecting errors and exceptions from the
+        // test, we cannot run this these tests in profile mode until this issue
+        // is resolved.  See https://github.com/flutter/flutter/issues/69070.
+        // '--profile',
+        '--driver=$testDriver',
+        '--target=$testTarget',
+        '-d',
+        headless ? 'web-server' : 'chrome',
+        // --disable-gpu speeds up tests that use ChromeDriver when run on
+        // GitHub Actions. See https://github.com/flutter/devtools/issues/8301.
+        '--web-browser-flag=--disable-gpu',
+        if (headless) ...[
+          // Flags to avoid breakage with chromedriver 128. See
+          // https://github.com/flutter/devtools/issues/8301.
+          '--web-browser-flag=--headless=old',
+          '--web-browser-flag=--disable-search-engine-choice-screen',
         ],
-      );
+        for (final arg in dartDefineArgs) '--dart-define=$arg',
+      ];
+
+      debugLog('> flutter ${flutterDriveArgs.join(' ')}');
+      final process = await Process.start('flutter', flutterDriveArgs);
 
       bool stdOutWriteInProgress = false;
       bool stdErrWriteInProgress = false;
@@ -112,6 +123,10 @@ class IntegrationTestRunner with IOMixin {
         timeout,
       ]);
 
+      debugLog(
+        'shutting down processes because '
+        '${testTimedOut ? 'test timed out' : 'test finished'}',
+      );
       debugLog('attempting to kill the flutter drive process');
       process.kill();
       debugLog('flutter drive process has exited');
@@ -288,6 +303,10 @@ Future<void> runOneOrManyTests<T extends IntegrationTestRunnerArgs>({
     return;
   }
 
+  void debugLog(String log) {
+    if (debugLogging) print(log);
+  }
+
   final chromedriver = ChromeDriver();
 
   try {
@@ -297,6 +316,7 @@ Future<void> runOneOrManyTests<T extends IntegrationTestRunnerArgs>({
     if (testRunnerArgs.testTarget != null) {
       // TODO(kenz): add support for specifying a directory as the target instead
       // of a single file.
+      debugLog('Attempting to run a single test: ${testRunnerArgs.testTarget}');
       await runTest(testRunnerArgs);
     } else {
       // Run all supported tests since a specific target test was not provided.
@@ -321,12 +341,18 @@ Future<void> runOneOrManyTests<T extends IntegrationTestRunnerArgs>({
         testFiles = testFiles.sublist(shardStart, shardEnd);
       }
 
+      debugLog(
+        'Attempting to run all tests: '
+        '${testFiles.map((file) => file.path).toList().toString()}',
+      );
+
       for (final testFile in testFiles) {
         final testTarget = testFile.path;
         final newArgsWithTarget = newArgsGenerator([
           ...testRunnerArgs.rawArgs,
           '--${IntegrationTestRunnerArgs.testTargetArg}=$testTarget',
         ]);
+        debugLog('Attempting to run: $testTarget');
         await runTest(newArgsWithTarget);
       }
     }

diff --git a/packages/devtools_shared/lib/src/test/io_utils.dart b/packages/devtools_shared/lib/src/test/io_utils.dart
@@ -82,7 +82,7 @@ mixin IOMixin {
   }) async {
     final processId = process.pid;
     if (debugLogging) {
-      print('Sending SIGTERM to $processId..');
+      print('Sending SIGTERM to $processId.');
     }
     await cancelAllStreamSubscriptions();
     Process.killPid(processId);
@@ -100,7 +100,7 @@ mixin IOMixin {
     // Use sigint here instead of sigkill. See
     // https://github.com/flutter/flutter/issues/117415.
     if (debugLogging) {
-      print('Sending SIGINT to $processId..');
+      print('Sending SIGINT to $processId.');
     }
     Process.killPid(processId, ProcessSignal.sigint);
     return process.exitCode;

diff --git a/tool/ci/bots.sh b/tool/ci/bots.sh
@@ -77,9 +77,9 @@ elif [ "$BOT" = "integration_dart2js" ]; then
         echo "Preparing to run integration tests. Warning: if you see the exception \
 'Web Driver Command WebDriverCommandType.screenshot failed while waiting for driver side', \
 this is a known issue and likely means that the golden image check failed (see \
-https://github.com/flutter/flutter/issues/118470). Run the test locally to see if new \
-images under a 'failures/' directory are created as a result of the test run: \
-$ dart run integration_test/run_tests.dart --headless"
+https://github.com/flutter/flutter/issues/118470). Look at the summary of the Github Actions \
+run to see if golden image failures have been uploaded (this only happens once all checks have \
+completed). Download these goldens and update them in the codebase to apply the updates."
 
         if [ "$DEVICE" = "flutter" ]; then
             dart run integration_test/run_tests.dart --headless --shard="$SHARD"