diff --git a/bin/test_server.dart b/bin/test_server.dart new file mode 100644 index 00000000..3e89ad07 --- /dev/null +++ b/bin/test_server.dart @@ -0,0 +1,216 @@ +import 'dart:io'; + +import 'package:http/http.dart' as http; +import 'package:path/path.dart' as p; +import 'package:shelf/shelf.dart'; +import 'package:shelf/shelf_io.dart' as shelf_io; +import 'package:shelf_static/shelf_static.dart'; + +// Some graph_ui tests fail with this on default settings. Seems to be because something is getting overlodaed. Running +// with -j 1 or 2 they run fine. But -j 1 seems to make no difference in performance, so might as well use it. This +// actually seems a bit faster, just on the test part, than the test part of build_runner test. + +// Get the package name from the current directory name, used to find the directory to serve inside build/generated. +String packageName = p.basename(Directory.current.path); + +// This seems slow, and if I don't run with -j 1, it can fail. I suspect it might be an issue in this server blocking or +// having a race condition, but not clear. + +// TODO: parse args and allow this to be specified. +int servePort = 8088; + +int pubServePort = 8080; + +void main() async { + var proxy = proxyHandler('http://localhost:$pubServePort'); + var handler = Cascade() + .add(createStaticHandler('.')) + .add(createStaticHandler('test')) + .add(createStaticHandler('.dart_tool/build/generated/$packageName/test')) + .add(proxy) // I think this is just needed for source code in /packages. + .handler; + var pipeline = const Pipeline() + .addMiddleware(logRequests(logger: logErrors)) + .addMiddleware(rewriteTests()) + .addHandler(handler); + var server = await shelf_io.serve(pipeline, 'localhost', servePort); + + print('Proxying at http://${server.address.host}:${server.port}'); +} + +// Just log errors, so it isn't overwhelming. +void logErrors(String msg, bool isError) { + if (isError) { + print('[ERROR] $msg'); + } else { + // print(msg); + } +} + +// Some of the test files need to be rewritten, because what we get from +// a serve isn't the same as what we'd get by creating a merged test directory. Not +// especially clear why/what? Should look at exactly what creating the merged directory +// does. +Middleware rewriteTests() { + return (innerHandler) { + return (request) async { + var newRequest = await rewrite(request); + return innerHandler(newRequest ?? request); + }; + }; +} + +// If we see we end with the key, then we insert the value after the _test +var replacementRules = { + // 'webdev serve' serves them up with the debug extension. Does that make a difference? Can we make it not do that? + '_test.html': '_test.debug.html', + // This seems to be necessary for some packages (microfrontend) but not others graph_ui. Presumably this is related to + // package:test expecting dart2js compilation, but I don't understand why the difference. Are there other files like this? + '_test.dart.js.map': '_test.unsound.ddc.js.map' +}; + +// TODO: Shouldn't this be the same list as what we have in the cascade above? +var knownDirectories = [ + 'test', + // This seems to be necessary for some packages (microfrontend) but not others (graph_ui). + '.dart_tool/build/generated/$packageName/test', +]; + +/// Rewrite a request to check if the path needs to be modified. +Future rewrite(Request request) async { + // This is a silly way to do this. We're hard-coding the directories here, and then making static servers for them, + // because I don't know how to make the rewrite happen within the handler and I don't want to inline all of the static + // server code. + + // First, check if the file exists in the directories that we just serve statically. + var path = request.url.path; + for (var dir in knownDirectories) { + var filePath = p.join(dir, path); + if (await File(filePath).exists()) { + return request; + } + } + + // For each pattern in [replacements], if the path matches it, rewrite it + // and return a modified request. + Request newRequest; + for (var pattern in replacementRules.keys) { + if (path.endsWith(pattern)) { + newRequest = + await rewriteFrom(pattern, replacementRules[pattern], request); + } + } + return newRequest; +} + +// We already know that [file] ends with [pattern]. Replace [pattern] with [replacement], and return a new request. +Future rewriteFrom( + String pattern, String replacement, Request request) async { + var path = request.url.path; + var firstPart = path.substring(0, path.length - pattern.length); + var newPath = '$firstPart$replacement'; + var newUrl = request.requestedUri.replace(path: newPath); + print("Rewrote $path into $newPath"); + return Request('GET', newUrl); +} + +///------------------------------------------------------------------------------------------------------- +///------------------------------------------------------------------------------------------------------- +///-------------------------------- This is just copied from shelf_proxy because its prereqs are a problem +///-------------------------------- Get rid of it. +///------------------------------------------------------------------------------------------------------- +///------------------------------------------------------------------------------------------------------- +////// A handler that proxies requests to [url]. +/// +/// To generate the proxy request, this concatenates [url] and [Request.url]. +/// This means that if the handler mounted under `/documentation` and [url] is +/// `http://example.com/docs`, a request to `/documentation/tutorials` +/// will be proxied to `http://example.com/docs/tutorials`. +/// +/// [url] must be a [String] or [Uri]. +/// +/// [client] is used internally to make HTTP requests. It defaults to a +/// `dart:io`-based client. +/// +/// [proxyName] is used in headers to identify this proxy. It should be a valid +/// HTTP token or a hostname. It defaults to `shelf_proxy`. +Handler proxyHandler(url, {http.Client client, String proxyName}) { + Uri uri; + if (url is String) { + uri = Uri.parse(url); + } else if (url is Uri) { + uri = url; + } else { + throw ArgumentError.value(url, 'url', 'url must be a String or Uri.'); + } + final nonNullClient = client ?? http.Client(); + proxyName ??= 'shelf_proxy'; + + return (serverRequest) async { + // TODO(nweiz): Support WebSocket requests. + + // TODO(nweiz): Handle TRACE requests correctly. See + // http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.8 + final requestUrl = uri.resolve(serverRequest.url.toString()); + final clientRequest = http.StreamedRequest(serverRequest.method, requestUrl) + ..followRedirects = false + ..headers.addAll(serverRequest.headers) + ..headers['Host'] = uri.authority; + + // Add a Via header. See + // http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.45 + _addHeader(clientRequest.headers, 'via', + '${serverRequest.protocolVersion} $proxyName'); + + serverRequest + .read() + .forEach(clientRequest.sink.add) + .catchError(clientRequest.sink.addError) + .whenComplete(clientRequest.sink.close); + final clientResponse = await nonNullClient.send(clientRequest); + // Add a Via header. See + // http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.45 + _addHeader(clientResponse.headers, 'via', '1.1 $proxyName'); + + // Remove the transfer-encoding since the body has already been decoded by + // [client]. + clientResponse.headers.remove('transfer-encoding'); + + // If the original response was gzipped, it will be decoded by [client] + // and we'll have no way of knowing its actual content-length. + if (clientResponse.headers['content-encoding'] == 'gzip') { + clientResponse.headers.remove('content-encoding'); + clientResponse.headers.remove('content-length'); + + // Add a Warning header. See + // http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.5.2 + _addHeader( + clientResponse.headers, 'warning', '214 $proxyName "GZIP decoded"'); + } + + // Make sure the Location header is pointing to the proxy server rather + // than the destination server, if possible. + if (clientResponse.isRedirect && + clientResponse.headers.containsKey('location')) { + final location = + requestUrl.resolve(clientResponse.headers['location']).toString(); + if (p.url.isWithin(uri.toString(), location)) { + clientResponse.headers['location'] = + '/${p.url.relative(location, from: uri.toString())}'; + } else { + clientResponse.headers['location'] = location; + } + } + + return Response(clientResponse.statusCode, + body: clientResponse.stream, headers: clientResponse.headers); + }; +} + +// TODO(nweiz): use built-in methods for this when http and shelf support them. +/// Add a header with [name] and [value] to [headers], handling existing headers +/// gracefully. +void _addHeader(Map headers, String name, String value) { + final existing = headers[name]; + headers[name] = existing == null ? value : '$existing, $value'; +} diff --git a/lib/dart_dev.dart b/lib/dart_dev.dart index e266ae58..a947064c 100644 --- a/lib/dart_dev.dart +++ b/lib/dart_dev.dart @@ -7,6 +7,7 @@ export 'src/tools/compound_tool.dart' export 'src/tools/format_tool.dart' show FormatMode, Formatter, FormatterInputs, FormatTool; export 'src/tools/process_tool.dart' show BackgroundProcessTool, ProcessTool; +export 'src/tools/tdd_tool.dart' show TddTool; export 'src/tools/test_tool.dart' show TestTool; export 'src/tools/tuneup_check_tool.dart' show TuneupCheckTool; export 'src/tools/webdev_serve_tool.dart' show WebdevServeTool; diff --git a/lib/src/core_config.dart b/lib/src/core_config.dart index 0df31897..5292dcd8 100644 --- a/lib/src/core_config.dart +++ b/lib/src/core_config.dart @@ -9,4 +9,5 @@ Map get coreConfig => { 'analyze': AnalyzeTool(), 'format': FormatTool(), 'test': TestTool(), + 'tdd': TddTool(), }; diff --git a/lib/src/tools/tdd_tool.dart b/lib/src/tools/tdd_tool.dart new file mode 100644 index 00000000..e48c90bf --- /dev/null +++ b/lib/src/tools/tdd_tool.dart @@ -0,0 +1,292 @@ +import 'dart:async'; +import 'dart:io'; + +import 'package:args/args.dart'; +import 'package:args/command_runner.dart'; +import 'package:io/ansi.dart'; +import 'package:io/io.dart'; +import 'package:logging/logging.dart'; + +import '../dart_dev_tool.dart'; +import '../utils/arg_results_utils.dart'; +import '../utils/logging.dart'; +import '../utils/package_is_immediate_dependency.dart'; +import '../utils/process_declaration.dart'; +import '../utils/run_process_and_ensure_exit.dart'; + +final _log = Logger('Test'); + +/// A dart_dev tool that repeatedly runs dart browser tests for the current project. +/// +/// #### This is all different Tests will be run via `dart test` unless the current project depends on +/// `build_test`, in which case it will run tests via +/// `dart run build_runner test`. +/// +/// To use this tool in your project, include it in the dart_dev config in +/// `tool/dart_dev/config.dart`: +/// import 'package:dart_dev/dart_dev.dart'; +/// +/// final config = { +/// 'test': TestTool(), +/// }; +/// +/// This will make it available via the `dart_dev` command-line app like so: +/// dart run dart_dev test +/// +/// This tool can be configured by modifying any of its fields: +/// // tool/dart_dev/config.dart +/// import 'package:dart_dev/dart_dev.dart'; +/// +/// final config = { +/// 'test': TestTool() +/// ..buildArgs = ['--delete-conflicting-outputs'] +/// ..testArgs = ['-P', 'unit'], +/// }; +/// +/// It is also possible to run this tool directly in a dart script: +/// TestTool().run(); +class TddTool extends DevTool { + @override + final ArgParser argParser = ArgParser() + // TODO Options for ports + ..addSeparator('======== Selecting Tests') + ..addMultiOption('name', + abbr: 'n', + help: 'A substring of the name of the test to run.\n' + 'Regular expression syntax is supported.\n' + 'If passed multiple times, tests must match all substrings.', + splitCommas: false) + ..addMultiOption('plain-name', + abbr: 'N', + help: 'A plain-text substring of the name of the test to run.\n' + 'If passed multiple times, tests must match all substrings.', + splitCommas: false) + ..addSeparator('======== Running Tests') + ..addMultiOption('preset', + abbr: 'P', help: 'The configuration preset(s) to use.') + ..addSeparator('======== Output') + ..addSeparator('======== Other Options') + ..addOption('test-args', + help: 'Args to pass to the test runner process.\n' + 'Run "dart test -h" to see all available options.') + ..addOption('build-args', + help: 'Args to pass to the build runner process.\n' + 'Run "dart run build_runner test -h" to see all available ' + 'options.\n' + 'Note: these args are only applicable if the current project ' + 'depends on "build_test".'); + + /// The args to pass to the `dart run build_runner test` process that will be + /// run by this command when the current project depends on `build_test`. + /// + /// Run `dart run build_runner test -h` to see all available args. + List buildArgs; + + @override + String description = + 'Repeatedly run dart browser tests in this package. Assumes you\'re already serving the test directory.'; + + /// The args to pass to the `dart test` process (either directly or + /// through the `dart run build_runner test` process if applicable). + /// + /// Run `dart test -h` to see all available args. + /// + /// Note that most of the command-line options for the `dart test` process + /// also have `dart_test.yaml` configuration counterparts. Rather than + /// configuring this field, it is preferred that the project be configured via + /// `dart_test.yaml` so that the configuration is used even when running tests + /// through some means other than `ddev`. + List testArgs; + + @override + FutureOr run([DevToolExecutionContext context]) { + print("actually running the TDD Tool"); + context ??= DevToolExecutionContext(); + final execution = buildExecution(context, + configuredBuildArgs: buildArgs, configuredTestArgs: testArgs); + print("running $execution"); + return execution.exitCode ?? + runProcessAndEnsureExit(execution.process, log: _log); + } + + @override + Command toCommand(String name) => TestToolCommand(name, this); +} + +class TestToolCommand extends DevToolCommand { + TestToolCommand(String name, DevTool devTool) : super(name, devTool); + + @override + String get usage => + super.usage.replaceFirst('[arguments]', '[files or directories...]'); +} + +/// A declarative representation of an execution of the [TestTool]. +/// +/// This class allows the [TestTool] to break its execution up into two steps: +/// 1. Validation of config/inputs and creation of this class. +/// 2. Execution of expensive or hard-to-test logic based on step 1. +/// +/// As a result, nearly all of the logic in [TestTool] can be tested via the +/// output of step 1 with very simple unit tests. +class TestExecution { + TestExecution.exitEarly(this.exitCode) : process = null; + TestExecution.process(this.process) : exitCode = null; + + /// If non-null, the execution is already complete and the [TestTool] should + /// exit with this code. + /// + /// If null, there is more work to do. + final int exitCode; + + /// A declarative representation of the test process that should be run. + /// + /// This process' result should become the final result of the [TestTool]. + final ProcessDeclaration process; +} + +/// Builds and returns the full list of args for the test process that +/// [TestTool] will start. +/// +/// If [useBuildRunner] is true, the returned args will run tests via +/// `dart run build_runner test`. Additional args targeting the build process +/// will immediately follow and args targeting the test process will follow an +/// arg separator (`--`). +/// +/// If [useBuildRunner] is false, the returned args will run tests via +/// `dart test` and additional args targeting the test process will follow +/// immediately. Build args will be ignored. +/// +/// When building the build args portion of the list, the [configuredBuildArgs] +/// will be included first (if non-null) followed by the value of the +/// `--build-args` option if it and [argResults] are non-null. +/// +/// When building the test args portion of the list, the [configuredTestArgs] +/// will be included first (if non-null), followed by the value of the +/// `--test-args` option if it and [argResults] are non-null, followed by any +/// remaining positional args passed directly to the [TestTool] command. +/// +/// If [verbose] is true, both the build args and the test args portions of the +/// returned list will include the `-v` verbose flag. +List buildArgs({ + ArgResults argResults, + List configuredBuildArgs, + List configuredTestArgs, + bool useBuildRunner, + bool verbose, +}) { + useBuildRunner ??= false; + verbose ??= false; + + final buildArgs = [ + // Combine all args that should be passed through to the build_runner + // process in this order: + // 1. Statically configured args from [WebdevServeTool.buildArgs] + ...?configuredBuildArgs, + // 3. Build filters to narrow the build to only the target tests. + // (If no test dirs/files are passed in as args, then no build filters + // will be created.) + ...buildFiltersForTestArgs(argResults?.rest), + // 4. Args passed to --build-args + ...?splitSingleOptionValue(argResults, 'build-args'), + ]; + + final testArgs = [ + // Combine all args that should be passed through to the webdev serve + // process in this order: + // 1. Statically configured args from [TestTool.testArgs] + ...?configuredTestArgs, + // 3. The -n|--name, -N|--plain-name, and -P|--preset options + ...?multiOptionValue(argResults, 'name')?.map((v) => '--name=$v'), + ...?multiOptionValue(argResults, 'plain-name') + ?.map((v) => '--plain-name=$v'), + ...?multiOptionValue(argResults, 'preset')?.map((v) => '--preset=$v'), + // 4. Args passed to --test-args + ...?splitSingleOptionValue(argResults, 'test-args'), + // 5. Rest args passed to this command + ...?argResults?.rest, + ]; + + if (verbose) { + if (!buildArgs.contains('-v') && !buildArgs.contains('--verbose')) { + buildArgs.add('-v'); + } + } + + return [ + // Add the args targeting the build_runner command. + // if (useBuildRunner) ...buildArgs, + // if (useBuildRunner && testArgs.isNotEmpty) '--', + + // Add the args targeting the test command. + ...testArgs, + ]; +} + +/// Returns a declarative representation of a test process to run based on the +/// given parameters. +/// +/// These parameters will be populated from [TestTool] when it is executed +/// (either directly or via a command-line app). +/// +/// [context] is the execution context that would be provided by [TestTool] when +/// converted to a [DevToolCommand]. For tests, this can be manually created to +/// to imitate the various CLI inputs. +/// +/// [configuredBuildArgs] will be populated from [TestTool.buildArgs]. +/// +/// [configuredTestArgs] will be populated from [TestTool.testArgs]. +/// +/// If non-null, [path] will override the current working directory for any +/// operations that require it. This is intended for use by tests. +/// +/// The [TestTool] can be tested almost completely via this function by +/// enumerating all of the possible parameter variations and making assertions +/// on the declarative output. +TestExecution buildExecution( + DevToolExecutionContext context, { + List configuredBuildArgs, + List configuredTestArgs, + String path, +}) { + print("building execution"); + if (!packageIsImmediateDependency('test', path: path)) { + _log.severe(red.wrap('Cannot run tests.\n') + + yellow.wrap('You must have a dependency on "test" in pubspec.yaml.\n')); + return TestExecution.exitEarly(ExitCode.config.code); + } + + final args = buildArgs( + argResults: context.argResults, + configuredBuildArgs: configuredBuildArgs, + configuredTestArgs: configuredTestArgs, + verbose: context.verbose); + // I think this will leave the proxy running, which is bad. + var hackyScript = + "dart run dart_dev:test_server & while true; do dart run test -p chrome -j 1 --pub-serve=8088 ${args.join(' ')}; read -r ignored; done"; + print("Got hacky script = $hackyScript"); + logSubprocessHeader(_log, 'dart ${args.join(' ')}'.trim()); + return TestExecution.process(ProcessDeclaration( + '/bin/sh', ['-c', hackyScript], + 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 testArgs) { + final testInputs = (testArgs ?? []).where((arg) => arg.startsWith('test')); + final filters = []; + for (final input in testInputs) { + if (input.endsWith('.dart')) { + filters..add('$input.*_test.dart.js*')..add(dartExtToHtml(input)); + } else { + filters.add('$input**'); + } + } + return [for (final filter in filters) '--build-filter=$filter']; +} + +String dartExtToHtml(String input) => + '${input.substring(0, input.length - 'dart'.length)}html'; diff --git a/lib/src/utils/run_process_and_ensure_exit.dart b/lib/src/utils/run_process_and_ensure_exit.dart index 183bd5ff..7c33c2fa 100644 --- a/lib/src/utils/run_process_and_ensure_exit.dart +++ b/lib/src/utils/run_process_and_ensure_exit.dart @@ -2,11 +2,12 @@ import 'dart:io'; import 'package:logging/logging.dart'; -import 'process_declaration.dart'; import 'ensure_process_exit.dart'; +import 'process_declaration.dart'; Future runProcessAndEnsureExit(ProcessDeclaration processDeclaration, {Logger log}) async { + print("starting actual process ${processDeclaration.args}"); final process = await Process.start( processDeclaration.executable, processDeclaration.args, mode: processDeclaration.mode ?? ProcessStartMode.normal, diff --git a/pubspec.yaml b/pubspec.yaml index 1d56cc4a..4677d62b 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -13,6 +13,7 @@ dependencies: build_runner: ^1.7.0 completion: ^0.2.1+1 glob: ^1.1.7 + http: ^0.12.0 io: ^0.3.3 meta: ">=1.2.2 <1.7.0" # Workaround to avoid https://github.com/dart-lang/sdk/issues/46142 logging: ^0.11.3+2