[CFE] Make unit_test_suites log better message when test starts to pass
In for instance https://dart-review.googlesource.com/c/sdk/+/193961/5
we had an instance of something good happening (a crashing-ish test
that started to pass) but where the author was told no (red bot) and
not given a path to make the bot happy (bad messaging).
Specifically tests failed and the best log one got was
```
To re-run this test, run:
dart pkg/front_end/test/unit_test_suites.dart -p pkg/front_end/test/fasta/textual_outline/general/constants/js_semantics/issue45376
```
which isn't very helpful.
If running it locally a little more information is given via stdout,
but having to do that isn't great (and the little extra information
still wasn't great).
This CL updates the messaging to - hopefully - give a path to make the
bot happy. The messaging will now - directly from the log
on the bot - be
```
FAILED: pkg/front_end/test/fasta/textual_outline/general/constants/js_semantics/issue45376: Pass
To re-run this test, run:
dart pkg/front_end/test/unit_test_suites.dart -p pkg/front_end/test/fasta/textual_outline/general/constants/js_semantics/issue45376
The test passed, but wasn't expected to. You should update the status file for this test.
There's a status entry looking something like
general/constants/js_semantics/issue45376: FormatterCrash
which should be removed.
The status file is file:///[wherever]/sdk/pkg/front_end/testcases/textual_outline.status.
```
which
a) gives the extra information previously only available via stdout when
running locally (the first line).
b) Gives an explanation as to why it failed
("The test passed, but wasn't expected to.").
c) A path to make the bot green ("You should update the status file for
this test. [...] should be removed").
Hopefully this will make it easier to deal with in the future when
something good happens.
Change-Id: I838699abce580334c956d8b4753884fd600802ae
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194242
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
diff --git a/pkg/front_end/test/unit_test_suites_impl.dart b/pkg/front_end/test/unit_test_suites_impl.dart
index c67f5b3..cdead85 100644
--- a/pkg/front_end/test/unit_test_suites_impl.dart
+++ b/pkg/front_end/test/unit_test_suites_impl.dart
@@ -207,15 +207,20 @@
@override
void logSuiteComplete(testing.Suite suite) {}
- handleTestResult(TestDescription testDescription, Result result,
- String fullSuiteName, bool matchedExpectations) {
+ handleTestResult(
+ testing.Suite suite,
+ TestDescription testDescription,
+ Result result,
+ String fullSuiteName,
+ bool matchedExpectations,
+ Set<Expectation> expectedOutcomes) {
String testName = getTestName(testDescription);
- String suite = "pkg";
- String shortTestName = testName.substring(suite.length + 1);
+ String suiteName = "pkg";
+ String shortTestName = testName.substring(suiteName.length + 1);
resultsPort.send(jsonEncode({
"name": testName,
"configuration": configurationName,
- "suite": suite,
+ "suite": suiteName,
"test_name": shortTestName,
"time_ms": stopwatches[testName]!.elapsedMilliseconds,
"expected": "Pass",
@@ -224,6 +229,10 @@
}));
if (!matchedExpectations) {
StringBuffer sb = new StringBuffer();
+ if (printFailureLog) {
+ String outcome = "${result.outcome}";
+ sb.write("FAILED: $testName: $outcome");
+ }
sb.write(result.log);
if (result.error != null) {
sb.write("\n\n${result.error}");
@@ -246,6 +255,26 @@
'used when many tests need updating.\n');
}
}
+ if (result.outcome == Expectation.Pass) {
+ String expectedString =
+ expectedOutcomes.map((e) => e.toString()).join(", ");
+ sb.write("\n\nThe test passed, but wasn't expected to. "
+ "You should update the status file for this test."
+ "\nThere's a status entry looking something like"
+ "\n\n ${testDescription.shortName}: ${expectedString}"
+ "\n\nwhich should be removed."
+ "\n\nThe status file is ${suite.statusFile}.");
+ } else if (result.autoFixCommand == null) {
+ String expectedString =
+ expectedOutcomes.map((e) => e.toString()).join(", ");
+ sb.write("\n\nThe test has outcome ${result.outcome}, "
+ "but was expected to have outcome(s) ${expectedOutcomes}. "
+ "You might have to update the status file to the new outcome"
+ "\nThere's a status entry looking something like"
+ "\n\n ${testDescription.shortName}: ${expectedString}"
+ "\n\nwhich should be updated."
+ "\n\nThe status file is ${suite.statusFile}.");
+ }
String failureLog = sb.toString();
String outcome = "${result.outcome}";
logsPort.send(jsonEncode({
@@ -255,7 +284,6 @@
"log": failureLog,
}));
if (printFailureLog) {
- print('FAILED: $testName: $outcome');
print(failureLog);
}
}
@@ -280,7 +308,8 @@
void logExpectedResult(testing.Suite suite, TestDescription description,
Result result, Set<Expectation> expectedOutcomes) {
- handleTestResult(description, result, prefix, true);
+ handleTestResult(
+ suite, description, result, prefix, true, expectedOutcomes);
}
@override
@@ -292,7 +321,8 @@
String testName = getTestName(description);
if (seenTests.contains(testName)) return;
seenTests.add(testName);
- handleTestResult(description, result, prefix, false);
+ handleTestResult(
+ suite, description, result, prefix, false, expectedOutcomes);
}
@override
