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/bin/tweaked_build_runner.dart b/bin/tweaked_build_runner.dart new file mode 100644 index 00000000..38c99427 --- /dev/null +++ b/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 main(List 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 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'); +} diff --git a/lib/src/dart_dev_runner.dart b/lib/src/dart_dev_runner.dart index ed96c285..725e5053 100644 --- a/lib/src/dart_dev_runner.dart +++ b/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 { 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; } } diff --git a/lib/src/dart_dev_tool.dart b/lib/src/dart_dev_tool.dart index 275dc0f1..0e6ae5fc 100644 --- a/lib/src/dart_dev_tool.dart +++ b/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,6 +110,8 @@ 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({ @@ -114,13 +119,14 @@ class DevToolExecutionContext { 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 { @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 run() async => devTool.run(DevToolExecutionContext( argResults: argResults, commandName: name, usageException: usageException, - verbose: verboseEnabled(this))); + verbose: verboseEnabled(this), + logFilePath: logFilePath)); } diff --git a/lib/src/events.dart b/lib/src/events.dart index b909c503..7c144d63 100644 --- a/lib/src/events.dart +++ b/lib/src/events.dart @@ -13,8 +13,9 @@ final _commandCompleteListeners = Function(CommandResult result)>[]; class CommandResult { - CommandResult(this.args, this.exitCode, this.duration); + CommandResult(this.args, this.exitCode, this.duration, {this.log}); final List args; final Duration duration; final int exitCode; + String log; } diff --git a/lib/src/executable.dart b/lib/src/executable.dart index eed66633..7a1e9e3b 100644 --- a/lib/src/executable.dart +++ b/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'; 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..c796a070 100644 --- a/lib/src/tools/test_tool.dart +++ b/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 buildArgs({ List configuredTestArgs, bool useBuildRunner, bool verbose, + String logFilePath, }) { useBuildRunner ??= false; verbose ??= false; @@ -233,10 +233,14 @@ List 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 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 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/pubspec.yaml b/pubspec.yaml index 1d56cc4a..86bd4b6f 100644 --- a/pubspec.yaml +++ b/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 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', + ]); + }); + }); +}