Skip to content

Commit 5450d08

Browse files
munificentcommit-bot@chromium.org
munificent
authored and
[email protected]
committed
Add support for analyzer static error tests.
If a test has the static error markers and is run on analyzer, the test runner verifies that the reported errors exactly match the expected ones. A test with error markers is skipped on all other configurations. Change-Id: Ib921acde517688f5b47937a5b100c0507e0e9138 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107823 Commit-Queue: Bob Nystrom <[email protected]> Reviewed-by: William Hesse <[email protected]>
1 parent 0425997 commit 5450d08

File tree

8 files changed

+549
-122
lines changed

8 files changed

+549
-122
lines changed

‎pkg/test_runner/lib/src/command_output.dart‎

Lines changed: 122 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'dart:convert';
88
import 'dart:io' as io;
99

1010
import 'package:status_file/expectation.dart';
11+
import 'package:test_runner/src/test_file.dart';
1112

1213
import 'browser_controller.dart';
1314
import 'command.dart';
@@ -128,7 +129,7 @@ class CommandOutput extends UniqueObject {
128129
}
129130

130131
/// Called when producing output for a test failure to describe this output.
131-
void describe(Progress progress, OutputWriter output) {
132+
void describe(TestCase testCase, Progress progress, OutputWriter output) {
132133
output.subsection("exit code");
133134
output.write(exitCode.toString());
134135

@@ -351,7 +352,7 @@ class BrowserCommandOutput extends CommandOutput
351352
return _rawOutcome;
352353
}
353354

354-
void describe(Progress progress, OutputWriter output) {
355+
void describe(TestCase testCase, Progress progress, OutputWriter output) {
355356
if (_jsonResult != null) {
356357
_describeEvents(progress, output);
357358
} else {
@@ -360,7 +361,7 @@ class BrowserCommandOutput extends CommandOutput
360361
output.write(_result.lastKnownMessage);
361362
}
362363

363-
super.describe(progress, output);
364+
super.describe(testCase, progress, output);
364365

365366
if (_result.browserOutput.stdout.isNotEmpty) {
366367
output.subsection("Browser stdout");
@@ -434,10 +435,11 @@ class BrowserCommandOutput extends CommandOutput
434435
}
435436

436437
class AnalysisCommandOutput extends CommandOutput {
437-
// An error line has 8 fields that look like:
438-
// ERROR|COMPILER|MISSING_SOURCE|file:/tmp/t.dart|15|1|24|Missing source.
439-
static const int _errorLevel = 0;
440-
static const int _formattedError = 7;
438+
/// Reported static errors, parsed from [stderr].
439+
final List<StaticError> _errors = [];
440+
441+
/// Reported static warnings, parsed from [stderr].
442+
final List<StaticError> _warnings = [];
441443

442444
AnalysisCommandOutput(
443445
Command command,
@@ -448,7 +450,20 @@ class AnalysisCommandOutput extends CommandOutput {
448450
Duration time,
449451
bool compilationSkipped)
450452
: super(command, exitCode, timedOut, stdout, stderr, time,
451-
compilationSkipped, 0);
453+
compilationSkipped, 0) {
454+
_parseOutput();
455+
}
456+
457+
@override
458+
void describe(TestCase testCase, Progress progress, OutputWriter output) {
459+
if (testCase.testFile.isStaticErrorTest) {
460+
_validateExpectedErrors(testCase, output);
461+
}
462+
463+
if (!testCase.testFile.isStaticErrorTest || progress == Progress.verbose) {
464+
super.describe(testCase, progress, output);
465+
}
466+
}
452467

453468
Expectation result(TestCase testCase) {
454469
// TODO(kustermann): If we run the analyzer not in batch mode, make sure
@@ -460,41 +475,41 @@ class AnalysisCommandOutput extends CommandOutput {
460475
if (hasTimedOut) return Expectation.timeout;
461476
if (hasNonUtf8) return Expectation.nonUtf8Error;
462477

463-
// Get the errors/warnings from the analyzer
464-
var errors = <String>[];
465-
var warnings = <String>[];
466-
parseAnalyzerOutput(errors, warnings);
478+
// If it's a static error test, validate the exact errors.
479+
if (testCase.testFile.isStaticErrorTest) {
480+
return _validateExpectedErrors(testCase);
481+
}
467482

468483
// Handle negative
469484
if (testCase.isNegative) {
470-
return errors.isNotEmpty
485+
return _errors.isNotEmpty
471486
? Expectation.pass
472487
: Expectation.missingCompileTimeError;
473488
}
474489

475490
// Handle errors / missing errors
476491
if (testCase.hasCompileError) {
477-
if (errors.isNotEmpty) {
492+
if (_errors.isNotEmpty) {
478493
return Expectation.pass;
479494
}
480495
return Expectation.missingCompileTimeError;
481496
}
482-
if (errors.isNotEmpty) {
497+
if (_errors.isNotEmpty) {
483498
return Expectation.compileTimeError;
484499
}
485500

486501
// Handle static warnings / missing static warnings
487502
if (testCase.hasStaticWarning) {
488-
if (warnings.isNotEmpty) {
503+
if (_warnings.isNotEmpty) {
489504
return Expectation.pass;
490505
}
491506
return Expectation.missingStaticWarning;
492507
}
493-
if (warnings.isNotEmpty) {
508+
if (_warnings.isNotEmpty) {
494509
return Expectation.staticWarning;
495510
}
496511

497-
assert(errors.isEmpty && warnings.isEmpty);
512+
assert(_errors.isEmpty && _warnings.isEmpty);
498513
assert(!testCase.hasCompileError && !testCase.hasStaticWarning);
499514
return Expectation.pass;
500515
}
@@ -511,23 +526,32 @@ class AnalysisCommandOutput extends CommandOutput {
511526
if (hasTimedOut) return Expectation.timeout;
512527
if (hasNonUtf8) return Expectation.nonUtf8Error;
513528

514-
// Get the errors/warnings from the analyzer
515-
var errors = <String>[];
516-
var warnings = <String>[];
517-
parseAnalyzerOutput(errors, warnings);
529+
// If it's a static error test, validate the exact errors.
530+
if (testCase.testFile.isStaticErrorTest) {
531+
return _validateExpectedErrors(testCase);
532+
}
518533

519-
if (errors.isNotEmpty) {
534+
if (_errors.isNotEmpty) {
520535
return Expectation.compileTimeError;
521536
}
522-
if (warnings.isNotEmpty) {
537+
if (_warnings.isNotEmpty) {
523538
return Expectation.staticWarning;
524539
}
525540
return Expectation.pass;
526541
}
527542

528-
void parseAnalyzerOutput(List<String> outErrors, List<String> outWarnings) {
529-
// Parse a line delimited by the | character using \ as an escape character
530-
// like: FOO|BAR|FOO\|BAR|FOO\\BAZ as 4 fields: FOO BAR FOO|BAR FOO\BAZ
543+
/// Parses the machine-readable output of analyzer, which looks like:
544+
///
545+
/// ERROR|STATIC_TYPE_WARNING|SOME_ERROR_CODE|/path/to/some_test.dart|9|26|1|Error message.
546+
///
547+
/// Pipes can be escaped with backslashes:
548+
///
549+
/// FOO|BAR|FOO\|BAR|FOO\\BAZ
550+
///
551+
/// Is parsed as:
552+
///
553+
/// FOO BAR FOO|BAR FOO\BAZ
554+
void _parseOutput() {
531555
List<String> splitMachineError(String line) {
532556
var field = StringBuffer();
533557
var result = <String>[];
@@ -550,20 +574,82 @@ class AnalysisCommandOutput extends CommandOutput {
550574
return result;
551575
}
552576

553-
for (String line in decodeUtf8(super.stderr).split("\n")) {
577+
for (var line in decodeUtf8(stderr).split("\n")) {
554578
if (line.isEmpty) continue;
555579

556-
List<String> fields = splitMachineError(line);
557-
// We only consider errors/warnings for files of interest.
558-
if (fields.length > _formattedError) {
559-
if (fields[_errorLevel] == 'ERROR') {
560-
outErrors.add(fields[_formattedError]);
561-
} else if (fields[_errorLevel] == 'WARNING') {
562-
outWarnings.add(fields[_formattedError]);
580+
var fields = splitMachineError(line);
581+
582+
// Lines without enough fields are other output we don't care about.
583+
if (fields.length >= 8) {
584+
var severity = fields[0];
585+
var errorCode = "${fields[1]}.${fields[2]}";
586+
var line = int.parse(fields[4]);
587+
var column = int.parse(fields[5]);
588+
var length = int.parse(fields[6]);
589+
590+
var error = StaticError(
591+
line: line, column: column, length: length, code: errorCode);
592+
593+
if (severity == 'ERROR') {
594+
_errors.add(error);
595+
} else if (severity == 'WARNING') {
596+
_warnings.add(error);
597+
}
598+
}
599+
}
600+
}
601+
602+
// TODO(rnystrom): This will probably move up to CommandOutput once other
603+
// front ends support static error tests.
604+
/// Compare the actual errors produced to the expected static errors parsed
605+
/// from the test file.
606+
///
607+
/// Returns [Expectation.pass] if all expected errors were correctly
608+
/// reported.
609+
///
610+
/// If [writer] is given, outputs a description of any error mismatches.
611+
Expectation _validateExpectedErrors(TestCase testCase,
612+
[OutputWriter writer]) {
613+
// Don't require the test or analyzer to output in any specific order.
614+
var expected = testCase.testFile.expectedErrors.toList();
615+
var actual = _errors.toList();
616+
expected.sort();
617+
actual.sort();
618+
619+
if (writer != null) writer.subsection("incorrect static errors");
620+
621+
var success = expected.length == actual.length;
622+
for (var i = 0; i < expected.length && i < actual.length; i++) {
623+
var differences = expected[i].describeDifferences(actual[i]);
624+
if (differences == null) continue;
625+
626+
if (writer != null) {
627+
writer.write(actual[i].location);
628+
for (var difference in differences) {
629+
writer.write("- $difference");
563630
}
564-
// OK to Skip error output that doesn't match the machine format.
631+
writer.separator();
632+
}
633+
634+
success = false;
635+
}
636+
637+
if (writer != null) {
638+
writer.subsection("missing expected static errors");
639+
for (var i = actual.length; i < expected.length; i++) {
640+
writer.write(expected[i].toString());
641+
writer.separator();
642+
}
643+
644+
writer.subsection("reported unexpected static errors");
645+
for (var i = expected.length; i < actual.length; i++) {
646+
writer.write(actual[i].toString());
647+
writer.separator();
565648
}
566649
}
650+
651+
// TODO(rnystrom): Is there a better expectation we can use?
652+
return success ? Expectation.pass : Expectation.missingCompileTimeError;
567653
}
568654
}
569655

‎pkg/test_runner/lib/src/process_queue.dart‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,7 +362,7 @@ class CommandEnqueuer {
362362
///
363363
/// It provides a [done] future, which will complete once there are no more
364364
/// nodes left in the states Initialized/Waiting/Enqueing/Processing
365-
/// and the [executor] has cleaned up it's resources.
365+
/// and the [executor] has cleaned up its resources.
366366
class CommandQueue {
367367
final Graph<Command> graph;
368368
final CommandExecutor executor;

‎pkg/test_runner/lib/src/test_case.dart‎

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,6 @@ const _excludedEnvironmentVariables = [
4949
/// The TestCase has a callback function, [completedHandler], that is run when
5050
/// the test is completed.
5151
class TestCase extends UniqueObject {
52-
/// Flags set in _expectations from the optional argument info.
53-
static const _hasRuntimeError = 1 << 0;
54-
static const _hasSyntaxError = 1 << 1;
55-
static const _hasCompileError = 1 << 2;
56-
static const _hasStaticWarning = 1 << 3;
57-
static const _hasCrash = 1 << 4;
58-
5952
/// A list of commands to execute. Most test cases have a single command.
6053
/// Dart2js tests have two commands, one to compile the source and another
6154
/// to execute it. Some isolate tests might even have three, if they require
@@ -65,47 +58,35 @@ class TestCase extends UniqueObject {
6558

6659
TestConfiguration configuration;
6760
String displayName;
68-
int _expectations = 0;
6961
int hash = 0;
7062
Set<Expectation> expectedOutcomes;
63+
final TestFile testFile;
7164

7265
TestCase(this.displayName, this.commands, this.configuration,
7366
this.expectedOutcomes,
74-
{TestFile testFile}) {
67+
{TestFile testFile})
68+
: testFile = testFile {
7569
// A test case should do something.
7670
assert(commands.isNotEmpty);
7771

7872
if (testFile != null) {
79-
_setExpectations(testFile);
8073
hash = (testFile.originPath?.relativeTo(Repository.dir)?.toString())
8174
.hashCode;
8275
}
8376
}
8477

85-
void _setExpectations(TestFile testFile) {
86-
// We don't want to keep the entire (large) TestInformation structure,
87-
// so we copy the needed bools into flags set in a single integer.
88-
if (testFile.hasRuntimeError) _expectations |= _hasRuntimeError;
89-
if (testFile.hasSyntaxError) _expectations |= _hasSyntaxError;
90-
if (testFile.hasCrash) _expectations |= _hasCrash;
91-
if (testFile.hasCompileError || testFile.hasSyntaxError) {
92-
_expectations |= _hasCompileError;
93-
}
94-
if (testFile.hasStaticWarning) _expectations |= _hasStaticWarning;
95-
}
96-
9778
TestCase indexedCopy(int index) {
9879
var newCommands = commands.map((c) => c.indexedCopy(index)).toList();
99-
return TestCase(displayName, newCommands, configuration, expectedOutcomes)
100-
.._expectations = _expectations
80+
return TestCase(displayName, newCommands, configuration, expectedOutcomes,
81+
testFile: testFile)
10182
..hash = hash;
10283
}
10384

104-
bool get hasRuntimeError => _expectations & _hasRuntimeError != 0;
105-
bool get hasStaticWarning => _expectations & _hasStaticWarning != 0;
106-
bool get hasSyntaxError => _expectations & _hasSyntaxError != 0;
107-
bool get hasCompileError => _expectations & _hasCompileError != 0;
108-
bool get hasCrash => _expectations & _hasCrash != 0;
85+
bool get hasRuntimeError => testFile?.hasRuntimeError ?? false;
86+
bool get hasStaticWarning => testFile?.hasStaticWarning ?? false;
87+
bool get hasSyntaxError => testFile?.hasSyntaxError ?? false;
88+
bool get hasCompileError => testFile?.hasCompileError ?? false;
89+
bool get hasCrash => testFile?.hasCrash ?? false;
10990
bool get isNegative =>
11091
hasCompileError ||
11192
hasRuntimeError && configuration.runtime != Runtime.none ||

0 commit comments

Comments
 (0)