From 8c06d8c3fed51bb6037a55db34b024561351cbcb Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Thu, 9 Dec 2021 11:53:09 -0700 Subject: [PATCH 1/5] Allow args after separator to TestTool --- CHANGELOG.md | 12 +++++ lib/src/tools/compound_tool.dart | 3 +- lib/src/tools/function_tool.dart | 3 -- lib/src/tools/test_tool.dart | 15 ++---- lib/src/utils/rest_args_with_separator.dart | 44 +++++++++++++++ test/tools/test_tool_test.dart | 38 +++++++++++++ test/utils/rest_args_with_separator_test.dart | 54 +++++++++++++++++++ 7 files changed, 153 insertions(+), 16 deletions(-) create mode 100644 lib/src/utils/rest_args_with_separator.dart create mode 100644 test/utils/rest_args_with_separator_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 097d61b9..ec4de32f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## [3.8.1](https://github.com/Workiva/dart_dev/compare/3.8.1...3.8.0) + +- 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.8.0](https://github.com/Workiva/dart_dev/compare/3.8.0...3.7.0) - Upgrade to analyzer ^1.0.0 and build_runner to ^2.0.0. This also brings along diff --git a/lib/src/tools/compound_tool.dart b/lib/src/tools/compound_tool.dart index ba003dd1..aeba5132 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 b5b68551..a59439e9 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..81b2fac8 --- /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 rCursor = 0; + for (var aCursor = 0; aCursor < args.length; aCursor++) { + // Iterate through the original args until we hit the first separator. + if (args[aCursor] == '--') 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[aCursor] == rest[rCursor]) { + rCursor++; + } + } + + // At this point, [rCursor] should be pointing to the spot where the first arg + // separator should be restored. + return [...rest.sublist(0, rCursor), '--', ...rest.sublist(rCursor)]; +} diff --git a/test/tools/test_tool_test.dart b/test/tools/test_tool_test.dart index f24adc84..c1d8c8a4 100644 --- a/test/tools/test_tool_test.dart +++ b/test/tools/test_tool_test.dart @@ -183,6 +183,44 @@ void main() { verbose: true), orderedEquals(['run', 'build_runner', 'test', '--verbose'])); }); + + 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', () { 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', + ]); + }); + }); +} From 4cff3b8982c51b614eeef13925264ba32be47f24 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Thu, 9 Dec 2021 12:30:13 -0700 Subject: [PATCH 2/5] Fix tests --- test/tools/function_tool_test.dart | 10 ++---- test/tools/test_tool_test.dart | 50 +++++++++++++++++++++++------- 2 files changed, 41 insertions(+), 19 deletions(-) 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 c1d8c8a4..38dfda9d 100644 --- a/test/tools/test_tool_test.dart +++ b/test/tools/test_tool_test.dart @@ -224,18 +224,6 @@ void main() { }); 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'))))); - }); - test( 'throws UsageException if --build-args is used but build_runner is not ' 'a direct dependency', () async { @@ -385,6 +373,16 @@ dev_dependencies: orderedEquals(['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, 'dart'); + expect(execution.process.args, orderedEquals(['test', '-j1'])); + }); + test( 'and logs a warning if --release is used in a non-build project', () => overrideAnsiOutput(false, () { @@ -445,6 +443,16 @@ dev_dependencies: orderedEquals(['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, 'dart'); + expect(execution.process.args, orderedEquals(['test', '-j1'])); + }); + test( 'and logs a warning if --release is used in a non-build project', () => overrideAnsiOutput(false, () { @@ -523,6 +531,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, 'dart'); + expect( + execution.process.args, + orderedEquals([ + 'run', + 'build_runner', + 'test', + '--', + '-j1', + ])); + }); + test('with verbose=true', () { final argParser = TestTool().toCommand('t').argParser; final argResults = argParser.parse( From 183b8b46be3104aaacdf27d964fd0cbffc384f1e Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Mon, 13 Dec 2021 13:50:14 -0700 Subject: [PATCH 3/5] Fix doc comment example --- lib/src/utils/rest_args_with_separator.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/utils/rest_args_with_separator.dart b/lib/src/utils/rest_args_with_separator.dart index 81b2fac8..d5c81c39 100644 --- a/lib/src/utils/rest_args_with_separator.dart +++ b/lib/src/utils/rest_args_with_separator.dart @@ -16,7 +16,7 @@ import 'package:args/args.dart'; /// print(results.rest); /// // ['a', 'b', '--unknown', 'c'] /// print(restArgsWithSeparator(results)); -/// // ['a', 'b', '--', '--unknown', '-c'] +/// // ['a', 'b', '--', '--unknown', 'c'] List restArgsWithSeparator(ArgResults argResults) { // If no separator was used, return the rest args as is. if (!argResults.arguments.contains('--')) { From 24d8b5d11bf67156211fdce0de14df71ff9b4fea Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Wed, 15 Dec 2021 09:25:36 -0700 Subject: [PATCH 4/5] Better variable naming --- lib/src/utils/rest_args_with_separator.dart | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/src/utils/rest_args_with_separator.dart b/lib/src/utils/rest_args_with_separator.dart index d5c81c39..b94335d7 100644 --- a/lib/src/utils/rest_args_with_separator.dart +++ b/lib/src/utils/rest_args_with_separator.dart @@ -25,20 +25,20 @@ List restArgsWithSeparator(ArgResults argResults) { final args = argResults.arguments; final rest = argResults.rest; - var rCursor = 0; - for (var aCursor = 0; aCursor < args.length; aCursor++) { + var restIndex = 0; + for (var argsIndex = 0; argsIndex < args.length; argsIndex++) { // Iterate through the original args until we hit the first separator. - if (args[aCursor] == '--') break; + 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[aCursor] == rest[rCursor]) { - rCursor++; + if (args[argsIndex] == rest[restIndex]) { + restIndex++; } } - // At this point, [rCursor] should be pointing to the spot where the first arg - // separator should be restored. - return [...rest.sublist(0, rCursor), '--', ...rest.sublist(rCursor)]; + // 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)]; } From d6f79827cfc72a81ecf7885f989919769a2364ee Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Wed, 15 Dec 2021 15:40:11 -0700 Subject: [PATCH 5/5] Fix changelog --- CHANGELOG.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec4de32f..f6956373 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## [3.8.1](https://github.com/Workiva/dart_dev/compare/3.8.1...3.8.0) +## [3.8.2](https://github.com/Workiva/dart_dev/compare/3.8.1...3.8.2) - 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 @@ -12,20 +12,24 @@ wants with those args (and now we have a concrete use case for just that). that it now properly restores the first separator (`--`) if present in the original arguments list. -## [3.8.0](https://github.com/Workiva/dart_dev/compare/3.8.0...3.7.0) +## [3.8.1](https://github.com/Workiva/dart_dev/compare/3.8.0..3.8.1) + +- Tech debt: use the new `dart` CLI everywhere. + +## [3.8.0](https://github.com/Workiva/dart_dev/compare/3.7.0...3.8.0) - Upgrade to analyzer ^1.0.0 and build_runner to ^2.0.0. This also brings along several other dependency upgrades. -## [3.7.0](https://github.com/Workiva/dart_dev/compare/3.7.0...3.6.7) +## [3.7.0](https://github.com/Workiva/dart_dev/compare/3.6.7...3.7.0) - Export ArgResults utilities. -## [3.6.7](https://github.com/Workiva/dart_dev/compare/3.6.7...3.6.6) +## [3.6.7](https://github.com/Workiva/dart_dev/compare/3.6.6...3.6.7) - Treat Dart 2.13.4 as the primary Dart SDK for development and CI. -## [3.6.6](https://github.com/Workiva/dart_dev/compare/3.6.6...3.6.5) +## [3.6.6](https://github.com/Workiva/dart_dev/compare/3.6.5...3.6.6) - Only use build_runner to run tests if the package has direct dependencies on both `build_runner` _and_ `build_test` (previously we only checked for @@ -33,12 +37,12 @@ both `build_runner` _and_ `build_test` (previously we only checked for likely have a dependency on `build_test` for use in their builder tests, but don't need to run tests via `build_runner`. -## [3.6.5](https://github.com/Workiva/dart_dev/compare/3.6.5...3.6.4) +## [3.6.5](https://github.com/Workiva/dart_dev/compare/3.6.4...3.6.5) - Widen dependency ranges to allow resolution on Dart 2.7 and Dart 2.13 - Switch to GitHub actions for CI -## [3.6.4](https://github.com/Workiva/dart_dev/compare/3.6.4...3.6.1) +## [3.6.4](https://github.com/Workiva/dart_dev/compare/3.6.1...3.6.4) - Widen `analyzer` constraint to `>=0.39.0 <0.42.0`