Skip to content

CPLAT-16476 Allow args after separator to TestTool #364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,32 +1,48 @@
# Changelog

## [3.8.0](https://github.com/Workiva/dart_dev/compare/3.8.0...3.7.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
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.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
`build_test`). For packages that contain builder implementations, they will
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`

Expand Down
3 changes: 2 additions & 1 deletion lib/src/tools/compound_tool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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 {}
Expand Down
3 changes: 0 additions & 3 deletions lib/src/tools/function_tool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 3 additions & 12 deletions lib/src/tools/test_tool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
44 changes: 44 additions & 0 deletions lib/src/utils/rest_args_with_separator.dart
Original file line number Diff line number Diff line change
@@ -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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble following this. It seems like the only result of this is setting rCursor. But isn't rCursor just args.indexOf('--')?

Copy link
Contributor Author

@evanweible-wf evanweible-wf Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be the position of the separator within the original arguments list, which isn't necessarily the same as the position at which the separator should be restored in the rest arguments list, since the latter excludes flags and options that were parsed by the ArgParser. So for example:

final argParser = ArgParser()
  ..addOption('output')
  ..addFlag('verbose');
final argResults = argParser.parse(['foo', '--output', 'out', '--verbose', '--', 'bar', '--baz']);
print(argResults.arguments);
// ['foo', '--output', 'out', '--verbose', '--', 'bar', '--baz']
// In this list, separator index is 4
print(argResults.rest);
// ['foo', 'bar', '--baz']
// In this list, we want to restore the separator at index 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I get it. Ugh. It'd be better if ArgParser just didn't think it knew about '--' and included it.

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)];
}
10 changes: 3 additions & 7 deletions test/tools/function_tool_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down
88 changes: 76 additions & 12 deletions test/tools/test_tool_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -183,21 +183,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 {
Expand Down Expand Up @@ -347,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, () {
Expand Down Expand Up @@ -407,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, () {
Expand Down Expand Up @@ -485,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(
Expand Down
54 changes: 54 additions & 0 deletions test/utils/rest_args_with_separator_test.dart
Original file line number Diff line number Diff line change
@@ -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',
]);
});
});
}