Added support for logging the output of failed flaky tests.

Added support for logging the output of failed flaky tests.

Failed flaky tests will be logged to 'failed_flaky_tests.log'.
The default behavior is that the old log file gets deleted before
any tests are run. To prevent this, the '--append_flaky_log' option
can be set.

Committed: https://code.google.com/p/dart/source/detail?r=14735

[ricow1, 2012-11-07 19:56:51]

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:52: var report = new StringBuffer();
extract the body of this conditional into a separate function

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:53: report.add("Flaky test
${test.displayName} failed: \n");
How about refactoring _printFailureOutput to call another function that actually
produces the string to print out and then also call that function from here.
That way we ensure that we always get the same output from a failing test no
matter if it is flaky or not? (there are info here that is not printed that we
print in _printFailureOutput)
If you do that, then please ignore the comment above about moving out

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

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:171: bool isExpectedToBeFlaky() {
I would rename to flaky or isFlaky and I would consider making it a getter

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:177: flags.remove(TIMEOUT);
you can use cascades here if you like

[ricow1, 2012-11-08 08:52:42]

One additional comment:
we should make sure that the statistics from --report are still correct and sums
up!

[kustermann, 2012-11-08 09:15:13]

PTAL

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:52: var report = new StringBuffer();
On 2012/11/07 19:56:51, ricow1 wrote:
> extract the body of this conditional into a separate function

I refactored this instead

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:53: report.add("Flaky test
${test.displayName} failed: \n");
On 2012/11/07 19:56:51, ricow1 wrote:
> How about refactoring _printFailureOutput to call another function that
actually
> produces the string to print out and then also call that function from here.
> That way we ensure that we always get the same output from a failing test no
> matter if it is flaky or not? (there are info here that is not printed that we
> print in _printFailureOutput)
> If you do that, then please ignore the comment above about moving out 

Done.

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

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:171: bool isExpectedToBeFlaky() {
On 2012/11/07 19:56:51, ricow1 wrote:
> I would rename to flaky or isFlaky and I would consider making it a getter

Done.

https://codereview.chromium.org/11364129/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:177: flags.remove(TIMEOUT);
On 2012/11/07 19:56:51, ricow1 wrote:
> you can use cascades here if you like

Done.

[ricow1, 2012-11-08 09:49:13]

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:14: #import("testing/dart/test_progress.dart");
why do we need this?

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:54: var appendFlakyLog = firstConf['append_flaky_log'];
no need to actually extract this, just use it in the conditional (we don't reuse
this to pass on to other functions)

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:12: const FAILED_FLAKY_TESTS_LOGFILE =
".flaky.log";
Add comment stating explicitly what this is used for

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:53: for (var l in
_buildFailureOutput(test)) {
alternatively some thing like
var buf = _buildFailureOutput(test).reduce(new StringBuffer(), (p, e) =>
p.add("$e\n"));

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:139: 
remove whitespaces

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:142: 
remove whitespaces

[kustermann, 2012-11-08 10:24:41]

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:14: #import("testing/dart/test_progress.dart");
On 2012/11/08 09:49:13, ricow1 wrote:
> why do we need this?

Because the FAILED_FLAKY_TESTS_LOGFILE constant is defined in the test_progress
library. Should I put the constant somewhere else?

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:54: var appendFlakyLog = firstConf['append_flaky_log'];
On 2012/11/08 09:49:13, ricow1 wrote:
> no need to actually extract this, just use it in the conditional (we don't
reuse
> this to pass on to other functions)

Done.

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:12: const FAILED_FLAKY_TESTS_LOGFILE =
".flaky.log";
On 2012/11/08 09:49:13, ricow1 wrote:
> Add comment stating explicitly what this is used for

Done.

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:53: for (var l in
_buildFailureOutput(test)) {
On 2012/11/08 09:49:13, ricow1 wrote:
> alternatively some thing like
> var buf = _buildFailureOutput(test).reduce(new StringBuffer(), (p, e) =>
> p.add("$e\n"));
I think that this would not improve readability.

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:139: 
On 2012/11/08 09:49:13, ricow1 wrote:
> remove whitespaces

Done.

https://codereview.chromium.org/11364129/diff/8/tools/testing/dart/test_progr...
tools/testing/dart/test_progress.dart:142: 
On 2012/11/08 09:49:13, ricow1 wrote:
> remove whitespaces

Done.

[ricow1, 2012-11-08 10:32:50]

LGTM with small comment, please wait for Peter to also take a look

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:14: #import("testing/dart/test_progress.dart");
On 2012/11/08 10:24:41, kustermann wrote:
> On 2012/11/08 09:49:13, ricow1 wrote:
> > why do we need this?
> 
> Because the FAILED_FLAKY_TESTS_LOGFILE constant is defined in the
test_progress
> library. Should I put the constant somewhere else?

Alternatively put a small utility function inside the TestUtils class and use
that from both places (you already include test_suite - and this is where we get
most locations from in the remainder of the code

[kustermann, 2012-11-08 12:05:34]

PTAL

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/11364129/diff/8/tools/test-runtime.dart#newco...
tools/test-runtime.dart:14: #import("testing/dart/test_progress.dart");
On 2012/11/08 10:32:51, ricow1 wrote:
> On 2012/11/08 10:24:41, kustermann wrote:
> > On 2012/11/08 09:49:13, ricow1 wrote:
> > > why do we need this?
> > 
> > Because the FAILED_FLAKY_TESTS_LOGFILE constant is defined in the
> test_progress
> > library. Should I put the constant somewhere else?
> 
> Alternatively put a small utility function inside the TestUtils class and use
> that from both places (you already include test_suite - and this is where we
get
> most locations from in the remainder of the code

Done.

[ricow1, 2012-11-08 12:28:49]

Still LGTM (but please wait for Peters comments)

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1355: // If a flaky test did fail, infos
about it (i.e. test name, stdin, stdout)
s/infos/info

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1356: // will be written to this file. This
is useful for the debugging of
This is useful for debugging flaky tests.

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1358: // When running on a built bot, the
file can be made visible in the waterfall UI.
Long line

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1358: // When running on a built bot, the
file can be made visible in the waterfall UI.
s/built bot/buildbot

[ahe, 2012-11-09 10:35:01]

LGTM!

https://codereview.chromium.org/11364129/diff/11001/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/11364129/diff/11001/tools/test-runtime.dart#n...
tools/test-runtime.dart:55: var file = new File(TestUtils.flakyFileName());
Should flakyFileName be a getter?

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_r...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:172: if (expectedOutcomes.contains(SKIP)) {
Weird indentation.

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11364129/diff/11001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1359: return ".flaky.log";
I suggest that you remove the first dot.