Add suppression to test.dart for flaky content_shell crashes

Add suppression to test.dart for flaky content_shell crashes

BUG=https://github.com/dart-lang/sdk/issues/26739
R=kustermann@google.com

Committed: https://github.com/dart-lang/sdk/commit/7e9a9804d96964a74527b1e067ff94bf02930224

[Bill Hesse, 2016-06-21 13:50:18]

Also cleans up some places that give analyzer hints.

[kustermann, 2016-06-21 14:22:19]

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
File tools/testing/dart/status_file_parser.dart (right):

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
tools/testing/dart/status_file_parser.dart:40: // Do not place in status files.
Maybe *ensure* this in the status file parser?

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
tools/testing/dart/status_file_parser.dart:103: if (outcome == IGNORE) return
true;
This is the controversial line: If you submit this change, then the bot can
suddenly have an issue with the X display and *nonetheless*:

  * build bot will be absolutely green
  * status files will be absolutely empty
  * everybody will be absolutely happy

Even if *none* of the tests are run anymore.

=> So I'm not really a fan of this.

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1066: for (String line in stderrLines) {
I think you can avoid the split into lines & iterate lines. Just grep in the
result of decodeUtf8(stderr)

[Bill Hesse, 2016-06-21 15:33:28]

On 2016/06/21 14:22:19, kustermann wrote:
>
https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
> File tools/testing/dart/status_file_parser.dart (right):
> 
>
https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
> tools/testing/dart/status_file_parser.dart:40: // Do not place in status
files.
> Maybe *ensure* this in the status file parser?
> 
>
https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
> tools/testing/dart/status_file_parser.dart:103: if (outcome == IGNORE) return
> true;
> This is the controversial line: If you submit this change, then the bot can
> suddenly have an issue with the X display and *nonetheless*:
> 
>   * build bot will be absolutely green
>   * status files will be absolutely empty
>   * everybody will be absolutely happy
> 
> Even if *none* of the tests are run anymore.
> 
> => So I'm not really a fan of this.
> 
>
https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
> tools/testing/dart/test_runner.dart:1066: for (String line in stderrLines) {
> I think you can avoid the split into lines & iterate lines. Just grep in the
> result of decodeUtf8(stderr)

To solve the problem of unreported new or mass failures, we can count the number
of infra failures ignoring tests (even split by type), and put a limit of 3 or 5
on them, and only fail if that limit is exceeded.  We could also sent that count
to a logging and monitoring system, which would warn us.  I'll add the count.

[Bill Hesse, 2016-06-24 12:06:59]

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
File tools/testing/dart/status_file_parser.dart (right):

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
tools/testing/dart/status_file_parser.dart:40: // Do not place in status files.
On 2016/06/21 14:22:19, kustermann wrote:
> Maybe *ensure* this in the status file parser?

Not easy to do.  Perhaps other statuses should also be forbidden.

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/status_f...
tools/testing/dart/status_file_parser.dart:103: if (outcome == IGNORE) return
true;
On 2016/06/21 14:22:19, kustermann wrote:
> This is the controversial line: If you submit this change, then the bot can
> suddenly have an issue with the X display and *nonetheless*:
> 
>   * build bot will be absolutely green
>   * status files will be absolutely empty
>   * everybody will be absolutely happy
> 
> Even if *none* of the tests are run anymore.
> 
> => So I'm not really a fan of this.

I have added code to monitor the number of tests that are skipped this way, and
if more than 5 are, the test suite will fail.

Done.

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2081173002/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1066: for (String line in stderrLines) {
On 2016/06/21 14:22:19, kustermann wrote:
> I think you can avoid the split into lines & iterate lines. Just grep in the
> result of decodeUtf8(stderr)

Done.

[kustermann, 2016-06-24 13:36:44]

The code lgtm

But if this CL makes sense depends on how flaky our buildbots are and if there
is no way to fix the underlying flakiness instead of suppressing it. That is up
to you.

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:183: if
(test.lastCommandOutput?.result(test) == Expectation.IGNORE) {
Why the "?" ? There should be at least one command output for every test!

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:187: print("the test infrastructure. 
Output of the last ignored test was:");
You should probably add a comment about notifying you about such issues.

[Bill Hesse, 2016-06-27 12:43:15]

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:183: if
(test.lastCommandOutput?.result(test) == Expectation.IGNORE) {
On 2016/06/24 13:36:44, kustermann wrote:
> Why the "?" ? There should be at least one command output for every test!

I wasn't sure.
Done.

https://codereview.chromium.org/2081173002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_progress.dart:187: print("the test infrastructure. 
Output of the last ignored test was:");
On 2016/06/24 13:36:44, kustermann wrote:
> You should probably add a comment about notifying you about such issues.

Done.

[Bill Hesse, 2016-06-27 12:44:16]

Description was changed from

==========
Add suppression to test.dart for flaky content_shell crashes

BUG=https://github.com/dart-lang/sdk/issues/26739
R=kustermann@google.com
==========

to

==========
Add suppression to test.dart for flaky content_shell crashes

BUG=https://github.com/dart-lang/sdk/issues/26739
R=kustermann@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7e9a9804d96964a74527b1e067ff94bf02930224
==========

[Bill Hesse, 2016-06-27 12:44:18]

Committed patchset #3 (id:40001) manually as
7e9a9804d96964a74527b1e067ff94bf02930224 (presubmit successful).