Don't retry browser tests if the bot is pretty red. Also, retry properly all

Don't retry browser tests if the bot is pretty red. Also, retry properly all
times that DRT crashes.
Committed: https://code.google.com/p/dart/source/detail?r=4370

[Emily Fortuna, 2012-02-17 19:34:23]

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (left):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:208: return (exitCode != 0 && !hasCrashed);
Sometimes when DRT crashes with a memory dump, and the exitCode == 1 and
!hasCrashed == true, meaning we never scan through the error output below to see
if we should retry the test. Therefore, now we're first testing the stderr to
determine if a retry is necessary.

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:713: new RunningProcess(test,
_progress.numFailedTests < 5).start();
Magic number... no me gusta :-(

[Siggi Cherem (dart-lang), 2012-02-17 19:51:38]

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (left):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:208: return (exitCode != 0 && !hasCrashed);
On 2012/02/17 19:34:24, Emily Fortuna wrote:
> Sometimes when DRT crashes with a memory dump, and the exitCode == 1 and
> !hasCrashed == true, meaning we never scan through the error output below to
see
> if we should retry the test. Therefore, now we're first testing the stderr to
> determine if a retry is necessary.

I thought in that case we wouldn't have PASS in the output?

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:254: RunningProcess(TestCase this.testCase,
bool this.allowRetries);
I think when using this.xxx our style is to omit the type.

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:709: // If we've failed more than four
tests, a lot of tests are likely
When is numFailedTest incremented?

I thought that if you fail, retry, and pass, then the numFailedTest is not
incremented.

If that's the case, then you simply need to check _progress.numFailedTests != 0
(as soon as one test is marked as failing after all retries, the bot will be
marked red, so we wouldn't need to retry any other test beyond that point).

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:713: new RunningProcess(test,
_progress.numFailedTests < 5).start();
On 2012/02/17 19:34:24, Emily Fortuna wrote:
> Magic number... no me gusta :-(

maybe we can do != 0?

[Emily Fortuna, 2012-02-17 21:03:03]

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (left):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:208: return (exitCode != 0 && !hasCrashed);
Sometimes it does, and sometimes it doesn't when DRT crashes. It's strange.


On 2012/02/17 19:51:38, sigmund wrote:
> On 2012/02/17 19:34:24, Emily Fortuna wrote:
> > Sometimes when DRT crashes with a memory dump, and the exitCode == 1 and
> > !hasCrashed == true, meaning we never scan through the error output below to
> see
> > if we should retry the test. Therefore, now we're first testing the stderr
to
> > determine if a retry is necessary.
> 
> I thought in that case we wouldn't have PASS in the output?

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:254: RunningProcess(TestCase this.testCase,
bool this.allowRetries);
On 2012/02/17 19:51:38, sigmund wrote:
> I think when using this.xxx our style is to omit the type.

Done.

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:709: // If we've failed more than four
tests, a lot of tests are likely
The progress counter keeps track of the *actual* failures (ones that failed
after all the retries, or if it wasn't allowed to retry) in the _failedTests
variable in test_progress.dart. Currently the variable is private, so I added an
gettor for it.

Yes, numFailedTests is not incremented. numFailedTests is here for the case
where the bot is super red (for example, when IE was failing 1/2 of its tests),
we don't bother allowing retries when more than 5 tests have *actually* failed.


On 2012/02/17 19:51:38, sigmund wrote:
> When is numFailedTest incremented?
> 
> I thought that if you fail, retry, and pass, then the numFailedTest is not
> incremented.
> 
> If that's the case, then you simply need to check _progress.numFailedTests !=
0
> (as soon as one test is marked as failing after all retries, the bot will be
> marked red, so we wouldn't need to retry any other test beyond that point).

[Emily Fortuna, 2012-02-17 21:04:45]

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9426005/diff/2001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:709: // If we've failed more than four
tests, a lot of tests are likely
*Edit to last paragraph: 
Yes, numFailedTests is not incremented if you fail, retry and then pass.


On 2012/02/17 21:03:03, Emily Fortuna wrote:
> The progress counter keeps track of the *actual* failures (ones that failed
> after all the retries, or if it wasn't allowed to retry) in the _failedTests
> variable in test_progress.dart. Currently the variable is private, so I added
an
> gettor for it.
> 
> Yes, numFailedTests is not incremented. numFailedTests is here for the case
> where the bot is super red (for example, when IE was failing 1/2 of its
tests),
> we don't bother allowing retries when more than 5 tests have *actually*
failed.
> 
> 
> On 2012/02/17 19:51:38, sigmund wrote:
> > When is numFailedTest incremented?
> > 
> > I thought that if you fail, retry, and pass, then the numFailedTest is not
> > incremented.
> > 
> > If that's the case, then you simply need to check _progress.numFailedTests
!=
> 0
> > (as soon as one test is marked as failing after all retries, the bot will be
> > marked red, so we wouldn't need to retry any other test beyond that point).
>

[Emily Fortuna, 2012-02-17 21:31:14]

ptal!

[Siggi Cherem (dart-lang), 2012-02-17 21:34:17]

lgtm

https://chromiumcodereview.appspot.com/9426005/diff/1004/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9426005/diff/1004/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:507: int MAX_FAILED_NO_RETRY = 4;
make it private (_MAX...)

https://chromiumcodereview.appspot.com/9426005/diff/1004/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:720: (MAX_FAILED_NO_RETRY -
_progress.numFailedTests) > 0).start();
nit =)

_progress.numFailedTests < _MAX_FAILED_NO_RETRY