Refactoring of ProgressIndicator

Refactoring of ProgressIndicator

Currently ProcessQueue creates and depends on exactly ONE ProgressIndicator.
This is not optimal for the following reasons:

 a) The used ProgressIndicator has several responsibilities (printing timing information,
    setting 'io.exitCode', printing test failures, ...)
    Splitting these responsibilities into different classes makes the code cleaner and more testable.

 b) If we make ProcessQueue depended on an abstract EventListener object (or a list of them) and make
    it send events to those listeners would make it easier to test ProcessQueue itself.

Issue: 8095

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

[kustermann, 2013-02-19 09:54:38]

Request for initial comments about the approach.

[ahe, 2013-02-19 11:43:42]

I like this approach.

https://codereview.chromium.org/12302016/diff/3002/tools/testing/dart/test_pr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12302016/diff/3002/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:71: // TODO: We should take all the
commands into account
Please use "TODO(username): Comment".

[kustermann, 2013-02-20 16:54:55]

Now you could review my CL.

I've broken the changes up, so you should use the 'delta from patchset
X-1'-feature.

[ricow1, 2013-03-12 10:31:47]

LGTM, very nice

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:12: import "test_suite.dart";
Martin: for future reference, stuff like this is easier to review if you
annotate what methods are just verbatim copy/move

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:137: _failingTest = _failingTest ||
test.lastCommandOutput.unexpectedOutput;
can't we just set the exit code here?

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:177: TimingPrinter(this._startTime);
can't we just set this to now?

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:184: // TODO: We should take all the
commands into account
Yes, this is probably very misleading for dart2js bots, can we file a bug for
this or alternatively just fix it (I assume that we could just add up the
timings for the commands?) - fix in another cl though

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:493: void _printStartProgress(TestCase
test) { }
we don't need this

[kustermann, 2013-03-12 13:59:16]

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:137: _failingTest = _failingTest ||
test.lastCommandOutput.unexpectedOutput;
On 2013/03/12 10:31:47, ricow1 wrote:
> can't we just set the exit code here?

Done.

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:177: TimingPrinter(this._startTime);
On 2013/03/12 10:31:47, ricow1 wrote:
> can't we just set this to now?

I don't really care, I just kept it as it was before. Let's keep it for now.

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:184: // TODO: We should take all the
commands into account
On 2013/03/12 10:31:47, ricow1 wrote:
> Yes, this is probably very misleading for dart2js bots, can we file a bug for
> this or alternatively just fix it (I assume that we could just add up the
> timings for the commands?) - fix in another cl though

We'll do it in another CL.

https://codereview.chromium.org/12302016/diff/35001/tools/testing/dart/test_p...
tools/testing/dart/test_progress.dart:493: void _printStartProgress(TestCase
test) { }
On 2013/03/12 10:31:47, ricow1 wrote:
> we don't need this

Done.

[kustermann, 2013-03-12 14:09:14]

Committed patchset #13 manually as r19867 (presubmit successful).