POC for better metrics gathering from build_runner

CHANGELOG.md

@@ -1,5 +1,17 @@
 # Changelog
 
+## [3.7.1](https://github.com/Workiva/dart_dev/compare/3.7.0...3.7.1)
+
+- Update `TestTool` to allow arguments after a separator (`--`). These arguments
+will always be passed to the `dart test` process. The main use case for this is
+integration with IDE plugins that enable running tests directly from the IDE.
+- Update `FunctionTool` to allow arguments after a separator (`--`). There isn't
+a strong reason to disallow this since the function tool could do anything it
+wants with those args (and now we have a concrete use case for just that).
+- Fix a bug in `takeAllArgs` (the arg mapper util used with `CompoundTool`) so
+that it now properly restores the first separator (`--`) if present in the
+original arguments list.
+
 ## [3.7.0](https://github.com/Workiva/dart_dev/compare/3.7.0...3.6.7)
 
 - Export ArgResults utilities.

bin/tweaked_build_runner.dart

@@ -0,0 +1,51 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Very simplified version of the build_runner main that also does logging of the
+/// build summary.
+import 'dart:async';
+import 'dart:io';
+
+import 'package:build_runner/src/build_script_generate/bootstrap.dart';
+import 'package:build_runner/src/build_script_generate/build_script_generate.dart';
+import 'package:build_runner/src/logging/std_io_logging.dart';
+import 'package:io/io.dart';
+import 'package:logging/logging.dart';
+
+String logFile;
+const String logFileArgument = '--logFile=';
+
+bool isLogArgument(String arg) => arg.startsWith(logFileArgument);
+Future<void> main(List<String> args) async {
+  logFile = args
+      .firstWhere(isLogArgument, orElse: () => null)
+      .substring(logFileArgument.length);
+  // We're generating the argument directly into the file, but that means that each run will
+  // rebuild the build script??
+  args = [
+    for (var arg in args)
+      if (!isLogArgument(arg)) arg
+  ];
+  var logListener = Logger.root.onRecord.listen(stdIOLogListener());
+
+  while ((exitCode = await generateAndRun(args,
+          generateBuildScript: generateMyBuildScript)) ==
+      ExitCode.tempFail.code) {}
+  await logListener.cancel();
+}
+
+// Tweak the build script to also log to the file named passed in the --logFileName argument.
+Future<String> generateMyBuildScript() async {
+  var basic = await generateBuildScript();
+  var lines = basic.split('\n');
+  lines.insert(7, "import 'dart:io';");
+  lines.insert(7, "import 'package:logging/logging.dart';");
+  var startOfMain =
+      lines.indexOf(lines.firstWhere((l) => l.contains('void main')));
+  lines.insert(startOfMain + 1,
+      '  var log = File("${logFile ?? 'temporaryLogFile.txt'}");');
+  lines.insert(startOfMain + 2,
+      '  Logger.root.onRecord.listen((r) {log.writeAsString("\$r", mode: FileMode.append);});');
+  return lines.join('\n');
+}

lib/src/dart_dev_runner.dart

@@ -1,4 +1,5 @@
 import 'dart:async';
+import 'dart:io';
 
 import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
@@ -44,12 +45,30 @@ class DartDevRunner extends CommandRunner<int> {
       print(dartDevVersion);
       return 0;
     }
+    // About the only mechanism I can find for communicating between the different levels of this
+    // is that this runner holds an actual list of command instances, so we can associate the log
+    // file path with that.
+    print("commands = $commands");
+    print("we're trying to execute ${argResults.name}");
+    print("or is that ${argResults.command.name}");
+    final command = commands[argResults.command.name];
+    print(
+        "We are trying to write a log at ${(command as DevToolCommand).logFilePath}");
     final stopwatch = Stopwatch()..start();
+
+    print("running with ${args}");
     final exitCode = (await super.run(args)) ?? 0;
     stopwatch.stop();
-    await events.commandComplete(
-        events.CommandResult(args, exitCode, stopwatch.elapsed));
-    return exitCode;
+    String log;
+    if (command != null && command is DevToolCommand) {
+      print("We expect to read a log at ${command.logFilePath}");
+      log = await File(command.logFilePath).readAsString();
+      print("Got ${log.length} bytes of log");
+    }
+    events.CommandResult result =
+        events.CommandResult(args, exitCode, stopwatch.elapsed, log: log);
+    await events.commandComplete(result);
+    return result.exitCode;
   }
 }
 

lib/src/dart_dev_tool.dart

@@ -1,9 +1,11 @@
 import 'dart:async';
 import 'dart:io';
+import 'dart:math';
 
 import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:dart_dev/dart_dev.dart';
+import 'package:path/path.dart' as path;
 
 import 'tools/function_tool.dart';
 import 'utils/verbose_enabled.dart';
@@ -83,7 +85,8 @@ class DevToolExecutionContext {
       {this.argResults,
       this.commandName,
       void Function(String message) usageException,
-      bool verbose})
+      bool verbose,
+      this.logFilePath})
       : _usageException = usageException,
         verbose = verbose ?? false;
 
@@ -107,20 +110,23 @@ class DevToolExecutionContext {
   /// This will not be null; it defaults to `false`.
   final bool verbose;
 
+  String logFilePath;
+
   /// Return a copy of this instance with optional updates; any field that does
   /// not have an updated value will remain the same.
   DevToolExecutionContext update({
     ArgResults argResults,
     String commandName,
     void Function(String message) usageException,
     bool verbose,
+    String logFilePath,
   }) =>
       DevToolExecutionContext(
-        argResults: argResults ?? this.argResults,
-        commandName: commandName ?? this.commandName,
-        usageException: usageException ?? this.usageException,
-        verbose: verbose ?? this.verbose,
-      );
+          argResults: argResults ?? this.argResults,
+          commandName: commandName ?? this.commandName,
+          usageException: usageException ?? this.usageException,
+          verbose: verbose ?? this.verbose,
+          logFilePath: logFilePath ?? this.logFilePath);
 
   /// Calling this will throw a [UsageException] with [message] that should be
   /// caught by [CommandRunner] and used to set the exit code accordingly and
@@ -150,10 +156,16 @@ class DevToolCommand extends Command<int> {
   @override
   final String name;
 
+  /// A probably unique file path to write logs to.
+  // TODO: A better name mechanism.
+  final String logFilePath = path.join(Directory.systemTemp.path,
+      '${Random(DateTime.now().microsecondsSinceEpoch).nextInt(1 << 32)}');
+
   @override
   Future<int> run() async => devTool.run(DevToolExecutionContext(
       argResults: argResults,
       commandName: name,
       usageException: usageException,
-      verbose: verboseEnabled(this)));
+      verbose: verboseEnabled(this),
+      logFilePath: logFilePath));
 }

lib/src/events.dart

@@ -13,8 +13,9 @@ final _commandCompleteListeners =
     <FutureOr<dynamic> Function(CommandResult result)>[];
 
 class CommandResult {
-  CommandResult(this.args, this.exitCode, this.duration);
+  CommandResult(this.args, this.exitCode, this.duration, {this.log});
   final List<String> args;
   final Duration duration;
   final int exitCode;
+  String log;
 }

lib/src/executable.dart

@@ -1,6 +1,5 @@
 import 'dart:async';
 import 'dart:io';
-import 'dart:math';
 
 import 'package:analyzer/dart/analysis/utilities.dart';
 import 'package:args/command_runner.dart';

lib/src/tools/compound_tool.dart

@@ -6,6 +6,7 @@ import 'package:dart_dev/dart_dev.dart';
 import 'package:logging/logging.dart';
 
 import '../dart_dev_tool.dart';
+import '../utils/rest_args_with_separator.dart';
 
 final _log = Logger('CompoundTool');
 
@@ -43,7 +44,7 @@ ArgResults takeOptionArgs(ArgParser parser, ArgResults results) =>
 ///     };
 ArgResults takeAllArgs(ArgParser parser, ArgResults results) => parser.parse([
       ...optionArgsOnly(results, allowedOptions: parser.options.keys),
-      ...results.rest,
+      ...restArgsWithSeparator(results),
     ]);
 
 class CompoundTool extends DevTool with CompoundToolMixin {}

lib/src/tools/function_tool.dart

@@ -36,9 +36,6 @@ class FunctionTool extends DevTool {
         assertNoPositionalArgsNorArgsAfterSeparator(
             context.argResults, context.usageException,
             commandName: context.commandName);
-      } else {
-        assertNoArgsAfterSeparator(context.argResults, context.usageException,
-            commandName: context.commandName);
       }
     }
     final exitCode = await _function(context);

lib/src/tools/test_tool.dart

@@ -9,7 +9,6 @@ import 'package:logging/logging.dart';
 
 import '../dart_dev_tool.dart';
 import '../utils/arg_results_utils.dart';
-import '../utils/assert_no_args_after_separator.dart';
 import '../utils/logging.dart';
 import '../utils/package_is_immediate_dependency.dart';
 import '../utils/process_declaration.dart';
@@ -188,6 +187,7 @@ List<String> buildArgs({
   List<String> configuredTestArgs,
   bool useBuildRunner,
   bool verbose,
+  String logFilePath,
 }) {
   useBuildRunner ??= false;
   verbose ??= false;
@@ -233,10 +233,14 @@ List<String> buildArgs({
   }
 
   return [
-    // `pub run test` or `pub run build_runner test`
-    'run',
-    if (useBuildRunner) 'build_runner',
-    'test',
+    // `dart test` or `dart run build_runner test`
+    if (useBuildRunner) ...[
+      'run',
+      'dart_dev:tweaked_build_runner',
+      'test',
+      if (useBuildRunner && (logFilePath != null)) '--logFile=$logFilePath',
+    ] else
+      'test',
 
     // Add the args targeting the build_runner command.
     if (useBuildRunner) ...buildArgs,
@@ -273,16 +277,6 @@ TestExecution buildExecution(
   List<String> configuredTestArgs,
   String path,
 }) {
-  if (context.argResults != null) {
-    assertNoArgsAfterSeparator(context.argResults, context.usageException,
-        commandName: context.commandName,
-        usageFooter:
-            'Arguments can be passed to the test runner process via the '
-            '--test-args option.\n'
-            'If this project runs tests via build_runner, arguments can be '
-            'passed to that process via the --build-args option.');
-  }
-
   final hasBuildRunner =
       packageIsImmediateDependency('build_runner', path: path);
   final hasBuildTest = packageIsImmediateDependency('build_test', path: path);
@@ -323,19 +317,21 @@ TestExecution buildExecution(
       configuredBuildArgs: configuredBuildArgs,
       configuredTestArgs: configuredTestArgs,
       useBuildRunner: useBuildRunner,
-      verbose: context.verbose);
+      verbose: context.verbose,
+      logFilePath: context.logFilePath);
   logSubprocessHeader(_log, 'pub ${args.join(' ')}'.trim());
   return TestExecution.process(
-      ProcessDeclaration('pub', args, mode: ProcessStartMode.inheritStdio));
+      ProcessDeclaration('dart', args, mode: ProcessStartMode.inheritStdio));
 }
 
 // NOTE: This currently depends on https://github.com/dart-lang/build/pull/2445
 // Additionally, consumers need to depend on build_web_compilers AND build_vm_compilers
 // We should add some guard-rails (don't use filters if either of those deps are
 // missing, and ensure adequate version of build_runner).
-Iterable<String> buildFiltersForTestArgs(List<String> testInputs) {
+Iterable<String> buildFiltersForTestArgs(List<String> testArgs) {
+  final testInputs = (testArgs ?? []).where((arg) => arg.startsWith('test'));
   final filters = <String>[];
-  for (final input in testInputs ?? []) {
+  for (final input in testInputs) {
     if (input.endsWith('.dart')) {
       filters..add('$input.*_test.dart.js*')..add(dartExtToHtml(input));
     } else {

lib/src/utils/rest_args_with_separator.dart

@@ -0,0 +1,44 @@
+import 'package:args/args.dart';
+
+/// Returns the "rest" args from [argResults], but with the arg separator "--"
+/// restored to its original position if it was included.
+///
+/// This is necessary because [ArgResults.rest] will _not_ include the separator
+/// unless it stopped parsing before it reached the separator.
+///
+/// The use case for this is a [CompoundTool] that uses the [takeAllArgs] arg
+/// mapper, because the goal there is to forward on the original args minus the
+/// consumed options and flags. If the separator has also been removed, you may
+/// hit an error when trying to parse those args.
+///
+///     var parser = ArgParser()..addFlag('verbose', abbr: 'v');
+///     var results = parser.parse(['a', '-v', 'b', '--', '--unknown', 'c']);
+///     print(results.rest);
+///     // ['a', 'b', '--unknown', 'c']
+///     print(restArgsWithSeparator(results));
+///     // ['a', 'b', '--', '--unknown', 'c']
+List<String> restArgsWithSeparator(ArgResults argResults) {
+  // If no separator was used, return the rest args as is.
+  if (!argResults.arguments.contains('--')) {
+    return argResults.rest;
+  }
+
+  final args = argResults.arguments;
+  final rest = argResults.rest;
+  var restIndex = 0;
+  for (var argsIndex = 0; argsIndex < args.length; argsIndex++) {
+    // Iterate through the original args until we hit the first separator.
+    if (args[argsIndex] == '--') break;
+    // While doing so, move a cursor through the rest args list each time we
+    // match up between the original list and the rest args list. This works
+    // because the rest args list should be an ordered subset of the original
+    // args list.
+    if (args[argsIndex] == rest[restIndex]) {
+      restIndex++;
+    }
+  }
+
+  // At this point, [restIndex] should be pointing to the spot where the first
+  // arg separator should be restored.
+  return [...rest.sublist(0, restIndex), '--', ...rest.sublist(restIndex)];
+}

pubspec.yaml

@@ -1,5 +1,5 @@
 name: dart_dev
-version: 3.7.0
+version: 3.7.1
 description: Centralized tooling for Dart projects. Consistent interface across projects. Easily configurable.
 homepage: https://github.com/Workiva/dart_dev
 

test/tools/function_tool_test.dart

@@ -36,14 +36,10 @@ void main() {
           .run(DevToolExecutionContext(argResults: parser.parse(['--flag'])));
     });
 
-    test(
-        'throws UsageException with custom ArgParser and args after a separator',
-        () {
+    test('allows a custom ArgParser and args after a separator', () async {
       final tool = DevTool.fromFunction((_) => 0, argParser: ArgParser());
-      expect(
-          () => tool.run(DevToolExecutionContext(
-              argResults: ArgParser().parse(['--', 'foo']))),
-          throwsA(isA<UsageException>()));
+      await tool.run(DevToolExecutionContext(
+          argResults: ArgParser().parse(['--', 'foo'])));
     });
 
     test('logs a warning if no exit code is returned', () {