Refactor command utilities for tests (#68324)

* refactor command running utilities for tests
Co-authored-by: Jonah Williams <jonahwilliams@google.com>

dev/bots/run_command.dart

@@ -2,92 +2,113 @@
 // 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:core' hide print;
-import 'dart:io' hide exit;
+import 'dart:io' as io;
 
 import 'package:path/path.dart' as path;
 
 import 'utils.dart';
 
-// TODO(ianh): These two functions should be refactored into something that avoids all this code duplication.
-
+/// Runs the `executable` and returns standard output as a stream of lines.
+///
+/// The returned stream reaches its end immediately after the command exits.
+///
+/// If `expectNonZeroExit` is false and the process exits with a non-zero exit
+/// code fails the test immediately by exiting the test process with exit code
+/// 1.
 Stream<String> runAndGetStdout(String executable, List<String> arguments, {
   String workingDirectory,
   Map<String, String> environment,
   bool expectNonZeroExit = false,
-  int expectedExitCode,
-  String failureMessage,
-  bool skip = false,
 }) async* {
-  final String commandDescription = '${path.relative(executable, from: workingDirectory)} ${arguments.join(' ')}';
-  final String relativeWorkingDir = path.relative(workingDirectory);
-  if (skip) {
-    printProgress('SKIPPING', relativeWorkingDir, commandDescription);
-    return;
-  }
-  printProgress('RUNNING', relativeWorkingDir, commandDescription);
-
-  final Stopwatch time = Stopwatch()..start();
-  final Process process = await Process.start(executable, arguments,
+  final StreamController<String> output = StreamController<String>();
+  final Future<CommandResult> command = runCommand(
+    executable,
+    arguments,
     workingDirectory: workingDirectory,
     environment: environment,
+    expectNonZeroExit: expectNonZeroExit,
+    // Capture the output so it's not printed to the console by default.
+    outputMode: OutputMode.capture,
+    outputListener: (String line, io.Process process) {
+      output.add(line);
+    },
   );
 
-  stderr.addStream(process.stderr);
-  final Stream<String> lines = process.stdout.transform(utf8.decoder).transform(const LineSplitter());
-  yield* lines;
+  // Close the stream controller after the command is complete. Otherwise,
+  // the yield* will never finish.
+  command.whenComplete(output.close);
 
-  final int exitCode = await process.exitCode;
-  if ((exitCode == 0) == expectNonZeroExit || (expectedExitCode != null && exitCode != expectedExitCode)) {
-    exitWithError(<String>[
-      if (failureMessage != null)
-        failureMessage
-      else
-        '${bold}ERROR: ${red}Last command exited with $exitCode (expected: ${expectNonZeroExit ? (expectedExitCode ?? 'non-zero') : 'zero'}).$reset',
-      '${bold}Command: $green$commandDescription$reset',
-      '${bold}Relative working directory: $cyan$relativeWorkingDir$reset',
-    ]);
+  yield* output.stream;
+}
+
+/// Represents a running process launched using [startCommand].
+class Command {
+  Command._(this.process, this._time, this._savedStdout, this._savedStderr);
+
+  /// The raw process that was launched for this command.
+  final io.Process process;
+
+  final Stopwatch _time;
+  final Future<List<List<int>>> _savedStdout;
+  final Future<List<List<int>>> _savedStderr;
+
+  /// Evaluates when the [process] exits.
+  ///
+  /// Returns the result of running the command.
+  Future<CommandResult> get onExit async {
+    final int exitCode = await process.exitCode;
+    _time.stop();
+
+    // Saved output is null when OutputMode.print is used.
+    final String flattenedStdout = _savedStdout != null ? _flattenToString(await _savedStdout) : null;
+    final String flattenedStderr = _savedStderr != null ? _flattenToString(await _savedStderr) : null;
+    return CommandResult._(exitCode, _time.elapsed, flattenedStdout, flattenedStderr);
   }
-  print('$clock ELAPSED TIME: ${prettyPrintDuration(time.elapsed)} for $green$commandDescription$reset in $cyan$relativeWorkingDir$reset');
 }
 
-/// Runs the `executable` and waits until the process exits.
-///
-/// If the process exits with a non-zero exit code, exits this process with
-/// exit code 1, unless `expectNonZeroExit` is set to true.
+/// The result of running a command using [startCommand] and [runCommand];
+class CommandResult {
+  CommandResult._(this.exitCode, this.elapsedTime, this.flattenedStdout, this.flattenedStderr);
+
+  /// The exit code of the process.
+  final int exitCode;
+
+  /// The amount of time it took the process to complete.
+  final Duration elapsedTime;
+
+  /// Standard output decoded as a string using UTF8 decoder.
+  final String flattenedStdout;
+
+  /// Standard error output decoded as a string using UTF8 decoder.
+  final String flattenedStderr;
+}
+
+/// Starts the `executable` and returns a command object representing the
+/// running process.
 ///
 /// `outputListener` is called for every line of standard output from the
 /// process, and is given the [Process] object. This can be used to interrupt
 /// an indefinitely running process, for example, by waiting until the process
 /// emits certain output.
-Future<void> runCommand(String executable, List<String> arguments, {
+///
+/// `outputMode` controls where the standard output from the command process
+/// goes. See [OutputMode].
+Future<Command> startCommand(String executable, List<String> arguments, {
   String workingDirectory,
   Map<String, String> environment,
-  bool expectNonZeroExit = false,
-  int expectedExitCode,
-  String failureMessage,
   OutputMode outputMode = OutputMode.print,
-  CapturedOutput output,
-  bool skip = false,
   bool Function(String) removeLine,
-  void Function(String, Process) outputListener,
+  void Function(String, io.Process) outputListener,
 }) async {
-  assert(
-    (outputMode == OutputMode.capture) == (output != null),
-    'The output parameter must be non-null with and only with OutputMode.capture',
-  );
-
   final String commandDescription = '${path.relative(executable, from: workingDirectory)} ${arguments.join(' ')}';
-  final String relativeWorkingDir = path.relative(workingDirectory ?? Directory.current.path);
-  if (skip) {
-    printProgress('SKIPPING', relativeWorkingDir, commandDescription);
-    return;
-  }
+  final String relativeWorkingDir = path.relative(workingDirectory ?? io.Directory.current.path);
   printProgress('RUNNING', relativeWorkingDir, commandDescription);
 
   final Stopwatch time = Stopwatch()..start();
-  final Process process = await Process.start(executable, arguments,
+  final io.Process process = await io.Process.start(executable, arguments,
     workingDirectory: workingDirectory,
     environment: environment,
   );
@@ -108,56 +129,103 @@ Future<void> runCommand(String executable, List<String> arguments, {
   switch (outputMode) {
     case OutputMode.print:
       await Future.wait<void>(<Future<void>>[
-        stdout.addStream(stdoutSource),
-        stderr.addStream(process.stderr),
+        io.stdout.addStream(stdoutSource),
+        io.stderr.addStream(process.stderr),
       ]);
       break;
     case OutputMode.capture:
-    case OutputMode.discard:
       savedStdout = stdoutSource.toList();
       savedStderr = process.stderr.toList();
       break;
   }
 
-  final int exitCode = await process.exitCode;
-  if (output != null) {
-    output.stdout = _flattenToString(await savedStdout);
-    output.stderr = _flattenToString(await savedStderr);
+  return Command._(process, time, savedStdout, savedStderr);
+}
+
+/// Runs the `executable` and waits until the process exits.
+///
+/// If the process exits with a non-zero exit code, exits this process with
+/// exit code 1, unless `expectNonZeroExit` is set to true.
+///
+/// `outputListener` is called for every line of standard output from the
+/// process, and is given the [Process] object. This can be used to interrupt
+/// an indefinitely running process, for example, by waiting until the process
+/// emits certain output.
+///
+/// Returns the result of the finished process, or null if `skip` is true.
+///
+/// `outputMode` controls where the standard output from the command process
+/// goes. See [OutputMode].
+Future<CommandResult> runCommand(String executable, List<String> arguments, {
+  String workingDirectory,
+  Map<String, String> environment,
+  bool expectNonZeroExit = false,
+  int expectedExitCode,
+  String failureMessage,
+  OutputMode outputMode = OutputMode.print,
+  bool skip = false,
+  bool Function(String) removeLine,
+  void Function(String, io.Process) outputListener,
+}) async {
+  final String commandDescription = '${path.relative(executable, from: workingDirectory)} ${arguments.join(' ')}';
+  final String relativeWorkingDir = path.relative(workingDirectory ?? io.Directory.current.path);
+  if (skip) {
+    printProgress('SKIPPING', relativeWorkingDir, commandDescription);
+    return null;
   }
 
-  if ((exitCode == 0) == expectNonZeroExit || (expectedExitCode != null && exitCode != expectedExitCode)) {
+  final Command command = await startCommand(executable, arguments,
+    workingDirectory: workingDirectory,
+    environment: environment,
+    outputMode: outputMode,
+    removeLine: removeLine,
+    outputListener: outputListener,
+  );
+
+  final CommandResult result = await command.onExit;
+
+  if ((result.exitCode == 0) == expectNonZeroExit || (expectedExitCode != null && result.exitCode != expectedExitCode)) {
     // Print the output when we get unexpected results (unless output was
     // printed already).
     switch (outputMode) {
       case OutputMode.print:
         break;
       case OutputMode.capture:
-      case OutputMode.discard:
-        stdout.writeln(_flattenToString(await savedStdout));
-        stderr.writeln(_flattenToString(await savedStderr));
+        io.stdout.writeln(result.flattenedStdout);
+        io.stderr.writeln(result.flattenedStderr);
         break;
     }
     exitWithError(<String>[
       if (failureMessage != null)
         failureMessage
       else
-        '${bold}ERROR: ${red}Last command exited with $exitCode (expected: ${expectNonZeroExit ? (expectedExitCode ?? 'non-zero') : 'zero'}).$reset',
+        '${bold}ERROR: ${red}Last command exited with ${result.exitCode} (expected: ${expectNonZeroExit ? (expectedExitCode ?? 'non-zero') : 'zero'}).$reset',
       '${bold}Command: $green$commandDescription$reset',
       '${bold}Relative working directory: $cyan$relativeWorkingDir$reset',
     ]);
   }
-  print('$clock ELAPSED TIME: ${prettyPrintDuration(time.elapsed)} for $green$commandDescription$reset in $cyan$relativeWorkingDir$reset');
+  print('$clock ELAPSED TIME: ${prettyPrintDuration(result.elapsedTime)} for $green$commandDescription$reset in $cyan$relativeWorkingDir$reset');
+  return result;
 }
 
 /// Flattens a nested list of UTF-8 code units into a single string.
 String _flattenToString(List<List<int>> chunks) =>
   utf8.decode(chunks.expand<int>((List<int> ints) => ints).toList());
 
-/// Specifies what to do with command output from [runCommand].
-enum OutputMode { print, capture, discard }
-
-/// Stores command output from [runCommand] when used with [OutputMode.capture].
-class CapturedOutput {
-  String stdout;
-  String stderr;
+/// Specifies what to do with the command output from [runCommand] and [startCommand].
+enum OutputMode {
+  /// Forwards standard output and standard error streams to the test process'
+  /// respective standard streams.
+  ///
+  /// Use this mode if all you want is print the output of the command to the
+  /// console. The output is no longer available after the process exits.
+  print,
+
+  /// Saves standard output and standard error streams in memory.
+  ///
+  /// Captured output can be retrieved from the [CommandResult] object.
+  ///
+  /// Use this mode in tests that need to inspect the output of a command, or
+  /// when the output should not be printed to console.
+  capture,
 }

dev/bots/test.dart

@@ -26,7 +26,7 @@ typedef ShardRunner = Future<void> Function();
 ///
 /// If the output does not match expectations, the function shall return an
 /// appropriate error message.
-typedef OutputChecker = String Function(CapturedOutput);
+typedef OutputChecker = String Function(CommandResult);
 
 final String exe = Platform.isWindows ? '.exe' : '';
 final String bat = Platform.isWindows ? '.bat' : '';
@@ -143,9 +143,8 @@ Future<void> _validateEngineHash() async {
     return;
   }
   final String expectedVersion = File(engineVersionFile).readAsStringSync().trim();
-  final CapturedOutput flutterTesterOutput = CapturedOutput();
-  await runCommand(flutterTester, <String>['--help'], output: flutterTesterOutput, outputMode: OutputMode.capture);
-  final String actualVersion = flutterTesterOutput.stderr.split('\n').firstWhere((final String line) {
+  final CommandResult result = await runCommand(flutterTester, <String>['--help'], outputMode: OutputMode.capture);
+  final String actualVersion = result.flattenedStderr.split('\n').firstWhere((final String line) {
     return line.startsWith('Flutter Engine Version:');
   });
   if (!actualVersion.contains(expectedVersion)) {
@@ -190,8 +189,8 @@ Future<void> _runSmokeTests() async {
     script: path.join('test_smoke_test', 'pending_timer_fail_test.dart'),
     expectFailure: true,
     printOutput: false,
-    outputChecker: (CapturedOutput output) {
-      return output.stdout.contains('failingPendingTimerTest')
+    outputChecker: (CommandResult result) {
+      return result.flattenedStdout.contains('failingPendingTimerTest')
         ? null
         : 'Failed to find the stack trace for the pending Timer.';
     }
@@ -230,7 +229,7 @@ Future<void> _runSmokeTests() async {
         <String>['drive', '--use-existing-app', '-t', path.join('test_driver', 'failure.dart')],
         workingDirectory: path.join(flutterRoot, 'packages', 'flutter_driver'),
         expectNonZeroExit: true,
-        outputMode: OutputMode.discard,
+        outputMode: OutputMode.capture,
       ),
     ],
   );
@@ -287,7 +286,7 @@ Future<void> _runToolCoverage() async {
       '--report-on=lib/'
     ],
     workingDirectory: toolRoot,
-    outputMode: OutputMode.discard,
+    outputMode: OutputMode.capture,
   );
 }
 
@@ -642,11 +641,11 @@ Future<void> _runFrameworkTests() async {
       script: path.join('test', 'bindings_test_failure.dart'),
       expectFailure: true,
       printOutput: false,
-      outputChecker: (CapturedOutput output) {
-        final Iterable<Match> matches = httpClientWarning.allMatches(output.stdout);
+      outputChecker: (CommandResult result) {
+        final Iterable<Match> matches = httpClientWarning.allMatches(result.flattenedStdout);
         if (matches == null || matches.isEmpty || matches.length > 1) {
           return 'Failed to print warning about HttpClientUsage, or printed it too many times.\n'
-                 'stdout:\n${output.stdout}';
+                 'stdout:\n${result.flattenedStdout}';
         }
         return null;
       },
@@ -868,9 +867,8 @@ Future<void> _runWebDebugTest(String target, {
   List<String> additionalArguments = const<String>[],
 }) async {
   final String testAppDirectory = path.join(flutterRoot, 'dev', 'integration_tests', 'web');
-  final CapturedOutput output = CapturedOutput();
   bool success = false;
-  await runCommand(
+  final CommandResult result = await runCommand(
     flutter,
     <String>[
       'run',
@@ -889,7 +887,6 @@ Future<void> _runWebDebugTest(String target, {
       '-t',
       target,
     ],
-    output: output,
     outputMode: OutputMode.capture,
     outputListener: (String line, Process process) {
       if (line.contains('--- TEST SUCCEEDED ---')) {
@@ -908,7 +905,8 @@ Future<void> _runWebDebugTest(String target, {
   if (success) {
     print('${green}Web stack trace integration test passed.$reset');
   } else {
-    print(output.stdout);
+    print(result.flattenedStdout);
+    print(result.flattenedStderr);
     print('${red}Web stack trace integration test failed.$reset');
     exit(1);
   }
@@ -1125,29 +1123,22 @@ Future<void> _runFlutterTest(String workingDirectory, {
   args.addAll(tests);
 
   if (!shouldProcessOutput) {
-    OutputMode outputMode = OutputMode.discard;
-    CapturedOutput output;
-
-    if (outputChecker != null) {
-      outputMode = OutputMode.capture;
-      output = CapturedOutput();
-    } else if (printOutput) {
-      outputMode = OutputMode.print;
-    }
+    final OutputMode outputMode = outputChecker == null && printOutput
+      ? OutputMode.print
+      : OutputMode.capture;
 
-    await runCommand(
+    final CommandResult result = await runCommand(
       flutter,
       args,
       workingDirectory: workingDirectory,
       expectNonZeroExit: expectFailure,
       outputMode: outputMode,
-      output: output,
       skip: skip,
       environment: environment,
     );
 
     if (outputChecker != null) {
-      final String message = outputChecker(output);
+      final String message = outputChecker(result);
       if (message != null)
         exitWithError(<String>[message]);
     }