[flutter_plugin_tools] Make firebase-test-lab fail when no tests run (flutter#4172)

stuartmorgan-g · web-flow · commit 44d7c072dfdb · 2021-07-21T12:03:47.000-07:00
If a package supports Android, it will now report failure instead of skip if no tests run. This matches the new behavior of drive-examples, and is intended to prevent recurrance of situations where we are silently failing to run tests because of, e.g., tests being in the wrong directory. Also fixes a long-standing but unnoticed problem where if a run tried to run more than one package's tests, it would hang forever (although on the bots it doesn't seem to time out, just end logs abruptly) due to a logic error in the call to configure gcloud. Fixes flutter#86732
diff --git a/.cirrus.yml b/.cirrus.yml
@@ -137,6 +137,22 @@ task:
           CHANNEL: "stable"
         MAPS_API_KEY: ENCRYPTED[596a9f6bca436694625ac50851dc5da6b4d34cba8025f7db5bc9465142e8cd44e15f69e3507787753accebfc4910d550]
         GCLOUD_FIREBASE_TESTLAB_KEY: ENCRYPTED[07586610af1fdfc894e5969f70ef2458341b9b7e9c3b7c4225a663b4a48732b7208a4d91c3b7d45305a6b55fa2a37fc4]
+        # Currently missing harness files (https://github.com/flutter/flutter/issues/86749):
+        #   camera/camera
+        #   google_sign_in/google_sign_in
+        #   in_app_purchase/in_app_purchase
+        #   in_app_purchase_android
+        #   quick_actions
+        #   shared_preferences/shared_preferences
+        #   url_launcher/url_launcher
+        #   video_player/video_player
+        #   webview_flutter
+        # Deprecated; no plan to backfill the missing files:
+        #   android_intent,connectivity/connectivity,device_info/device_info,sensors,share,wifi_info_flutter/wifi_info_flutter
+        # No integration tests to run:
+        #   image_picker/image_picker - Native UI is the critical functionality
+        #   espresso - No Dart code, so no integration tests
+        PLUGINS_TO_EXCLUDE_INTEGRATION_TESTS: "camera/camera,google_sign_in/google_sign_in,in_app_purchase/in_app_purchase,in_app_purchase_android,quick_actions,shared_preferences/shared_preferences,url_launcher/url_launcher,video_player/video_player,webview_flutter,android_intent,connectivity/connectivity,device_info/device_info,sensors,share,wifi_info_flutter/wifi_info_flutter,image_picker/image_picker,espresso"
       build_script:
         # Unsetting CIRRUS_CHANGE_MESSAGE and CIRRUS_COMMIT_MESSAGE as they
         # might include non-ASCII characters which makes Gradle crash.
@@ -159,7 +175,7 @@ task:
         - export CIRRUS_COMMIT_MESSAGE=""
         - if [[ -n "$GCLOUD_FIREBASE_TESTLAB_KEY" ]]; then
         -   echo $GCLOUD_FIREBASE_TESTLAB_KEY > ${HOME}/gcloud-service-key.json
-        -   ./script/tool_runner.sh firebase-test-lab --device model=flame,version=29 --device model=starqlteue,version=26
+        -   ./script/tool_runner.sh firebase-test-lab --device model=flame,version=29 --device model=starqlteue,version=26 --exclude $PLUGINS_TO_EXCLUDE_INTEGRATION_TESTS
         - else
         -   echo "This user does not have permission to run Firebase Test Lab tests."
         - fi
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
@@ -3,6 +3,12 @@
 - Added an `xctest` flag to select specific test targets, to allow running only
   unit tests or integration tests.
 - Split Xcode analysis out of `xctest` and into a new `xcode-analyze` command.
+- Fixed a bug that caused `firebase-test-lab` to hang if it tried to run more
+  than one plugin's tests in a single run.
+- **Breaking change**: If `firebase-test-lab` is run on a package that supports
+  Android, but for which no tests are run, it now fails instead of skipping.
+  This matches `drive-examples`, as this command is what is used for driving
+  Android Flutter integration tests on CI.
 
 ## 0.4.1
 
diff --git a/script/tool/lib/src/firebase_test_lab_command.dart b/script/tool/lib/src/firebase_test_lab_command.dart
@@ -76,13 +76,12 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
 
   static const String _gradleWrapper = 'gradlew';
 
-  Completer<void>? _firebaseProjectConfigured;
+  bool _firebaseProjectConfigured = false;
 
   Future<void> _configureFirebaseProject() async {
-    if (_firebaseProjectConfigured != null) {
-      return _firebaseProjectConfigured!.future;
+    if (_firebaseProjectConfigured) {
+      return;
     }
-    _firebaseProjectConfigured = Completer<void>();
 
     final String serviceKey = getStringArg('service-key');
     if (serviceKey.isEmpty) {
@@ -110,31 +109,34 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
       print('');
       if (exitCode == 0) {
         print('Firebase project configured.');
-        return;
       } else {
         logWarning(
             'Warning: gcloud config set returned a non-zero exit code. Continuing anyway.');
       }
     }
-    _firebaseProjectConfigured!.complete(null);
+    _firebaseProjectConfigured = true;
   }
 
   @override
   Future<PackageResult> runForPackage(Directory package) async {
-    if (!package
-        .childDirectory('example')
-        .childDirectory('android')
+    final Directory exampleDirectory = package.childDirectory('example');
+    final Directory androidDirectory =
+        exampleDirectory.childDirectory('android');
+    if (!androidDirectory.existsSync()) {
+      return PackageResult.skip(
+          '${getPackageDescription(exampleDirectory)} does not support Android.');
+    }
+
+    if (!androidDirectory
         .childDirectory('app')
         .childDirectory('src')
         .childDirectory('androidTest')
         .existsSync()) {
-      return PackageResult.skip('No example with androidTest directory');
+      printError('No androidTest directory found.');
+      return PackageResult.fail(
+          <String>['No tests ran (use --exclude if this is intentional).']);
     }
 
-    final Directory exampleDirectory = package.childDirectory('example');
-    final Directory androidDirectory =
-        exampleDirectory.childDirectory('android');
-
     // Ensures that gradle wrapper exists
     if (!await _ensureGradleWrapperExists(androidDirectory)) {
       return PackageResult.fail(<String>['Unable to build example apk']);
@@ -191,6 +193,12 @@ class FirebaseTestLabCommand extends PackageLoopingCommand {
         errors.add('$testName failed tests');
       }
     }
+
+    if (errors.isEmpty && resultsCounter == 0) {
+      printError('No integration tests were run.');
+      errors.add('No tests ran (use --exclude if this is intentional).');
+    }
+
     return errors.isEmpty
         ? PackageResult.success()
         : PackageResult.fail(errors);
diff --git a/script/tool/test/firebase_test_lab_command_test.dart b/script/tool/test/firebase_test_lab_command_test.dart
@@ -84,6 +84,85 @@ void main() {
           ]));
     });
 
+    test('only runs gcloud configuration once', () async {
+      createFakePlugin('plugin1', packagesDir, extraFiles: <String>[
+        'test/plugin_test.dart',
+        'example/integration_test/foo_test.dart',
+        'example/android/gradlew',
+        'example/android/app/src/androidTest/MainActivityTest.java',
+      ]);
+      createFakePlugin('plugin2', packagesDir, extraFiles: <String>[
+        'test/plugin_test.dart',
+        'example/integration_test/bar_test.dart',
+        'example/android/gradlew',
+        'example/android/app/src/androidTest/MainActivityTest.java',
+      ]);
+
+      final List<String> output = await runCapturingPrint(runner, <String>[
+        'firebase-test-lab',
+        '--device',
+        'model=flame,version=29',
+        '--device',
+        'model=seoul,version=26',
+        '--test-run-id',
+        'testRunId',
+        '--build-id',
+        'buildId',
+      ]);
+
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin1'),
+          contains('Firebase project configured.'),
+          contains('Testing example/integration_test/foo_test.dart...'),
+          contains('Running for plugin2'),
+          contains('Testing example/integration_test/bar_test.dart...'),
+        ]),
+      );
+
+      expect(
+        processRunner.recordedCalls,
+        orderedEquals(<ProcessCall>[
+          ProcessCall(
+              'gcloud',
+              'auth activate-service-account --key-file=${Platform.environment['HOME']}/gcloud-service-key.json'
+                  .split(' '),
+              null),
+          ProcessCall(
+              'gcloud', 'config set project flutter-infra'.split(' '), null),
+          ProcessCall(
+              '/packages/plugin1/example/android/gradlew',
+              'app:assembleAndroidTest -Pverbose=true'.split(' '),
+              '/packages/plugin1/example/android'),
+          ProcessCall(
+              '/packages/plugin1/example/android/gradlew',
+              'app:assembleDebug -Pverbose=true -Ptarget=/packages/plugin1/example/integration_test/foo_test.dart'
+                  .split(' '),
+              '/packages/plugin1/example/android'),
+          ProcessCall(
+              'gcloud',
+              'firebase test android run --type instrumentation --app build/app/outputs/apk/debug/app-debug.apk --test build/app/outputs/apk/androidTest/debug/app-debug-androidTest.apk --timeout 5m --results-bucket=gs://flutter_firebase_testlab --results-dir=plugins_android_test/plugin1/buildId/testRunId/0/ --device model=flame,version=29 --device model=seoul,version=26'
+                  .split(' '),
+              '/packages/plugin1/example'),
+          ProcessCall(
+              '/packages/plugin2/example/android/gradlew',
+              'app:assembleAndroidTest -Pverbose=true'.split(' '),
+              '/packages/plugin2/example/android'),
+          ProcessCall(
+              '/packages/plugin2/example/android/gradlew',
+              'app:assembleDebug -Pverbose=true -Ptarget=/packages/plugin2/example/integration_test/bar_test.dart'
+                  .split(' '),
+              '/packages/plugin2/example/android'),
+          ProcessCall(
+              'gcloud',
+              'firebase test android run --type instrumentation --app build/app/outputs/apk/debug/app-debug.apk --test build/app/outputs/apk/androidTest/debug/app-debug-androidTest.apk --timeout 5m --results-bucket=gs://flutter_firebase_testlab --results-dir=plugins_android_test/plugin2/buildId/testRunId/0/ --device model=flame,version=29 --device model=seoul,version=26'
+                  .split(' '),
+              '/packages/plugin2/example'),
+        ]),
+      );
+    });
+
     test('runs integration tests', () async {
       createFakePlugin('plugin', packagesDir, extraFiles: <String>[
         'test/plugin_test.dart',
@@ -203,12 +282,87 @@ void main() {
       );
     });
 
-    test('skips packages with no androidTest directory', () async {
+    test('fails for packages with no androidTest directory', () async {
       createFakePlugin('plugin', packagesDir, extraFiles: <String>[
         'example/integration_test/foo_test.dart',
         'example/android/gradlew',
       ]);
 
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(
+        runner,
+        <String>[
+          'firebase-test-lab',
+          '--device',
+          'model=flame,version=29',
+          '--device',
+          'model=seoul,version=26',
+          '--test-run-id',
+          'testRunId',
+          '--build-id',
+          'buildId',
+        ],
+        errorHandler: (Error e) {
+          commandError = e;
+        },
+      );
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('No androidTest directory found.'),
+          contains('The following packages had errors:'),
+          contains('plugin:\n'
+              '    No tests ran (use --exclude if this is intentional).'),
+        ]),
+      );
+    });
+
+    test('fails for packages with no integration test files', () async {
+      createFakePlugin('plugin', packagesDir, extraFiles: <String>[
+        'example/android/gradlew',
+        'example/android/app/src/androidTest/MainActivityTest.java',
+      ]);
+
+      Error? commandError;
+      final List<String> output = await runCapturingPrint(
+        runner,
+        <String>[
+          'firebase-test-lab',
+          '--device',
+          'model=flame,version=29',
+          '--device',
+          'model=seoul,version=26',
+          '--test-run-id',
+          'testRunId',
+          '--build-id',
+          'buildId',
+        ],
+        errorHandler: (Error e) {
+          commandError = e;
+        },
+      );
+
+      expect(commandError, isA<ToolExit>());
+      expect(
+        output,
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('No integration tests were run'),
+          contains('The following packages had errors:'),
+          contains('plugin:\n'
+              '    No tests ran (use --exclude if this is intentional).'),
+        ]),
+      );
+    });
+
+    test('skips packages with no android directory', () async {
+      createFakePackage('package', packagesDir, extraFiles: <String>[
+        'example/integration_test/foo_test.dart',
+      ]);
+
       final List<String> output = await runCapturingPrint(runner, <String>[
         'firebase-test-lab',
         '--device',
@@ -224,8 +378,8 @@ void main() {
       expect(
         output,
         containsAllInOrder(<Matcher>[
-          contains('Running for plugin'),
-          contains('No example with androidTest directory'),
+          contains('Running for package'),
+          contains('package/example does not support Android'),
         ]),
       );
       expect(output,
