Refactor devicelab bin/run.dart and other cleanup (#96320)
diff --git a/dev/devicelab/bin/run.dart b/dev/devicelab/bin/run.dart
index 515b3a1..d4589bd 100644
--- a/dev/devicelab/bin/run.dart
+++ b/dev/devicelab/bin/run.dart
@@ -13,102 +13,108 @@
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;
-
-/// The git branch being tested on.
-String? gitBranch;
-
-/// The build of the local engine to use.
-///
-/// Required for A/B test mode.
-String? localEngine;
-
-/// The path to the engine "src/" directory.
-String? localEngineSrcPath;
-
-/// 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;
-
-/// Whether to exit on first test failure.
-bool exitOnFirstTestFailure = false;
-
-/// Path to write test results to.
-String? resultsPath;
-
-/// File containing a service account token.
-///
-/// If passed, the test run results will be uploaded to Flutter infrastructure.
-String? serviceAccountTokenFile;
-
-/// Suppresses standard output, prints only standard error output.
-bool silent = false;
-
/// Runs tasks.
///
-/// The tasks are chosen depending on the command-line options
-/// (see [_argParser]).
+/// 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);
+
+ ArgResults args;
try {
- args = _argParser.parse(rawArgs);
+ 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);
- exitCode = 1;
- return;
+ stderr.writeln(argParser.usage);
+ exit(1);
}
- 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;
+ /// Suppresses standard output, prints only standard error output.
+ final bool silent = (args['silent'] as bool?) ?? false;
+
+ /// The build of the local engine to use.
+ ///
+ /// Required for A/B test mode.
+ final String? localEngine = args['local-engine'] as String?;
+
+ /// The path to the engine "src/" directory.
+ final String? localEngineSrcPath = args['local-engine-src-path'] as String?;
+
+ /// The device-id to run test on.
+ final String? deviceId = args['device-id'] as String?;
+
+ /// Whether to exit on first test failure.
+ final bool exitOnFirstTestFailure = (args['exit'] as bool?) ?? false;
+
+ /// Whether to tell tasks to clean up after themselves.
+ final bool terminateStrayDartProcesses = (args['terminate-stray-dart-processes'] as bool?) ?? false;
+
+ /// The git branch being tested on.
+ final String? gitBranch = args['git-branch'] 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.
+ final String? luciBuilder = args['luci-builder'] as String?;
+
+ /// 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> _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 @@
}
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,142 +232,152 @@
}
}
-/// Command-line options for the `run.dart` command.
-final ArgParser _argParser = ArgParser()
- ..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'
- '\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');
+ArgParser createArgParser(List<String> taskNames) {
+ return ArgParser()
+ ..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'
+ '\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));
+ 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));
+ }
}
- }
- },
- )
- ..addOption(
- 'device-id',
- abbr: 'd',
- help: 'Target device id (prefixes are allowed, names are not supported).\n'
- 'The option will be ignored if the test target does not run on a\n'
- 'mobile device. This still respects the device operating system\n'
- 'settings in the test case, and will results in error if no device\n'
- 'with given ID/ID prefix is found.',
- )
- ..addOption(
- 'ab',
- help: 'Runs an A/B test comparing the default engine with the local\n'
- 'engine build for one task. This option does not support running\n'
- 'multiple tasks. The value is the number of times to run the task.\n'
- 'The task is expected to be a benchmark that reports score keys.\n'
- 'The A/B test collects the metrics collected by the test and\n'
- 'produces a report containing averages, noise, and the speed-up\n'
- 'between the two engines. --local-engine is required when running\n'
- 'an A/B test.',
- callback: (String? value) {
- if (value != null && int.tryParse(value) == null) {
- throw ArgParserException('Option --ab must be a number, but was "$value".');
- }
- },
- )
- ..addOption(
- 'ab-result-file',
- help: 'The filename in which to place the json encoded results of an A/B test.\n'
- 'The filename may contain a single # character to be replaced by a sequence\n'
- 'number if the name already exists.',
- )
- ..addFlag(
- 'all',
- abbr: 'a',
- help: 'Runs all tasks defined in manifest.yaml in alphabetical order.',
- )
- ..addOption(
- 'continue-from',
- abbr: 'c',
- help: 'With --all or --stage, continue from the given test.',
- )
- ..addFlag(
- 'exit',
- defaultsTo: true,
- help: 'Exit on the first test failure.',
- )
- ..addOption(
- 'git-branch',
- help: '[Flutter infrastructure] Git branch of the current commit. LUCI\n'
- 'checkouts run in detached HEAD state, so the branch must be passed.',
- )
- ..addOption(
- 'local-engine',
- help: 'Name of a build output within the engine out directory, if you\n'
- 'are building Flutter locally. Use this to select a specific\n'
- 'version of the engine if you have built multiple engine targets.\n'
- 'This path is relative to --local-engine-src-path/out. This option\n'
- 'is required when running an A/B test (see the --ab option).',
- )
- ..addFlag(
- 'list',
- abbr: 'l',
- help: "Don't actually run the tasks, but list out the tasks that would\n"
- 'have been run, in the order they would have run.',
- )
- ..addOption(
- 'local-engine-src-path',
- help: 'Path to your engine src directory, if you are building Flutter\n'
- 'locally. Defaults to \$FLUTTER_ENGINE if set, or tries to guess at\n'
- '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'
- 'task, will write test results to the file.'
- )
- ..addOption(
- 'service-account-token-file',
- help: '[Flutter infrastructure] Authentication for uploading results.',
- )
- ..addOption(
- 'stage',
- abbr: 's',
- help: 'Name of the stage. Runs all tasks for that stage. The tasks and\n'
- 'their stages are read from manifest.yaml.',
- )
- ..addFlag(
- 'silent',
- )
- ..addMultiOption(
- 'test',
- hide: true,
- callback: (List<String> value) {
- if (value.isNotEmpty) {
- throw const FormatException(
- 'Invalid option --test. Did you mean --task (-t)?',
- );
- }
- },
- );
+ },
+ )
+ ..addOption(
+ 'device-id',
+ abbr: 'd',
+ help: 'Target device id (prefixes are allowed, names are not supported).\n'
+ 'The option will be ignored if the test target does not run on a\n'
+ 'mobile device. This still respects the device operating system\n'
+ 'settings in the test case, and will results in error if no device\n'
+ 'with given ID/ID prefix is found.',
+ )
+ ..addOption(
+ 'ab',
+ help: 'Runs an A/B test comparing the default engine with the local\n'
+ 'engine build for one task. This option does not support running\n'
+ 'multiple tasks. The value is the number of times to run the task.\n'
+ 'The task is expected to be a benchmark that reports score keys.\n'
+ 'The A/B test collects the metrics collected by the test and\n'
+ 'produces a report containing averages, noise, and the speed-up\n'
+ 'between the two engines. --local-engine is required when running\n'
+ 'an A/B test.',
+ callback: (String? value) {
+ if (value != null && int.tryParse(value) == null) {
+ throw ArgParserException('Option --ab must be a number, but was "$value".');
+ }
+ },
+ )
+ ..addOption(
+ 'ab-result-file',
+ help: 'The filename in which to place the json encoded results of an A/B test.\n'
+ 'The filename may contain a single # character to be replaced by a sequence\n'
+ 'number if the name already exists.',
+ )
+ ..addFlag(
+ 'all',
+ abbr: 'a',
+ help: 'Runs all tasks defined in manifest.yaml in alphabetical order.',
+ )
+ ..addOption(
+ 'continue-from',
+ abbr: 'c',
+ help: 'With --all or --stage, continue from the given test.',
+ )
+ ..addFlag(
+ 'exit',
+ defaultsTo: true,
+ help: 'Exit on the first test failure. Currently flakes are intentionally (though '
+ 'incorrectly) not considered to be failures.',
+ )
+ ..addOption(
+ 'git-branch',
+ help: '[Flutter infrastructure] Git branch of the current commit. LUCI\n'
+ 'checkouts run in detached HEAD state, so the branch must be passed.',
+ )
+ ..addOption(
+ 'local-engine',
+ help: 'Name of a build output within the engine out directory, if you\n'
+ 'are building Flutter locally. Use this to select a specific\n'
+ 'version of the engine if you have built multiple engine targets.\n'
+ 'This path is relative to --local-engine-src-path/out. This option\n'
+ 'is required when running an A/B test (see the --ab option).',
+ )
+ ..addFlag(
+ 'list',
+ abbr: 'l',
+ help: "Don't actually run the tasks, but list out the tasks that would\n"
+ 'have been run, in the order they would have run.',
+ )
+ ..addOption(
+ 'local-engine-src-path',
+ help: 'Path to your engine src directory, if you are building Flutter\n'
+ 'locally. Defaults to \$FLUTTER_ENGINE if set, or tries to guess at\n'
+ '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'
+ 'task, will write test results to the file.'
+ )
+ ..addOption(
+ 'service-account-token-file',
+ help: '[Flutter infrastructure] Authentication for uploading results.',
+ )
+ ..addOption(
+ 'stage',
+ abbr: 's',
+ help: 'Name of the stage. Runs all tasks for that stage. The tasks and\n'
+ 'their stages are read from manifest.yaml.',
+ )
+ ..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',
+ hide: true,
+ callback: (List<String> value) {
+ if (value.isNotEmpty) {
+ throw const FormatException(
+ 'Invalid option --test. Did you mean --task (-t)?',
+ );
+ }
+ },
+ );
+}
diff --git a/dev/devicelab/lib/framework/framework.dart b/dev/devicelab/lib/framework/framework.dart
index 1c9f689..a81f035 100644
--- a/dev/devicelab/lib/framework/framework.dart
+++ b/dev/devicelab/lib/framework/framework.dart
@@ -10,6 +10,7 @@
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 @@
///
/// 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 @@
}
final TaskFunction task;
+ final ProcessManager processManager;
Future<Device?> _getWorkingDeviceIfAvailable() async {
try {
@@ -103,6 +113,7 @@
Future<TaskResult> run(Duration? taskTimeout, {
bool runFlutterConfig = true,
bool runProcessCleanup = true,
+ required String? localEngine,
}) async {
try {
_taskStarted = true;
@@ -113,20 +124,19 @@
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 @@
'--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 @@
}
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;
diff --git a/dev/devicelab/lib/framework/runner.dart b/dev/devicelab/lib/framework/runner.dart
index de7cf5d..56aa781 100644
--- a/dev/devicelab/lib/framework/runner.dart
+++ b/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 @@
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 @@
deviceId: deviceId,
localEngine: localEngine,
localEngineSrcPath: localEngineSrcPath,
+ terminateStrayDartProcesses: terminateStrayDartProcesses,
silent: silent,
taskArgs: taskArgs,
resultsPath: resultsPath,
@@ -58,15 +62,17 @@
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 @@
}
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 @@
String? deviceId,
String? localEngine,
String? localEngineSrcPath,
+ bool terminateStrayDartProcesses = false,
bool silent = false,
List<String>? taskArgs,
String? resultsPath,
@@ -105,6 +113,7 @@
deviceId: deviceId,
localEngine: localEngine,
localEngineSrcPath: localEngineSrcPath,
+ terminateStrayDartProcesses: terminateStrayDartProcesses,
silent: silent,
taskArgs: taskArgs,
isolateParams: isolateParams,
@@ -138,11 +147,12 @@
/// [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 @@
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();
}
diff --git a/dev/devicelab/lib/framework/running_processes.dart b/dev/devicelab/lib/framework/running_processes.dart
index 07a5e52..96b6e59 100644
--- a/dev/devicelab/lib/framework/running_processes.dart
+++ b/dev/devicelab/lib/framework/running_processes.dart
@@ -9,12 +9,12 @@
@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 @@
&& other.creationDate == creationDate;
}
+ 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) {
+ // 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',
+ '/f',
+ ]);
+ 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}';
+ 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;
- if (Platform.isWindows) {
- result = await processManager.run(<String>[
- 'taskkill.exe',
- '/pid',
- pid,
- '/f',
- ]);
- } else {
- result = await processManager.run(<String>[
- 'kill',
- '-9',
- pid,
- ]);
- }
- return result.exitCode == 0;
-}
-
-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 @@
print('Could not list processes!');
print(result.stderr);
print(result.stdout);
- return;
+ return <RunningProcessInfo>{};
}
- for (final RunningProcessInfo info in processPowershellOutput(result.stdout as String)) {
- yield info;
- }
+ return processPowershellOutput(result.stdout as String).toSet();
}
/// Parses the output of the PowerShell script from [windowsRunningProcesses].
@@ -149,22 +144,22 @@
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 @@
print('Could not list processes!');
print(result.stderr);
print(result.stdout);
- return;
+ return <RunningProcessInfo>{};
}
- for (final RunningProcessInfo info in processPsOutput(result.stdout as String, processName)) {
- yield info;
- }
+ return processPsOutput(result.stdout as String, processName).toSet();
}
/// Parses the output of the command in [posixRunningProcesses].
@@ -240,8 +233,8 @@
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);
}
}
diff --git a/dev/devicelab/lib/framework/utils.dart b/dev/devicelab/lib/framework/utils.dart
index f5d960b..b1ef715 100644
--- a/dev/devicelab/lib/framework/utils.dart
+++ b/dev/devicelab/lib/framework/utils.dart
@@ -20,14 +20,18 @@
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,9 +49,9 @@
@override
String toString() {
return '''
- command : $command
- started : $startTime
- pid : ${process.pid}
+ command: $command
+ started: $startTime
+ pid : ${process.pid}
'''
.trim();
}
@@ -275,7 +279,7 @@
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 @@
_runningProcesses.add(processInfo);
unawaited(process.exitCode.then<void>((int exitCode) {
- print('"$executable" exit code: $exitCode');
_runningProcesses.remove(processInfo);
}));
@@ -303,7 +306,7 @@
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();
@@ -349,7 +352,7 @@
stderr: stderr,
printStdout: printStdout,
printStderr: printStderr,
- );
+ );
final int exitCode = await process.exitCode;
if (exitCode != 0 && !canFail)
@@ -373,23 +376,23 @@
final Completer<void> stdoutDone = Completer<void>();
final Completer<void> stderrDone = Completer<void>();
process.stdout
- .transform<String>(utf8.decoder)
- .transform<String>(const LineSplitter())
- .listen((String line) {
- if (printStdout) {
- print('stdout: $line');
- }
- output?.writeln(line);
- }, onDone: () { stdoutDone.complete(); });
+ .transform<String>(utf8.decoder)
+ .transform<String>(const LineSplitter())
+ .listen((String line) {
+ if (printStdout) {
+ print('stdout: $line');
+ }
+ output?.writeln(line);
+ }, onDone: () { stdoutDone.complete(); });
process.stderr
- .transform<String>(utf8.decoder)
- .transform<String>(const LineSplitter())
- .listen((String line) {
- if (printStderr) {
- print('stderr: $line');
- }
- stderr?.writeln(line);
- }, onDone: () { stderrDone.complete(); });
+ .transform<String>(utf8.decoder)
+ .transform<String>(const LineSplitter())
+ .listen((String line) {
+ if (printStderr) {
+ print('stderr: $line');
+ }
+ stderr?.writeln(line);
+ }, onDone: () { stderrDone.complete(); });
return Future.wait<void>(<Future<void>>[
stdoutDone.future,
@@ -436,6 +439,8 @@
'run',
'screenshot',
};
+ final String? localEngine = localEngineFromEnv;
+ final String? localEngineSrcPath = localEngineSrcPathFromEnv;
return <String>[
command,
if (deviceOperatingSystem == DeviceOperatingSystem.ios && supportedDeviceTimeoutCommands.contains(command))
@@ -448,8 +453,8 @@
'--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,
];
}
diff --git a/dev/devicelab/lib/tasks/perf_tests.dart b/dev/devicelab/lib/tasks/perf_tests.dart
index 79ce804..306464e 100644
--- a/dev/devicelab/lib/tasks/perf_tests.dart
+++ b/dev/devicelab/lib/tasks/perf_tests.dart
@@ -847,12 +847,14 @@
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 @@
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>[
diff --git a/dev/devicelab/test/run_test.dart b/dev/devicelab/test/run_test.dart
index 61ff3ea..a5332dd 100644
--- a/dev/devicelab/test/run_test.dart
+++ b/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 @@
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 @@
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 @@
'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 {
diff --git a/dev/devicelab/test/runner_test.dart b/dev/devicelab/test/runner_test.dart
index 661a961..baee698 100644
--- a/dev/devicelab/test/runner_test.dart
+++ b/dev/devicelab/test/runner_test.dart
@@ -11,7 +11,6 @@
void main() {
final Map<String, String> isolateParams = <String, String>{
'runFlutterConfig': 'false',
- 'runProcessCleanup': 'false',
'timeoutInMinutes': '1',
};
List<String> printLog;
@@ -27,7 +26,7 @@
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 @@
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');
});
});
diff --git a/dev/devicelab/test/running_processes_test.dart b/dev/devicelab/test/running_processes_test.dart
index 76f3df0..100b3dd 100644
--- a/dev/devicelab/test/running_processes_test.dart
+++ b/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 @@
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 @@
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;
+ }
}
diff --git a/dev/devicelab/test/tasks/build_test_task_test.dart b/dev/devicelab/test/tasks/build_test_task_test.dart
index 5cc8b98..70bb86b 100644
--- a/dev/devicelab/test/tasks/build_test_task_test.dart
+++ b/dev/devicelab/test/tasks/build_test_task_test.dart
@@ -12,7 +12,6 @@
void main() {
final Map<String, String> isolateParams = <String, String>{
'runFlutterConfig': 'false',
- 'runProcessCleanup': 'false',
'timeoutInMinutes': '1',
};
@@ -66,7 +65,7 @@
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 {
diff --git a/dev/devicelab/test/utils_test.dart b/dev/devicelab/test/utils_test.dart
index 06db6a1..ca3e12b 100644
--- a/dev/devicelab/test/utils_test.dart
+++ b/dev/devicelab/test/utils_test.dart
@@ -36,8 +36,8 @@
group('engine environment declarations', () {
test('localEngine', () {
- expect(localEngine, null);
- expect(localEngineSrcPath, null);
+ expect(localEngineFromEnv, null);
+ expect(localEngineSrcPathFromEnv, null);
});
});
}
