From 1c5d3dfd4aecd0dcd7056532116e749ff310e4e4 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Wed, 12 Aug 2020 23:15:38 +0000 Subject: [PATCH] Run web static error tests on dart2js and DDC and validate output. If a static error test contains "[web]" error markers, this runs them on DDC and dart2js, parses the output, and tests that the compilers report the expected errors. Change-Id: I8b846f75828a0078f61c2fc1f3da0a4a587349cb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/158100 Commit-Queue: Bob Nystrom Auto-Submit: Bob Nystrom Reviewed-by: Srujan Gaddam --- pkg/test_runner/lib/src/command.dart | 3 + pkg/test_runner/lib/src/command_output.dart | 104 ++++++++++++++++-- pkg/test_runner/lib/src/test_file.dart | 8 ++ pkg/test_runner/lib/src/test_suite.dart | 31 +++--- .../constructor_parameters_static_test.dart | 22 ++-- tests/lib/js/js_annotation_static_test.dart | 12 +- tests/lib/js/parameters_static_test.dart | 20 ++-- 7 files changed, 146 insertions(+), 54 deletions(-) diff --git a/pkg/test_runner/lib/src/command.dart b/pkg/test_runner/lib/src/command.dart index ec9eda395af..ae9fb110098 100644 --- a/pkg/test_runner/lib/src/command.dart +++ b/pkg/test_runner/lib/src/command.dart @@ -180,6 +180,9 @@ class CompilationCommand extends ProcessCommand { if (displayName == 'precompiler' || displayName == 'app_jit') { return VMCommandOutput( this, exitCode, timedOut, stdout, stderr, time, pid); + } else if (displayName == 'dart2js') { + return Dart2jsCompilerCommandOutput( + this, exitCode, timedOut, stdout, stderr, time, compilationSkipped); } else if (displayName == 'dartdevc') { return DevCompilerCommandOutput(this, exitCode, timedOut, stdout, stderr, time, compilationSkipped, pid); diff --git a/pkg/test_runner/lib/src/command_output.dart b/pkg/test_runner/lib/src/command_output.dart index eb548077df4..c55896c7a59 100644 --- a/pkg/test_runner/lib/src/command_output.dart +++ b/pkg/test_runner/lib/src/command_output.dart @@ -967,7 +967,56 @@ class CompilationCommandOutput extends CommandOutput { } } -class DevCompilerCommandOutput extends CommandOutput { +class Dart2jsCompilerCommandOutput extends CompilationCommandOutput + with _StaticErrorOutput { + /// Matches the location and message of a dart2js error message, which looks + /// like: + /// + /// tests/language_2/some_test.dart:9:3: + /// Error: Some message. + /// BadThing(); + /// ^ + /// + /// The test runner only validates the main error message, and not the + /// suggested fixes, so we only parse the first line. + static final _errorRegexp = + RegExp(r"^([^:]+):(\d+):(\d+):\nError: (.*)$", multiLine: true); + + Dart2jsCompilerCommandOutput( + Command command, + int exitCode, + bool timedOut, + List stdout, + List stderr, + Duration time, + bool compilationSkipped) + : super(command, exitCode, timedOut, stdout, stderr, time, + compilationSkipped); + + @override + void _parseErrors() { + var errors = []; + _StaticErrorOutput._parseCfeErrors( + ErrorSource.web, _errorRegexp, decodeUtf8(stdout), errors); + errors.forEach(addError); + } +} + +class DevCompilerCommandOutput extends CommandOutput with _StaticErrorOutput { + /// Matches the first line of a DDC error message. DDC prints errors to + /// stdout that look like: + /// + /// org-dartlang-app:/tests/language_2/some_test.dart:7:21: Error: Some message. + /// Try fixing the code to be less bad. + /// var _ = [if (1) 2]; + /// ^ + /// + /// The test runner only validates the main error message, and not the + /// suggested fixes, so we only parse the first line. + static final _errorRegexp = RegExp( + r"^org-dartlang-app:/([^:]+):(\d+):(\d+): Error: (.*)$", + multiLine: true); + DevCompilerCommandOutput( Command command, int exitCode, @@ -985,6 +1034,11 @@ class DevCompilerCommandOutput extends CommandOutput { if (hasTimedOut) return Expectation.timeout; if (hasNonUtf8) return Expectation.nonUtf8Error; + // If it's a static error test, validate the exact errors. + if (testCase.testFile.isStaticErrorTest) { + return _validateExpectedErrors(testCase); + } + // Handle errors / missing errors if (testCase.hasCompileError) { return exitCode == 0 @@ -1001,9 +1055,24 @@ class DevCompilerCommandOutput extends CommandOutput { if (hasCrashed) return Expectation.crash; if (hasTimedOut) return Expectation.timeout; if (hasNonUtf8) return Expectation.nonUtf8Error; + + // If it's a static error test, validate the exact errors. + if (testCase.testFile.isStaticErrorTest) { + return _validateExpectedErrors(testCase); + } + if (exitCode != 0) return Expectation.compileTimeError; + return Expectation.pass; } + + @override + void _parseErrors() { + var errors = []; + _StaticErrorOutput._parseCfeErrors( + ErrorSource.web, _errorRegexp, decodeUtf8(stdout), errors); + errors.forEach(addError); + } } class VMKernelCompilationCommandOutput extends CompilationCommandOutput { @@ -1178,13 +1247,8 @@ class FastaCommandOutput extends CompilationCommandOutput with _StaticErrorOutput { static void parseErrors(String stdout, List errors, [List warnings]) { - for (var match in _errorRegexp.allMatches(stdout)) { - var line = int.parse(match.group(2)); - var column = int.parse(match.group(3)); - var message = match.group(4); - errors.add( - StaticError({ErrorSource.cfe: message}, line: line, column: column)); - } + _StaticErrorOutput._parseCfeErrors( + ErrorSource.cfe, _errorRegexp, stdout, errors); } /// Matches the first line of a Fasta error message. Fasta prints errors to @@ -1222,6 +1286,19 @@ class FastaCommandOutput extends CompilationCommandOutput /// Mixin for outputs from a command that implement a Dart front end which /// reports static errors. mixin _StaticErrorOutput on CommandOutput { + /// Parses compile errors reported by CFE using the given [regExp] and adds + /// them to [errors] as coming from [errorSource]. + static void _parseCfeErrors(ErrorSource errorSource, RegExp regExp, + String stdout, List errors) { + for (var match in regExp.allMatches(stdout)) { + var line = int.parse(match.group(2)); + var column = int.parse(match.group(3)); + var message = match.group(4); + errors + .add(StaticError({errorSource: message}, line: line, column: column)); + } + } + /// Reported static errors, parsed from [stderr]. List get errors { if (!_parsedErrors) { @@ -1310,9 +1387,14 @@ mixin _StaticErrorOutput on CommandOutput { Expectation _validateExpectedErrors(TestCase testCase, [OutputWriter writer]) { // Filter out errors that aren't for this configuration. - var errorSource = testCase.configuration.compiler == Compiler.dart2analyzer - ? ErrorSource.analyzer - : ErrorSource.cfe; + var errorSource = { + Compiler.dart2analyzer: ErrorSource.analyzer, + Compiler.dart2js: ErrorSource.web, + Compiler.dartdevc: ErrorSource.web, + Compiler.fasta: ErrorSource.cfe + }[testCase.configuration.compiler]; + assert(errorSource != null); + var expected = testCase.testFile.expectedErrors .where((error) => error.hasError(errorSource)); diff --git a/pkg/test_runner/lib/src/test_file.dart b/pkg/test_runner/lib/src/test_file.dart index c5a0fd88619..01afcd935e5 100644 --- a/pkg/test_runner/lib/src/test_file.dart +++ b/pkg/test_runner/lib/src/test_file.dart @@ -72,6 +72,14 @@ abstract class _TestFileBase { /// is skipped on configurations that are not purely front end. bool get isStaticErrorTest => expectedErrors.isNotEmpty; + /// If the test contains any web-specific (`[web]`) static error expectations, + /// then it's a "web static error test". + /// + /// These tests exist to validate that a Dart web compiler reports the right + /// expected errors. + bool get isWebStaticErrorTest => + expectedErrors.any((error) => error.hasError(ErrorSource.web)); + /// If the tests has no static error expectations, or all of the expectations /// are warnings, then the test tests runtime semantics. /// diff --git a/pkg/test_runner/lib/src/test_suite.dart b/pkg/test_runner/lib/src/test_suite.dart index d05d3c1a3c1..2a2bfb4ac23 100644 --- a/pkg/test_runner/lib/src/test_suite.dart +++ b/pkg/test_runner/lib/src/test_suite.dart @@ -165,7 +165,19 @@ abstract class TestSuite { } } - return true; + // Normal runtime tests are always run. + if (testFile.isRuntimeTest) return true; + + // Tests of web-specific static errors are run on web compilers. + if (testFile.isWebStaticErrorTest && + (configuration.compiler == Compiler.dart2js || + configuration.compiler == Compiler.dartdevc)) { + return true; + } + + // Other static error tests are run on front-end-only configurations. + return configuration.compiler == Compiler.dart2analyzer || + configuration.compiler == Compiler.fasta; } /// Whether a test with [expectations] should be skipped under the current @@ -557,19 +569,6 @@ class StandardTestSuite extends TestSuite { /// options. void _testCasesFromTestFile( TestFile testFile, ExpectationSet expectations, TestCaseEvent onTest) { - // TODO(rnystrom): Skipping this here is a little unusual because most - // skips are handled in _addTestCase(). However, if the configuration - // is running on a browser, calling _addTestCase() will try to create - // a set of commands which ultimately causes an exception in - // DummyRuntimeConfiguration. This avoids that. - // If the test only has static expectations, skip it on any configurations - // that are not purely front ends. - if (!testFile.isRuntimeTest && - configuration.compiler != Compiler.dart2analyzer && - configuration.compiler != Compiler.fasta) { - return; - } - // The configuration must support everything the test needs. if (!configuration.supportedFeatures.containsAll(testFile.requirements)) { return; @@ -690,7 +689,7 @@ class StandardTestSuite extends TestSuite { commands.addAll(compilationArtifact.commands); } - if (testFile.hasCompileError && + if ((testFile.hasCompileError || testFile.isStaticErrorTest) && compilerConfiguration.hasCompiler && !compilerConfiguration.runRuntimeDespiteMissingCompileTimeError) { // Do not attempt to run the compiled result. A compilation @@ -744,9 +743,9 @@ class StandardTestSuite extends TestSuite { return "/$prefixDartDir/$fileRelativeToDartDir"; } - // Unreachable. print("Cannot create URL for path $file. Not in build or dart directory."); exit(1); + throw "unreachable"; } String _uriForBrowserTest(String pathComponent) { diff --git a/tests/lib/js/constructor_parameters_static_test.dart b/tests/lib/js/constructor_parameters_static_test.dart index 3efa2a2ed5f..82b7b1ee638 100644 --- a/tests/lib/js/constructor_parameters_static_test.dart +++ b/tests/lib/js/constructor_parameters_static_test.dart @@ -17,10 +17,10 @@ import 'package:expect/expect.dart'; class Foo { external Foo({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external factory Foo.fooFactory({int? a}); - // ^ - // [web] TODO(srujzs): Add error once supported. + // ^ + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } @JS() @@ -28,21 +28,21 @@ class Foo { class Bar { external Bar({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. // Factories of an anonymous class can only contain named parameters. external factory Bar.barFactoryPositional(int? a); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Factory constructors for @anonymous JS interop classes should not contain any positional parameters. external factory Bar.barFactoryOptional([int? a]); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Factory constructors for @anonymous JS interop classes should not contain any positional parameters. external factory Bar.barFactoryMixedOptional(int? a, [int? b]); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Factory constructors for @anonymous JS interop classes should not contain any positional parameters. external factory Bar.barFactoryMixedNamed(int? a, {int? b}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Factory constructors for @anonymous JS interop classes should not contain any positional parameters. // Named parameters are okay only for factories of an anonymous class. external factory Bar.barFactoryNamed({int? a}); @@ -52,10 +52,10 @@ class Bar { abstract class Baz { external Baz({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external factory Baz.bazFactory({int? a}); - // ^ - // [web] TODO(srujzs): Add error once supported. + // ^ + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } main() {} diff --git a/tests/lib/js/js_annotation_static_test.dart b/tests/lib/js/js_annotation_static_test.dart index ef7dc40e91c..a8d7cd7188a 100644 --- a/tests/lib/js/js_annotation_static_test.dart +++ b/tests/lib/js/js_annotation_static_test.dart @@ -17,27 +17,27 @@ class Foo { @JS() external Foo(int bar); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. @JS() external factory Foo.fooFactory(); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. @JS() external int get bar; // ^^^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. @JS() external set bar(int val); // ^^^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. @JS() external int baz(); // ^^^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. @JS() external static int bazStatic(); // ^^^^^^^^^ - // [web] TODO(srujzs): Add error once supported. + // [web] Member has a JS interop annotation but the enclosing class does not. } @JS() diff --git a/tests/lib/js/parameters_static_test.dart b/tests/lib/js/parameters_static_test.dart index 0928ca5070c..11f875a3383 100644 --- a/tests/lib/js/parameters_static_test.dart +++ b/tests/lib/js/parameters_static_test.dart @@ -16,48 +16,48 @@ import 'package:expect/expect.dart'; class Foo { external int singleNamedArg({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external int mixedNamedArgs(int a, {int? b}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } @JS() class Bar { external static int singleNamedArg({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external static int mixedNamedArgs(int a, {int? b}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } external int singleNamedArg({int? a}); // ^ -// [web] TODO(srujzs): Add error once supported. +// [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external int mixedNamedArgs(int a, {int? b}); // ^ -// [web] TODO(srujzs): Add error once supported. +// [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. @JS() @anonymous class Baz { external int singleNamedArg({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external int mixedNamedArgs(int a, {int? b}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } @JS() abstract class Qux { external int singleNamedArg({int? a}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. external int mixedNamedArgs(int a, {int? b}); // ^ - // [web] TODO(srujzs): Add error once supported. + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } main() {}