Simplify devicelab logic and fix tests (#139122)

- fix flutter/flutter#53707 by having the test not expect a timeout but instead actually look for the retry message - simplify the `--task` option to only accept task names rather than also accepting paths - remove some obsolete options that referred to the manifest which no longer seems to exist
engine-flutter-autoroll · Nov 29, 2023 · 5e216d4 · 5e216d4
1 parent c532865
commit 5e216d4
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 61 deletions.
diff --git a/dev/devicelab/README.md b/dev/devicelab/README.md
@@ -66,16 +66,11 @@ To run a test, use option `-t` (`--task`):
 
 ```sh
 # from the .../flutter/dev/devicelab directory
-../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t {NAME_OR_PATH_OF_TEST}
+../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t {NAME_OF_TEST}
 ```
 
-Where `NAME_OR_PATH_OF_TEST` can be either of:
-
-* the _name_ of a task, which is a file's basename in `bin/tasks`. Example:
-  `complex_layout__start_up`.
-* the path to a Dart _file_ corresponding to a task, which resides in
-  `bin/tasks`. Tip: most shells support path auto-completion using the Tab key.
-  Example: `bin/tasks/complex_layout__start_up.dart`.
+Where `NAME_OR_PATH_OF_TEST` is the name of a task, which is a file's
+basename in `bin/tasks`. Example: `complex_layout__start_up`.
 
 To run multiple tests, repeat option `-t` (`--task`) multiple times:
 
@@ -261,4 +256,4 @@ Take gallery tasks for example:
 1. Linux android
    - Separating PR: https://github.com/flutter/flutter/pull/103550
    - Switching PR: https://github.com/flutter/flutter/pull/110533
-2. Mac iOS: https://github.com/flutter/flutter/pull/111164
+2. Mac iOS: https://github.com/flutter/flutter/pull/111164
diff --git a/dev/devicelab/bin/run.dart b/dev/devicelab/bin/run.dart
@@ -10,7 +10,6 @@ import 'package:flutter_devicelab/framework/ab.dart';
 import 'package:flutter_devicelab/framework/runner.dart';
 import 'package:flutter_devicelab/framework/task_result.dart';
 import 'package:flutter_devicelab/framework/utils.dart';
-import 'package:path/path.dart' as path;
 
 /// Runs tasks.
 ///
@@ -235,29 +234,12 @@ ArgParser createArgParser(List<String> taskNames) {
     ..addMultiOption(
       'task',
       abbr: 't',
-      help: 'Either:\n'
-          ' - the name of a task defined in manifest.yaml.\n'
-          '   Example: complex_layout__start_up.\n'
-          ' - the path to a Dart file corresponding to a task,\n'
-          '   which resides in bin/tasks.\n'
-          '   Example: bin/tasks/complex_layout__start_up.dart.\n'
+      help: 'Name of a Dart file in bin/tasks.\n'
+          '   Example: complex_layout__start_up\n'
           '\n'
           'This option may be repeated to specify multiple tasks.',
-      callback: (List<String> value) {
-        for (final String nameOrPath in value) {
-          final List<String> fragments = path.split(nameOrPath);
-          final bool isDartFile = fragments.last.endsWith('.dart');
-
-          if (fragments.length == 1 && !isDartFile) {
-            // Not a path
-            taskNames.add(nameOrPath);
-          } else if (!isDartFile || !path.equals(path.dirname(nameOrPath), path.join('bin', 'tasks'))) {
-            // Unsupported executable location
-            throw FormatException('Invalid value for option -t (--task): $nameOrPath');
-          } else {
-            taskNames.add(path.withoutExtension(fragments.last));
-          }
-        }
+      callback: (List<String> tasks) {
+        taskNames.addAll(tasks);
       },
     )
     ..addOption(
@@ -339,14 +321,6 @@ ArgParser createArgParser(List<String> taskNames) {
             'the location based on the value of the --flutter-root option.',
     )
     ..addOption('luci-builder', help: '[Flutter infrastructure] Name of the LUCI builder being run on.')
-    ..addFlag(
-      'match-host-platform',
-      defaultsTo: true,
-      help: 'Only run tests that match the host platform (e.g. do not run a\n'
-            'test with a `required_agent_capabilities` value of "mac/android"\n'
-            'on a windows host). Each test publishes its '
-            '`required_agent_capabilities`\nin the `manifest.yaml` file.',
-    )
     ..addOption(
       'results-file',
       help: '[Flutter infrastructure] File path for test results. If passed with\n'

diff --git a/dev/devicelab/bin/tasks/smoke_test_setup_failure.dart b/dev/devicelab/bin/tasks/smoke_test_setup_failure.dart
@@ -7,5 +7,8 @@
 /// By not calling `task()` the VM service extension is not registered and
 /// therefore will not accept requests to run tasks. When the runner attempts to
 /// connect and run the test it will receive a "method not found" error from the
-/// VM service, will likely retry and finally time out.
+/// VM service, will likely retry forever.
+///
+/// The test in ../../test/run_test.dart runs this task until it detects
+/// the retry message and then aborts the task.
 Future<void> main() async {}
diff --git a/dev/devicelab/lib/framework/runner.dart b/dev/devicelab/lib/framework/runner.dart
@@ -171,7 +171,8 @@ Future<TaskResult> runTask(
   final String taskExecutable = 'bin/tasks/$taskName.dart';
 
   if (!file(taskExecutable).existsSync()) {
-    throw 'Executable Dart file not found: $taskExecutable';
+    print('Executable Dart file not found: $taskExecutable');
+    exit(1);
   }
 
   if (useEmulator) {
@@ -288,7 +289,13 @@ Future<ConnectionResult> _connectToRunnerIsolate(Uri vmServiceUri) async {
       return ConnectionResult(client, isolate);
     } catch (error) {
       if (stopwatch.elapsed > const Duration(seconds: 10)) {
-        print('VM service still not ready after ${stopwatch.elapsed}: $error\nContinuing to retry...');
+        print(
+          'VM service still not ready. It is possible the target has failed.\n'
+          'Latest connection error:\n'
+          '  $error\n'
+          'Continuing to retry...\n',
+        );
+        stopwatch.reset();
       }
       await Future<void>.delayed(const Duration(milliseconds: 50));
     }

diff --git a/dev/devicelab/test/run_test.dart b/dev/devicelab/test/run_test.dart
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:convert';
 import 'dart:io';
 
 import 'package:flutter_devicelab/framework/utils.dart' show rm;
@@ -12,45 +13,54 @@ import 'common.dart';
 
 void main() {
   const ProcessManager processManager = LocalProcessManager();
+  final String dart = path.absolute(
+    path.join('..', '..', 'bin', 'cache', 'dart-sdk', 'bin', 'dart'));
 
   group('run.dart script', () {
-    Future<ProcessResult> runScript(List<String> testNames,
+    // The tasks here refer to files in ../bin/tasks/*.dart
+
+    Future<ProcessResult> runScript(List<String> taskNames,
         [List<String> otherArgs = const <String>[]]) async {
-      final String dart = path.absolute(
-          path.join('..', '..', 'bin', 'cache', 'dart-sdk', 'bin', 'dart'));
       final ProcessResult scriptProcess = processManager.runSync(<String>[
         dart,
         'bin/run.dart',
         '--no-terminate-stray-dart-processes',
         ...otherArgs,
-        for (final String testName in testNames) ...<String>['-t', testName],
+        for (final String testName in taskNames) ...<String>['-t', testName],
       ]);
       return scriptProcess;
     }
 
     Future<void> expectScriptResult(
-        List<String> testNames,
+        List<String> taskNames,
         int expectedExitCode,
         {String? deviceId}
       ) async {
-      final ProcessResult result = await runScript(testNames, <String>[
+      final ProcessResult result = await runScript(taskNames, <String>[
         if (deviceId != null) ...<String>['-d', deviceId],
       ]);
-      expect(result.exitCode, expectedExitCode,
-          reason:
-              '[ stderr from test process ]\n\n${result.stderr}\n\n[ end of stderr ]'
-              '\n\n[ stdout from test process ]\n\n${result.stdout}\n\n[ end of stdout ]');
+      expect(
+        result.exitCode,
+        expectedExitCode,
+        reason:
+          '[ stderr from test process ]\n'
+          '\n'
+          '${result.stderr}\n'
+          '\n'
+          '[ end of stderr ]\n'
+          '\n'
+          '[ stdout from test process ]\n'
+          '\n'
+          '${result.stdout}\n'
+          '\n'
+          '[ end of stdout ]',
+      );
     }
 
     test('exits with code 0 when succeeds', () async {
       await expectScriptResult(<String>['smoke_test_success'], 0);
     });
 
-    test('accepts file paths', () async {
-      await expectScriptResult(
-          <String>['bin/tasks/smoke_test_success.dart'], 0);
-    });
-
     test('rejects invalid file paths', () async {
       await expectScriptResult(<String>['lib/framework/adb.dart'], 1);
     });
@@ -63,9 +73,18 @@ void main() {
       await expectScriptResult(<String>['smoke_test_failure'], 1);
     });
 
-    test('exits with code 1 when fails to connect', () async {
-      await expectScriptResult(<String>['smoke_test_setup_failure'], 1);
-    }, skip: true); // https://github.com/flutter/flutter/issues/53707
+    test('prints a message after a few seconds when failing to connect (this test takes >10s)', () async {
+      final Process process = await processManager.start(<String>[
+        dart,
+        'bin/run.dart',
+        '--no-terminate-stray-dart-processes',
+        '-t', 'smoke_test_setup_failure',
+      ]);
+      await process.stdout.transform(utf8.decoder).where(
+        (String line) => line.contains('VM service still not ready. It is possible the target has failed')
+      ).first;
+      expect(process.kill(), isTrue);
+    });
 
     test('exits with code 1 when results are mixed', () async {
       await expectScriptResult(

diff --git a/packages/flutter_goldens_client/pubspec.yaml b/packages/flutter_goldens_client/pubspec.yaml
@@ -19,4 +19,4 @@ dartdoc:
   # Exclude this package from the hosted API docs.
   nodoc: true
 
-# PUBSPEC CHECKSUM: 1c2e
+# PUBSPEC CHECKSUM: 1c2e