diff --git a/CHANGELOG.md b/CHANGELOG.md index 52f1e2eb..d59fcf4a 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/lib/src/tools/compound_tool.dart b/lib/src/tools/compound_tool.dart index c71b22e1..2a2e1821 100644 --- a/lib/src/tools/compound_tool.dart +++ b/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 {} diff --git a/lib/src/tools/function_tool.dart b/lib/src/tools/function_tool.dart index 6bd90c66..7e63422b 100644 --- a/lib/src/tools/function_tool.dart +++ b/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); diff --git a/lib/src/tools/test_tool.dart b/lib/src/tools/test_tool.dart index 044aab7f..c634619a 100644 --- a/lib/src/tools/test_tool.dart +++ b/lib/src/tools/test_tool.dart @@ -273,16 +273,6 @@ TestExecution buildExecution( List 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 buildFiltersForTestArgs(List testInputs) { +Iterable buildFiltersForTestArgs(List testArgs) { + final testInputs = (testArgs ?? []).where((arg) => arg.startsWith('test')); final filters = []; - for (final input in testInputs ?? []) { + for (final input in testInputs) { if (input.endsWith('.dart')) { filters..add('$input.*_test.dart.js*')..add(dartExtToHtml(input)); } else { diff --git a/lib/src/utils/rest_args_with_separator.dart b/lib/src/utils/rest_args_with_separator.dart new file mode 100644 index 00000000..b94335d7 --- /dev/null +++ b/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 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)]; +} diff --git a/test/tools/function_tool_test.dart b/test/tools/function_tool_test.dart index a9823ca9..226ce076 100644 --- a/test/tools/function_tool_test.dart +++ b/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())); + await tool.run(DevToolExecutionContext( + argResults: ArgParser().parse(['--', 'foo']))); }); test('logs a warning if no exit code is returned', () { diff --git a/test/tools/test_tool_test.dart b/test/tools/test_tool_test.dart index 26591296..17596e6e 100644 --- a/test/tools/test_tool_test.dart +++ b/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() - ..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( diff --git a/test/utils/rest_args_with_separator_test.dart b/test/utils/rest_args_with_separator_test.dart new file mode 100644 index 00000000..2c03bb03 --- /dev/null +++ b/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), []); + }); + + 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', + ]); + }); + }); +}