Merge pull request #365 from Workiva/test_tool_trailing_args_v370backpatch

Allow args after separator to TestTool (backpatch to v3.7)

Author: rmconsole5-wk

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.

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

@@ -273,16 +273,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);
@@ -333,9 +323,10 @@ TestExecution buildExecution(
 // 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)];
+}

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', () {

test/tools/test_tool_test.dart

@@ -185,21 +185,47 @@ void main() {
               verbose: true),
           orderedEquals(['run', 'build_runner', 'test', '--verbose']));
     });
-  });
 
-  group('buildExecution', () {
-    test('throws UsageException if args are given after a separator', () {
-      final argResults = ArgParser().parse(['--', 'a']);
-      final context = DevToolExecutionContext(
-          argResults: argResults, commandName: 'test_test');
-      expect(
-          () => buildExecution(context),
-          throwsA(isA<UsageException>()
-            ..having((e) => e.message, 'command name', contains('test_test'))
-            ..having((e) => e.message, 'help',
-                allOf(contains('--test-args'), contains('--build-args')))));
+    group('supports test args after a separator', () {
+      test('with no test file', () {
+        final argParser = TestTool().toCommand('t').argParser;
+        final argResults = argParser.parse(['--', '-r', 'json', '-j1']);
+        expect(
+            buildArgs(argResults: argResults, useBuildRunner: true),
+            orderedEquals([
+              'run',
+              'build_runner',
+              'test',
+              '--',
+              '-r',
+              'json',
+              '-j1',
+            ]));
+      });
+
+      test('with a test file', () {
+        final argParser = TestTool().toCommand('t').argParser;
+        final argResults =
+            argParser.parse(['--', '-r', 'json', '-j1', 'test/foo_test.dart']);
+        expect(
+            buildArgs(argResults: argResults, useBuildRunner: true),
+            orderedEquals([
+              'run',
+              'build_runner',
+              'test',
+              '--build-filter=test/foo_test.dart.*_test.dart.js*',
+              '--build-filter=test/foo_test.html',
+              '--',
+              '-r',
+              'json',
+              '-j1',
+              'test/foo_test.dart',
+            ]));
+      });
     });
+  });
 
+  group('buildExecution', () {
     test(
         'throws UsageException if --build-args is used but build_runner is not '
         'a direct dependency', () async {
@@ -349,6 +375,16 @@ dev_dependencies:
               orderedEquals(['run', 'test', '-P', 'unit', '-n', 'foo']));
         });
 
+        test('with args after a separator', () {
+          final argParser = TestTool().toCommand('t').argParser;
+          final argResults = argParser.parse(['--', '-j1']);
+          final context = DevToolExecutionContext(argResults: argResults);
+          final execution = buildExecution(context, path: d.sandbox);
+          expect(execution.exitCode, isNull);
+          expect(execution.process.executable, 'pub');
+          expect(execution.process.args, orderedEquals(['run', 'test', '-j1']));
+        });
+
         test(
             'and logs a warning if --release is used in a non-build project',
             () => overrideAnsiOutput(false, () {
@@ -409,6 +445,16 @@ dev_dependencies:
               orderedEquals(['run', 'test', '-P', 'unit', '-n', 'foo']));
         });
 
+        test('with args after a separator', () {
+          final argParser = TestTool().toCommand('t').argParser;
+          final argResults = argParser.parse(['--', '-j1']);
+          final context = DevToolExecutionContext(argResults: argResults);
+          final execution = buildExecution(context, path: d.sandbox);
+          expect(execution.exitCode, isNull);
+          expect(execution.process.executable, 'pub');
+          expect(execution.process.args, orderedEquals(['run', 'test', '-j1']));
+        });
+
         test(
             'and logs a warning if --release is used in a non-build project',
             () => overrideAnsiOutput(false, () {
@@ -487,6 +533,24 @@ dev_dependencies:
               ]));
         });
 
+        test('with args after a separator', () {
+          final argParser = TestTool().toCommand('t').argParser;
+          final argResults = argParser.parse(['--', '-j1']);
+          final context = DevToolExecutionContext(argResults: argResults);
+          final execution = buildExecution(context, path: d.sandbox);
+          expect(execution.exitCode, isNull);
+          expect(execution.process.executable, 'pub');
+          expect(
+              execution.process.args,
+              orderedEquals([
+                'run',
+                'build_runner',
+                'test',
+                '--',
+                '-j1',
+              ]));
+        });
+
         test('with verbose=true', () {
           final argParser = TestTool().toCommand('t').argParser;
           final argResults = argParser.parse(

test/utils/rest_args_with_separator_test.dart

@@ -0,0 +1,54 @@
+import 'package:args/args.dart';
+import 'package:dart_dev/src/utils/rest_args_with_separator.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group('restArgsWithSeparator', () {
+    ArgParser parser;
+
+    setUp(() {
+      parser = ArgParser()
+        ..addOption('output', abbr: 'o')
+        ..addFlag('verbose', abbr: 'v');
+    });
+
+    test('with no args', () {
+      final results = parser.parse([]);
+      expect(restArgsWithSeparator(results), <String>[]);
+    });
+
+    test('restores the separator to the correct spot', () {
+      final results = parser.parse([
+        'a',
+        '-o',
+        'out',
+        '-v',
+        'b',
+        '--',
+        'c',
+        '-d',
+      ]);
+      expect(restArgsWithSeparator(results), [
+        'a',
+        'b',
+        '--',
+        'c',
+        '-d',
+      ]);
+    });
+
+    test('with multiple separators', () {
+      final results = parser
+          .parse(['a', '-o', 'out', '-v', 'b', '--', 'c', '-d', '--', 'e']);
+      expect(restArgsWithSeparator(results), [
+        'a',
+        'b',
+        '--',
+        'c',
+        '-d',
+        '--',
+        'e',
+      ]);
+    });
+  });
+}