Refactor devicelab bin/run.dart and other cleanup (#96320)

dev/devicelab/bin/run.dart

@@ -13,102 +13,108 @@ import 'package:flutter_devicelab/framework/task_result.dart';
 import 'package:flutter_devicelab/framework/utils.dart';
 import 'package:path/path.dart' as path;
 
-late ArgResults args;
-
-List<String> _taskNames = <String>[];
-
-/// The device-id to run test on.
-String? deviceId;
+/// Runs tasks.
+///
+/// The tasks are chosen depending on the command-line options.
+Future<void> main(List<String> rawArgs) async {
+  // This is populated by a callback in the ArgParser.
+  final List<String> taskNames = <String>[];
+  final ArgParser argParser = createArgParser(taskNames);
 
-/// The git branch being tested on.
-String? gitBranch;
+  ArgResults args;
+  try {
+    args = argParser.parse(rawArgs); // populates taskNames as a side-effect
+  } on FormatException catch (error) {
+    stderr.writeln('${error.message}\n');
+    stderr.writeln('Usage:\n');
+    stderr.writeln(argParser.usage);
+    exit(1);
+  }
 
-/// The build of the local engine to use.
-///
-/// Required for A/B test mode.
-String? localEngine;
+  /// Suppresses standard output, prints only standard error output.
+  final bool silent = (args['silent'] as bool?) ?? false;
 
-/// The path to the engine "src/" directory.
-String? localEngineSrcPath;
+  /// The build of the local engine to use.
+  ///
+  /// Required for A/B test mode.
+  final String? localEngine = args['local-engine'] as String?;
 
-/// Name of the LUCI builder this test is currently running on.
-///
-/// This is only passed on CI runs for Cocoon to be able to uniquely identify
-/// this test run.
-String? luciBuilder;
+  /// The path to the engine "src/" directory.
+  final String? localEngineSrcPath = args['local-engine-src-path'] as String?;
 
-/// Whether to exit on first test failure.
-bool exitOnFirstTestFailure = false;
+  /// The device-id to run test on.
+  final String? deviceId = args['device-id'] as String?;
 
-/// Path to write test results to.
-String? resultsPath;
+  /// Whether to exit on first test failure.
+  final bool exitOnFirstTestFailure = (args['exit'] as bool?) ?? false;
 
-/// File containing a service account token.
-///
-/// If passed, the test run results will be uploaded to Flutter infrastructure.
-String? serviceAccountTokenFile;
+  /// Whether to tell tasks to clean up after themselves.
+  final bool terminateStrayDartProcesses = (args['terminate-stray-dart-processes'] as bool?) ?? false;
 
-/// Suppresses standard output, prints only standard error output.
-bool silent = false;
+  /// The git branch being tested on.
+  final String? gitBranch = args['git-branch'] as String?;
 
-/// Runs tasks.
-///
-/// The tasks are chosen depending on the command-line options
-/// (see [_argParser]).
-Future<void> main(List<String> rawArgs) async {
-  try {
-    args = _argParser.parse(rawArgs);
-  } on FormatException catch (error) {
-    stderr.writeln('${error.message}\n');
-    stderr.writeln('Usage:\n');
-    stderr.writeln(_argParser.usage);
-    exitCode = 1;
-    return;
-  }
+  /// Name of the LUCI builder this test is currently running on.
+  ///
+  /// This is only passed on CI runs for Cocoon to be able to uniquely identify
+  /// this test run.
+  final String? luciBuilder = args['luci-builder'] as String?;
 
-  deviceId = args['device-id'] as String?;
-  exitOnFirstTestFailure = (args['exit'] as bool?) ?? false;
-  gitBranch = args['git-branch'] as String?;
-  localEngine = args['local-engine'] as String?;
-  localEngineSrcPath = args['local-engine-src-path'] as String?;
-  luciBuilder = args['luci-builder'] as String?;
-  resultsPath = args['results-file'] as String?;
-  serviceAccountTokenFile = args['service-account-token-file'] as String?;
-  silent = (args['silent'] as bool?) ?? false;
+  /// Path to write test results to.
+  final String? resultsPath = args['results-file'] as String?;
 
   if (!args.wasParsed('task')) {
     if (args.wasParsed('stage') || args.wasParsed('all')) {
       addTasks(
         tasks: loadTaskManifest().tasks,
         args: args,
-        taskNames: _taskNames,
+        taskNames: taskNames,
       );
     }
   }
 
   if (args.wasParsed('list')) {
-    for (int i = 0; i < _taskNames.length; i++) {
-      print('${(i + 1).toString().padLeft(3)} - ${_taskNames[i]}');
+    for (int i = 0; i < taskNames.length; i++) {
+      print('${(i + 1).toString().padLeft(3)} - ${taskNames[i]}');
     }
-    exitCode = 0;
-    return;
+    exit(0);
   }
 
-  if (_taskNames.isEmpty) {
+  if (taskNames.isEmpty) {
     stderr.writeln('Failed to find tasks to run based on supplied options.');
-    exitCode = 1;
-    return;
+    exit(1);
   }
 
   if (args.wasParsed('ab')) {
-    await _runABTest();
+    final int runsPerTest = int.parse(args['ab'] as String);
+    final String resultsFile = args['ab-result-file'] as String? ?? 'ABresults#.json';
+    if (taskNames.length > 1) {
+      stderr.writeln('When running in A/B test mode exactly one task must be passed but got ${taskNames.join(', ')}.\n');
+      stderr.writeln(argParser.usage);
+      exit(1);
+    }
+    if (localEngine == null) {
+      stderr.writeln('When running in A/B test mode --local-engine is required.\n');
+      stderr.writeln(argParser.usage);
+      exit(1);
+    }
+    await _runABTest(
+      runsPerTest: runsPerTest,
+      silent: silent,
+      localEngine: localEngine,
+      localEngineSrcPath: localEngineSrcPath,
+      deviceId: deviceId,
+      resultsFile: resultsFile,
+      taskName: taskNames.single,
+    );
   } else {
-    await runTasks(_taskNames,
+    await runTasks(taskNames,
       silent: silent,
       localEngine: localEngine,
       localEngineSrcPath: localEngineSrcPath,
       deviceId: deviceId,
       exitOnFirstTestFailure: exitOnFirstTestFailure,
+      terminateStrayDartProcesses: terminateStrayDartProcesses,
       gitBranch: gitBranch,
       luciBuilder: luciBuilder,
       resultsPath: resultsPath,
@@ -116,26 +122,18 @@ Future<void> main(List<String> rawArgs) async {
   }
 }
 
-Future<void> _runABTest() async {
-  final int runsPerTest = int.parse(args['ab'] as String);
-
-  if (_taskNames.length > 1) {
-    stderr.writeln('When running in A/B test mode exactly one task must be passed but got ${_taskNames.join(', ')}.\n');
-    stderr.writeln(_argParser.usage);
-    exit(1);
-  }
-
-  if (!args.wasParsed('local-engine')) {
-    stderr.writeln('When running in A/B test mode --local-engine is required.\n');
-    stderr.writeln(_argParser.usage);
-    exit(1);
-  }
-
-  final String taskName = _taskNames.single;
-
+Future<void> _runABTest({
+  required int runsPerTest,
+  required bool silent,
+  required String localEngine,
+  required String? localEngineSrcPath,
+  required String? deviceId,
+  required String resultsFile,
+  required String taskName,
+}) async {
   print('$taskName A/B test. Will run $runsPerTest times.');
 
-  final ABTest abTest = ABTest(localEngine!, taskName);
+  final ABTest abTest = ABTest(localEngine, taskName);
   for (int i = 1; i <= runsPerTest; i++) {
     section('Run #$i');
 
@@ -182,7 +180,7 @@ Future<void> _runABTest() async {
   }
   abTest.finalize();
 
-  final File jsonFile = _uniqueFile(args['ab-result-file'] as String? ?? 'ABresults#.json');
+  final File jsonFile = _uniqueFile(resultsFile);
   jsonFile.writeAsStringSync(const JsonEncoder.withIndent('  ').convert(abTest.jsonMap));
 
   if (silent != true) {
@@ -234,8 +232,8 @@ void addTasks({
   }
 }
 
-/// Command-line options for the `run.dart` command.
-final ArgParser _argParser = ArgParser()
+ArgParser createArgParser(List<String> taskNames) {
+  return ArgParser()
     ..addMultiOption(
       'task',
       abbr: 't',
@@ -254,12 +252,12 @@ final ArgParser _argParser = ArgParser()
 
           if (fragments.length == 1 && !isDartFile) {
             // Not a path
-          _taskNames.add(nameOrPath);
+            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));
+            taskNames.add(path.withoutExtension(fragments.last));
           }
         }
       },
@@ -308,7 +306,8 @@ final ArgParser _argParser = ArgParser()
     ..addFlag(
       'exit',
       defaultsTo: true,
-    help: 'Exit on the first test failure.',
+      help: 'Exit on the first test failure. Currently flakes are intentionally (though '
+            'incorrectly) not considered to be failures.',
     )
     ..addOption(
       'git-branch',
@@ -361,6 +360,14 @@ final ArgParser _argParser = ArgParser()
     )
     ..addFlag(
       'silent',
+      help: 'Reduce verbosity slightly.',
+    )
+    ..addFlag(
+      'terminate-stray-dart-processes',
+      defaultsTo: true,
+      help: 'Whether to send a SIGKILL signal to any Dart processes that are still '
+            'running when a task is completed. If any Dart processes are terminated '
+            'in this way, the test is considered to have failed.',
     )
     ..addMultiOption(
       'test',
@@ -373,3 +380,4 @@ final ArgParser _argParser = ArgParser()
         }
       },
     );
+}

dev/devicelab/lib/framework/framework.dart

@@ -10,6 +10,7 @@ import 'dart:isolate';
 
 import 'package:logging/logging.dart';
 import 'package:path/path.dart' as path;
+import 'package:process/process.dart';
 import 'package:stack_trace/stack_trace.dart';
 
 import 'devices.dart';
@@ -41,35 +42,43 @@ bool _isTaskRegistered = false;
 ///
 /// It is OK for a [task] to perform many things. However, only one task can be
 /// registered per Dart VM.
-Future<TaskResult> task(TaskFunction task) async {
+///
+/// If no `processManager` is provided, a default [LocalProcessManager] is created
+/// for the task.
+Future<TaskResult> task(TaskFunction task, { ProcessManager? processManager }) async {
   if (_isTaskRegistered)
     throw StateError('A task is already registered');
-
   _isTaskRegistered = true;
 
+  processManager ??= const LocalProcessManager();
+
   // TODO(ianh): allow overriding logging.
   Logger.root.level = Level.ALL;
   Logger.root.onRecord.listen((LogRecord rec) {
     print('${rec.level.name}: ${rec.time}: ${rec.message}');
   });
 
-  final _TaskRunner runner = _TaskRunner(task);
+  final _TaskRunner runner = _TaskRunner(task, processManager);
   runner.keepVmAliveUntilTaskRunRequested();
   return runner.whenDone;
 }
 
 class _TaskRunner {
-  _TaskRunner(this.task) {
+  _TaskRunner(this.task, this.processManager) {
     registerExtension('ext.cocoonRunTask',
         (String method, Map<String, String> parameters) async {
       final Duration? taskTimeout = parameters.containsKey('timeoutInMinutes')
         ? Duration(minutes: int.parse(parameters['timeoutInMinutes']!))
         : null;
-      // This is only expected to be passed in unit test runs so they do not
-      // kill the Dart process that is running them and waste time running config.
-      final bool runFlutterConfig = parameters['runFlutterConfig'] != 'false';
+      final bool runFlutterConfig = parameters['runFlutterConfig'] != 'false'; // used by tests to avoid changing the configuration
       final bool runProcessCleanup = parameters['runProcessCleanup'] != 'false';
-      final TaskResult result = await run(taskTimeout, runProcessCleanup: runProcessCleanup, runFlutterConfig: runFlutterConfig);
+      final String? localEngine = parameters['localEngine'];
+      final TaskResult result = await run(
+        taskTimeout,
+        runProcessCleanup: runProcessCleanup,
+        runFlutterConfig: runFlutterConfig,
+        localEngine: localEngine,
+      );
       return ServiceExtensionResponse.result(json.encode(result.toJson()));
     });
     registerExtension('ext.cocoonRunnerReady',
@@ -79,6 +88,7 @@ class _TaskRunner {
   }
 
   final TaskFunction task;
+  final ProcessManager processManager;
 
   Future<Device?> _getWorkingDeviceIfAvailable() async {
     try {
@@ -103,6 +113,7 @@ class _TaskRunner {
   Future<TaskResult> run(Duration? taskTimeout, {
     bool runFlutterConfig = true,
     bool runProcessCleanup = true,
+    required String? localEngine,
   }) async {
     try {
       _taskStarted = true;
@@ -113,20 +124,19 @@ class _TaskRunner {
         section('Checking running Dart$exe processes');
         beforeRunningDartInstances = await getRunningProcesses(
           processName: 'dart$exe',
-        ).toSet();
-        final Set<RunningProcessInfo> allProcesses = await getRunningProcesses().toSet();
+          processManager: processManager,
+        );
+        final Set<RunningProcessInfo> allProcesses = await getRunningProcesses(processManager: processManager);
         beforeRunningDartInstances.forEach(print);
         for (final RunningProcessInfo info in allProcesses) {
           if (info.commandLine.contains('iproxy')) {
             print('[LEAK]: ${info.commandLine} ${info.creationDate} ${info.pid} ');
           }
         }
-      } else {
-        section('Skipping check running Dart$exe processes');
       }
 
       if (runFlutterConfig) {
-        print('enabling configs for macOS, Linux, Windows, and Web...');
+        print('Enabling configs for macOS, Linux, Windows, and Web...');
         final int configResult = await exec(path.join(flutterDirectory.path, 'bin', 'flutter'), <String>[
           'config',
           '-v',
@@ -134,13 +144,11 @@ class _TaskRunner {
           '--enable-windows-desktop',
           '--enable-linux-desktop',
           '--enable-web',
-          if (localEngine != null) ...<String>['--local-engine', localEngine!],
+          if (localEngine != null) ...<String>['--local-engine', localEngine],
         ], canFail: true);
         if (configResult != 0) {
           print('Failed to enable configuration, tasks may not run.');
         }
-      } else {
-        print('Skipping enabling configs for macOS, Linux, Windows, and Web');
       }
 
       final Device? device = await _getWorkingDeviceIfAvailable();
@@ -169,26 +177,24 @@ class _TaskRunner {
       }
 
       if (runProcessCleanup) {
-        section('Checking running Dart$exe processes after task...');
-        final List<RunningProcessInfo> afterRunningDartInstances = await getRunningProcesses(
+        section('Terminating lingering Dart$exe processes after task...');
+        final Set<RunningProcessInfo> afterRunningDartInstances = await getRunningProcesses(
           processName: 'dart$exe',
-        ).toList();
+          processManager: processManager,
+        );
         for (final RunningProcessInfo info in afterRunningDartInstances) {
           if (!beforeRunningDartInstances.contains(info)) {
             print('$info was leaked by this test.');
             if (result is TaskResultCheckProcesses) {
               result = TaskResult.failure('This test leaked dart processes');
             }
-            final bool killed = await killProcess(info.pid);
-            if (!killed) {
-              print('Failed to kill process ${info.pid}.');
-            } else {
+            if (await info.terminate(processManager: processManager)) {
               print('Killed process id ${info.pid}.');
+            } else {
+              print('Failed to kill process ${info.pid}.');
             }
           }
         }
-      } else {
-        print('Skipping check running Dart$exe processes after task');
       }
       _completer.complete(result);
       return result;

dev/devicelab/lib/framework/runner.dart

@@ -4,7 +4,6 @@
 
 import 'dart:async';
 import 'dart:convert';
-// import 'dart:core' as core;
 import 'dart:io';
 
 import 'package:meta/meta.dart';
@@ -29,6 +28,10 @@ import 'utils.dart';
 Future<void> runTasks(
   List<String> taskNames, {
   bool exitOnFirstTestFailure = false,
+  // terminateStrayDartProcesses defaults to false so that tests don't have to specify it.
+  // It is set based on the --terminate-stray-dart-processes command line argument in
+  // normal execution, and that flag defaults to true.
+  bool terminateStrayDartProcesses = false,
   bool silent = false,
   String? deviceId,
   String? gitBranch,
@@ -50,6 +53,7 @@ Future<void> runTasks(
         deviceId: deviceId,
         localEngine: localEngine,
         localEngineSrcPath: localEngineSrcPath,
+        terminateStrayDartProcesses: terminateStrayDartProcesses,
         silent: silent,
         taskArgs: taskArgs,
         resultsPath: resultsPath,
@@ -58,15 +62,17 @@ Future<void> runTasks(
         isolateParams: isolateParams,
       );
 
-      section('Flaky status for "$taskName"');
       if (!result.succeeded) {
-        retry++;
+        retry += 1;
       } else {
+        section('Flaky status for "$taskName"');
         if (retry > 0) {
-          print('Total ${retry+1} executions: $retry failures and 1 success');
+          print('Total ${retry+1} executions: $retry failures and 1 false positive.');
           print('flaky: true');
+          // TODO(ianh): stop ignoring this failure. We should set exitCode=1, and quit
+          // if exitOnFirstTestFailure is true.
         } else {
-          print('Total ${retry+1} executions: 1 success');
+          print('Test passed on first attempt.');
           print('flaky: false');
         }
         break;
@@ -74,7 +80,8 @@ Future<void> runTasks(
     }
 
     if (!result.succeeded) {
-      print('Total $retry executions: 0 success');
+      section('Flaky status for "$taskName"');
+      print('Consistently failed across all $retry executions.');
       print('flaky: false');
       exitCode = 1;
       if (exitOnFirstTestFailure) {
@@ -92,6 +99,7 @@ Future<TaskResult> rerunTask(
   String? deviceId,
   String? localEngine,
   String? localEngineSrcPath,
+  bool terminateStrayDartProcesses = false,
   bool silent = false,
   List<String>? taskArgs,
   String? resultsPath,
@@ -105,6 +113,7 @@ Future<TaskResult> rerunTask(
     deviceId: deviceId,
     localEngine: localEngine,
     localEngineSrcPath: localEngineSrcPath,
+    terminateStrayDartProcesses: terminateStrayDartProcesses,
     silent: silent,
     taskArgs: taskArgs,
     isolateParams: isolateParams,
@@ -138,11 +147,12 @@ Future<TaskResult> rerunTask(
 /// [taskArgs] are passed to the task executable for additional configuration.
 Future<TaskResult> runTask(
   String taskName, {
+  bool terminateStrayDartProcesses = false,
   bool silent = false,
   String? localEngine,
   String? localEngineSrcPath,
   String? deviceId,
-  List<String> ?taskArgs,
+  List<String>? taskArgs,
   @visibleForTesting Map<String, String>? isolateParams,
 }) async {
   final String taskExecutable = 'bin/tasks/$taskName.dart';
@@ -198,17 +208,26 @@ Future<TaskResult> runTask(
 
   try {
     final ConnectionResult result = await _connectToRunnerIsolate(await uri.future);
+    print('[$taskName] Connected to VM server.');
+    isolateParams = isolateParams == null ? <String, String>{} : Map<String, String>.of(isolateParams);
+    isolateParams['runProcessCleanup'] = terminateStrayDartProcesses.toString();
     final Map<String, dynamic> taskResultJson = (await result.vmService.callServiceExtension(
       'ext.cocoonRunTask',
       args: isolateParams,
       isolateId: result.isolate.id,
     )).json!;
     final TaskResult taskResult = TaskResult.fromJson(taskResultJson);
-    await runner.exitCode;
+    final int exitCode = await runner.exitCode;
+    print('[$taskName] Process terminated with exit code $exitCode.');
     return taskResult;
+  } catch (error, stack) {
+    print('[$taskName] Task runner system failed with exception!\n$error\n$stack');
+    rethrow;
   } finally {
-    if (!runnerFinished)
+    if (!runnerFinished) {
+      print('[$taskName] Terminating process...');
       runner.kill(ProcessSignal.sigkill);
+    }
     await stdoutSub.cancel();
     await stderrSub.cancel();
   }

dev/devicelab/lib/framework/running_processes.dart

@@ -9,12 +9,12 @@ import 'package:process/process.dart';
 
 @immutable
 class RunningProcessInfo {
-  const RunningProcessInfo(this.pid, this.creationDate, this.commandLine)
+  const RunningProcessInfo(this.pid, this.commandLine, this.creationDate)
       : assert(pid != null),
         assert(commandLine != null);
 
+  final int pid;
   final String commandLine;
-  final String pid;
   final DateTime creationDate;
 
   @override
@@ -25,57 +25,54 @@ class RunningProcessInfo {
         && other.creationDate == creationDate;
   }
 
-  @override
-  int get hashCode => Object.hash(pid, commandLine, creationDate);
-
-  @override
-  String toString() {
-    return 'RunningProcesses{pid: $pid, commandLine: $commandLine, creationDate: $creationDate}';
-  }
-}
-
-Future<bool> killProcess(String pid, {ProcessManager? processManager}) async {
-  assert(pid != null, 'Must specify a pid to kill');
-  processManager ??= const LocalProcessManager();
-  ProcessResult result;
+  Future<bool> terminate({required ProcessManager processManager}) async {
+    // This returns true when the signal is sent, not when the process goes away.
+    // See also https://github.com/dart-lang/sdk/issues/40759 (killPid should wait for process to be terminated).
     if (Platform.isWindows) {
-    result = await processManager.run(<String>[
+      // TODO(ianh): Move Windows to killPid once we can.
+      //  - killPid on Windows has not-useful return code: https://github.com/dart-lang/sdk/issues/47675
+      final ProcessResult result = await processManager.run(<String>[
           'taskkill.exe',
         '/pid',
-      pid,
+        '$pid',
         '/f',
       ]);
-  } else {
-    result = await processManager.run(<String>[
-      'kill',
-      '-9',
-      pid,
-    ]);
-  }
       return result.exitCode == 0;
+    }
+    return processManager.killPid(pid, ProcessSignal.sigkill);
+  }
+
+  @override
+  int get hashCode => Object.hash(pid, commandLine, creationDate);
+
+  @override
+  String toString() {
+    return 'RunningProcesses(pid: $pid, commandLine: $commandLine, creationDate: $creationDate)';
+  }
 }
 
-Stream<RunningProcessInfo> getRunningProcesses({
+Future<Set<RunningProcessInfo>> getRunningProcesses({
   String? processName,
-  ProcessManager? processManager,
+  required ProcessManager processManager,
 }) {
-  processManager ??= const LocalProcessManager();
   if (Platform.isWindows) {
-    return windowsRunningProcesses(processName);
+    return windowsRunningProcesses(processName, processManager);
   }
   return posixRunningProcesses(processName, processManager);
 }
 
 @visibleForTesting
-Stream<RunningProcessInfo> windowsRunningProcesses(String? processName) async* {
-  // PowerShell script to get the command line arguments and create time of
-  // a process.
+Future<Set<RunningProcessInfo>> windowsRunningProcesses(
+  String? processName,
+  ProcessManager processManager,
+) async {
+  // PowerShell script to get the command line arguments and create time of a process.
   // See: https://docs.microsoft.com/en-us/windows/desktop/cimwin32prov/win32-process
   final String script = processName != null
       ? '"Get-CimInstance Win32_Process -Filter \\"name=\'$processName\'\\" | Select-Object ProcessId,CreationDate,CommandLine | Format-Table -AutoSize | Out-String -Width 4096"'
       : '"Get-CimInstance Win32_Process | Select-Object ProcessId,CreationDate,CommandLine | Format-Table -AutoSize | Out-String -Width 4096"';
-  // Unfortunately, there doesn't seem to be a good way to get ProcessManager to
-  // run this.
+  // TODO(ianh): Unfortunately, there doesn't seem to be a good way to get
+  // ProcessManager to run this.
   final ProcessResult result = await Process.run(
     'powershell -command $script',
     <String>[],
@@ -84,11 +81,9 @@ Stream<RunningProcessInfo> windowsRunningProcesses(String? processName) async* {
     print('Could not list processes!');
     print(result.stderr);
     print(result.stdout);
-    return;
-  }
-  for (final RunningProcessInfo info in processPowershellOutput(result.stdout as String)) {
-    yield info;
+    return <RunningProcessInfo>{};
   }
+  return processPowershellOutput(result.stdout as String).toSet();
 }
 
 /// Parses the output of the PowerShell script from [windowsRunningProcesses].
@@ -149,22 +144,22 @@ Iterable<RunningProcessInfo> processPowershellOutput(String output) sync* {
       time = '${hours + 12}${time.substring(2)}';
     }
 
-    final String pid = line.substring(0, processIdHeaderSize).trim();
+    final int pid = int.parse(line.substring(0, processIdHeaderSize).trim());
     final DateTime creationDate = DateTime.parse('$year-$month-${day}T$time');
     final String commandLine = line.substring(commandLineHeaderStart).trim();
-    yield RunningProcessInfo(pid, creationDate, commandLine);
+    yield RunningProcessInfo(pid, commandLine, creationDate);
   }
 }
 
 @visibleForTesting
-Stream<RunningProcessInfo> posixRunningProcesses(
+Future<Set<RunningProcessInfo>> posixRunningProcesses(
   String? processName,
   ProcessManager processManager,
-) async* {
+) async {
   // Cirrus is missing this in Linux for some reason.
   if (!processManager.canRun('ps')) {
-    print('Cannot list processes on this system: `ps` not available.');
-    return;
+    print('Cannot list processes on this system: "ps" not available.');
+    return <RunningProcessInfo>{};
   }
   final ProcessResult result = await processManager.run(<String>[
     'ps',
@@ -175,11 +170,9 @@ Stream<RunningProcessInfo> posixRunningProcesses(
     print('Could not list processes!');
     print(result.stderr);
     print(result.stdout);
-    return;
-  }
-  for (final RunningProcessInfo info in processPsOutput(result.stdout as String, processName)) {
-    yield info;
+    return <RunningProcessInfo>{};
   }
+  return processPsOutput(result.stdout as String, processName).toSet();
 }
 
 /// Parses the output of the command in [posixRunningProcesses].
@@ -240,8 +233,8 @@ Iterable<RunningProcessInfo> processPsOutput(
     final DateTime creationDate = DateTime.parse('$year-$month-${day}T$time');
     line = line.substring(24).trim();
     final int nextSpace = line.indexOf(' ');
-    final String pid = line.substring(0, nextSpace);
+    final int pid = int.parse(line.substring(0, nextSpace));
     final String commandLine = line.substring(nextSpace + 1);
-    yield RunningProcessInfo(pid, creationDate, commandLine);
+    yield RunningProcessInfo(pid, commandLine, creationDate);
   }
 }

dev/devicelab/lib/framework/utils.dart

@@ -20,14 +20,18 @@ import 'task_result.dart';
 String cwd = Directory.current.path;
 
 /// The local engine to use for [flutter] and [evalFlutter], if any.
-String? get localEngine {
+///
+/// This is set as an environment variable when running the task, see runTask in runner.dart.
+String? get localEngineFromEnv {
   const bool isDefined = bool.hasEnvironment('localEngine');
   return isDefined ? const String.fromEnvironment('localEngine') : null;
 }
 
 /// The local engine source path to use if a local engine is used for [flutter]
 /// and [evalFlutter].
-String? get localEngineSrcPath {
+///
+/// This is set as an environment variable when running the task, see runTask in runner.dart.
+String? get localEngineSrcPathFromEnv {
   const bool isDefined = bool.hasEnvironment('localEngineSrcPath');
   return isDefined ? const String.fromEnvironment('localEngineSrcPath') : null;
 }
@@ -45,8 +49,8 @@ class ProcessInfo {
   @override
   String toString() {
     return '''
-  command : $command
-  started : $startTime
+  command: $command
+  started: $startTime
   pid    : ${process.pid}
 '''
         .trim();
@@ -275,7 +279,7 @@ Future<Process> startProcess(
   final Map<String, String> newEnvironment = Map<String, String>.from(environment ?? <String, String>{});
   newEnvironment['BOT'] = isBot ? 'true' : 'false';
   newEnvironment['LANG'] = 'en_US.UTF-8';
-  print('\nExecuting: $command in $finalWorkingDirectory with environment $newEnvironment');
+  print('Executing "$command" in "$finalWorkingDirectory" with environment $newEnvironment');
   final Process process = await _processManager.start(
     <String>[executable, ...?arguments],
     environment: newEnvironment,
@@ -285,7 +289,6 @@ Future<Process> startProcess(
   _runningProcesses.add(processInfo);
 
   unawaited(process.exitCode.then<void>((int exitCode) {
-    print('"$executable" exit code: $exitCode');
     _runningProcesses.remove(processInfo);
   }));
 
@@ -303,7 +306,7 @@ Future<void> forceQuitRunningProcesses() async {
   for (final ProcessInfo p in _runningProcesses) {
     print('Force-quitting process:\n$p');
     if (!p.process.kill()) {
-      print('Failed to force quit process');
+      print('Failed to force quit process.');
     }
   }
   _runningProcesses.clear();
@@ -436,6 +439,8 @@ List<String> flutterCommandArgs(String command, List<String> options) {
     'run',
     'screenshot',
   };
+  final String? localEngine = localEngineFromEnv;
+  final String? localEngineSrcPath = localEngineSrcPathFromEnv;
   return <String>[
     command,
     if (deviceOperatingSystem == DeviceOperatingSystem.ios && supportedDeviceTimeoutCommands.contains(command))
@@ -448,8 +453,8 @@ List<String> flutterCommandArgs(String command, List<String> options) {
       '--screenshot',
       hostAgent.dumpDirectory!.path,
     ],
-    if (localEngine != null) ...<String>['--local-engine', localEngine!],
-    if (localEngineSrcPath != null) ...<String>['--local-engine-src-path', localEngineSrcPath!],
+    if (localEngine != null) ...<String>['--local-engine', localEngine],
+    if (localEngineSrcPath != null) ...<String>['--local-engine-src-path', localEngineSrcPath],
     ...options,
   ];
 }

dev/devicelab/lib/tasks/perf_tests.dart

@@ -847,12 +847,14 @@ class PerfTest {
       final Device device = await devices.workingDevice;
       await device.unlock();
       final String deviceId = device.deviceId;
+      final String? localEngine = localEngineFromEnv;
+      final String? localEngineSrcPath = localEngineSrcPathFromEnv;
 
       await flutter('drive', options: <String>[
         if (localEngine != null)
-          ...<String>['--local-engine', localEngine!],
+          ...<String>['--local-engine', localEngine],
         if (localEngineSrcPath != null)
-          ...<String>['--local-engine-src-path', localEngineSrcPath!],
+          ...<String>['--local-engine-src-path', localEngineSrcPath],
         '--no-dds',
         '--no-android-gradle-daemon',
         '-v',
@@ -1028,15 +1030,16 @@ class PerfTestWithSkSL extends PerfTest {
     if (File(_vmserviceFileName).existsSync()) {
       File(_vmserviceFileName).deleteSync();
     }
-
+    final String? localEngine = localEngineFromEnv;
+    final String? localEngineSrcPath = localEngineSrcPathFromEnv;
     _runProcess = await startProcess(
       _flutterPath,
       <String>[
         'run',
         if (localEngine != null)
-          ...<String>['--local-engine', localEngine!],
+          ...<String>['--local-engine', localEngine],
         if (localEngineSrcPath != null)
-          ...<String>['--local-engine-src-path', localEngineSrcPath!],
+          ...<String>['--local-engine-src-path', localEngineSrcPath],
         '--no-dds',
         if (deviceOperatingSystem == DeviceOperatingSystem.ios)
           ...<String>[

dev/devicelab/test/run_test.dart

@@ -4,6 +4,7 @@
 
 import 'dart:io';
 
+import 'package:flutter_devicelab/framework/utils.dart' show rm;
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
 
@@ -20,6 +21,7 @@ void main() {
       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],
       ]);
@@ -87,9 +89,14 @@ void main() {
 
 
     test('runs A/B test', () async {
+      final Directory tempDirectory = Directory.systemTemp.createTempSync('flutter_devicelab_ab_test.');
+      final File abResultsFile = File(path.join(tempDirectory.path, 'test_results.json'));
+
+      expect(abResultsFile.existsSync(), isFalse);
+
       final ProcessResult result = await runScript(
         <String>['smoke_test_success'],
-        <String>['--ab=2', '--local-engine=host_debug_unopt'],
+        <String>['--ab=2', '--local-engine=host_debug_unopt', '--ab-result-file', abResultsFile.path],
       );
       expect(result.exitCode, 0);
 
@@ -137,6 +144,9 @@ void main() {
           'metric2\t123.00 (0.00%)\t123.00 (0.00%)\t1.00x\t\n',
         ),
       );
+
+      expect(abResultsFile.existsSync(), isTrue);
+      rm(tempDirectory, recursive: true);
     });
 
     test('fails to upload results to Cocoon if flags given', () async {

dev/devicelab/test/runner_test.dart

@@ -11,7 +11,6 @@ import 'common.dart';
 void main() {
   final Map<String, String> isolateParams = <String, String>{
     'runFlutterConfig': 'false',
-    'runProcessCleanup': 'false',
     'timeoutInMinutes': '1',
   };
   List<String> printLog;
@@ -27,7 +26,7 @@ void main() {
         logs: printLog,
       );
       expect(printLog.length, 2);
-      expect(printLog[0], 'Total 1 executions: 1 success');
+      expect(printLog[0], 'Test passed on first attempt.');
       expect(printLog[1], 'flaky: false');
     });
 
@@ -40,7 +39,7 @@ void main() {
         logs: printLog,
       );
       expect(printLog.length, 2);
-      expect(printLog[0], 'Total 3 executions: 0 success');
+      expect(printLog[0], 'Consistently failed across all 3 executions.');
       expect(printLog[1], 'flaky: false');
     });
   });

dev/devicelab/test/running_processes_test.dart

@@ -2,7 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:async';
+import 'dart:convert';
+import 'dart:io';
+
 import 'package:flutter_devicelab/framework/running_processes.dart';
+import 'package:process/process.dart';
+
 import 'common.dart';
 
 void main() {
@@ -24,19 +30,19 @@ ProcessId CreationDate         CommandLine
         results,
         equals(<RunningProcessInfo>[
           RunningProcessInfo(
-            '6552',
-            DateTime(2019, 7, 3, 17, 0, 27),
+            6552,
             r'"C:\tools\dart-sdk\bin\dart.exe" .\bin\agent.dart ci',
+            DateTime(2019, 7, 3, 17, 0, 27),
           ),
           RunningProcessInfo(
-            '6553',
-            DateTime(2019, 7, 3, 22, 0, 27),
+            6553,
             r'"C:\tools\dart-sdk1\bin\dart.exe" .\bin\agent.dart ci',
+            DateTime(2019, 7, 3, 22, 0, 27),
           ),
           RunningProcessInfo(
-            '6554',
-            DateTime(2019, 7, 3, 11, 0, 27),
+            6554,
             r'"C:\tools\dart-sdk2\bin\dart.exe" .\bin\agent.dart ci',
+            DateTime(2019, 7, 3, 11, 0, 27),
           ),
         ]));
   });
@@ -55,15 +61,81 @@ Sat Mar  9 20:13:00 2019        49 /usr/sbin/syslogd
         results,
         equals(<RunningProcessInfo>[
           RunningProcessInfo(
-            '1',
-            DateTime(2019, 3, 9, 20, 12, 47),
+            1,
             '/sbin/launchd',
+            DateTime(2019, 3, 9, 20, 12, 47),
           ),
           RunningProcessInfo(
-            '49',
-            DateTime(2019, 3, 9, 20, 13),
+            49,
             '/usr/sbin/syslogd',
+            DateTime(2019, 3, 9, 20, 13),
           ),
         ]));
   });
+
+  test('RunningProcessInfo.terminate', () {
+    final RunningProcessInfo process = RunningProcessInfo(123, 'test', DateTime(456));
+    final FakeProcessManager fakeProcessManager = FakeProcessManager();
+    process.terminate(processManager: fakeProcessManager);
+    if (Platform.isWindows) {
+      expect(fakeProcessManager.log, <String>['run([taskkill.exe, /pid, 123, /f], null, null, null, null, null, null)']);
+    } else {
+      expect(fakeProcessManager.log, <String>['killPid(123, SIGKILL)']);
+    }
+  });
+}
+
+class FakeProcessManager implements ProcessManager {
+  final List<String> log = <String>[];
+
+  @override
+  bool canRun(Object? a, { String? workingDirectory }) {
+    log.add('canRun($a, $workingDirectory)');
+    return true;
+  }
+
+  @override
+  bool killPid(int a, [ProcessSignal? b]) {
+    log.add('killPid($a, $b)');
+    return true;
+  }
+
+  @override
+  Future<ProcessResult> run(List<Object> a, {
+    Map<String, String>? environment,
+    bool? includeParentEnvironment,
+    bool? runInShell,
+    Encoding? stderrEncoding,
+    Encoding? stdoutEncoding,
+    String? workingDirectory,
+  }) async {
+    log.add('run($a, $environment, $includeParentEnvironment, $runInShell, $stderrEncoding, $stdoutEncoding, $workingDirectory)');
+    return ProcessResult(1, 0, 'stdout', 'stderr');
+  }
+
+  @override
+  ProcessResult runSync(List<Object> a, {
+    Map<String, String>? environment,
+    bool? includeParentEnvironment,
+    bool? runInShell,
+    Encoding? stderrEncoding,
+    Encoding? stdoutEncoding,
+    String? workingDirectory,
+  }) {
+    log.add('runSync($a, $environment, $includeParentEnvironment, $runInShell, $stderrEncoding, $stdoutEncoding, $workingDirectory)');
+    return ProcessResult(1, 0, 'stdout', 'stderr');
+  }
+
+  @override
+  Future<Process> start(
+    List<Object> a, {
+    Map<String, String>? environment,
+    bool? includeParentEnvironment,
+    ProcessStartMode? mode,
+    bool? runInShell,
+    String? workingDirectory,
+  }) {
+    log.add('start($a, $environment, $includeParentEnvironment, $mode, $runInShell, $workingDirectory)');
+    return Completer<Process>().future;
+  }
 }

dev/devicelab/test/tasks/build_test_task_test.dart

@@ -12,7 +12,6 @@ import '../common.dart';
 void main() {
   final Map<String, String> isolateParams = <String, String>{
     'runFlutterConfig': 'false',
-    'runProcessCleanup': 'false',
     'timeoutInMinutes': '1',
   };
 
@@ -66,7 +65,7 @@ void main() {
     final String capturedPrint = capturedPrintLines.toString();
     expect(capturedPrint,
         contains('with environment {FLUTTER_DEVICELAB_DEVICEID: FAKE_SUCCESS, BOT: true, LANG: en_US.UTF-8}'));
-    expect(capturedPrint, contains('exit code: 0'));
+    expect(capturedPrint, contains('Process terminated with exit code 0.'));
   });
 
   test('throws exception when build and test arg are given', () async {

dev/devicelab/test/utils_test.dart

@@ -36,8 +36,8 @@ void main() {
 
   group('engine environment declarations', () {
     test('localEngine', () {
-      expect(localEngine, null);
-      expect(localEngineSrcPath, null);
+      expect(localEngineFromEnv, null);
+      expect(localEngineSrcPathFromEnv, null);
     });
   });
 }