Support for running a limited amount of browser tests in parallel

Support for running a limited amount of browser tests in parallel

Patch Set 1:
  Refactored test_runner.dart to execute "Command after Command" instead of
  "TestCase after TestCase". By doing so, we don't couple the execution of
  compilation commands to the execution of the browser (or test) command.

Patch Set 2:
  Introduce a limitation on the number of parallel browser executions. By doing
  so we can run 'NumCores' compilation commands in parallel but only a single
  browser test command at any given time (necessary for IE).
  This way we can significantly reduce the cycle time on IE bots.

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

[kustermann, 2013-01-23 16:59:18]

I'd like to get some feedback on the general approach and the implementation.

[ricow1, 2013-01-24 09:04:28]

Initial comments

https://codereview.chromium.org/12035053/diff/2001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/12035053/diff/2001/tests/standalone/standalon...
tests/standalone/standalone.status:7: io/skipping_dart2js_compilations_test:
Fail
why?

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

https://codereview.chromium.org/12035053/diff/2001/tools/test-runtime.dart#ne...
tools/test-runtime.dart:108: maxProcesses,
this is a little hard to read - could we make a variable caled
maxBrowserProcesses just to make this easier

https://codereview.chromium.org/12035053/diff/2001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12035053/diff/2001/tools/test.dart#newcode158
tools/test.dart:158: var maxBrowserProcesses =
Add a comment as to why we do this

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:557: bool get unexpectedOutput {
can't we just override this in the BrowserCommandOutputImpl class instead, so
that we have the specialized functionality there?

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:564: return exitCode != 0;
was this wrong before?

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:959: command.outputIsUpToDate.then((bool
isUpToDate) {
I would just make this function return the future, and not have the completer as
an instance variable. You can have _commandComplete as a private method in here
if you like.
Actually I don't even think you need a completer here, you can just return a
future (or a transformation in the case where you need to run the process)

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1621: var isLastCommand =
((test.commands.length-1) == test.commandOutputs.length);
long line

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1624: // If there is no free browser runner,
put it back into the queue.
I don't think this is an optimal solution:
Before: we would put some tests in a "queue" when selenium was not ready, now we
are basically adding new timers as fast as we can possibly compile?
How about this: We create a real queue that a batch runner scheduler will poll.
If this is a browser command, simply put it into that queue (the added benefit
of this is that we can pull out some of the functionality below that is specific
for browser/dartc).
Additionally, this will eliminate all the bookkeeping of the number of browser
processes.

[kustermann, 2013-01-29 08:43:32]

PTAL.

@ahe: FYI. But comments are welcome ;-)

https://codereview.chromium.org/12035053/diff/2001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/12035053/diff/2001/tests/standalone/standalon...
tests/standalone/standalone.status:7: io/skipping_dart2js_compilations_test:
Fail
On 2013/01/24 09:04:28, ricow1 wrote:
> why?

Because this test uses RunningProcess, which I changed, so it's failing now. But
once we've decided that the current approach is correct, I'll make it pass
again.

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

https://codereview.chromium.org/12035053/diff/2001/tools/test-runtime.dart#ne...
tools/test-runtime.dart:108: maxProcesses,
On 2013/01/24 09:04:28, ricow1 wrote:
> this is a little hard to read - could we make a variable caled
> maxBrowserProcesses just to make this easier

Done.

https://codereview.chromium.org/12035053/diff/2001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12035053/diff/2001/tools/test.dart#newcode158
tools/test.dart:158: var maxBrowserProcesses =
On 2013/01/24 09:04:28, ricow1 wrote:
> Add a comment as to why we do this

Done.

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:557: bool get unexpectedOutput {
On 2013/01/24 09:04:28, ricow1 wrote:
> can't we just override this in the BrowserCommandOutputImpl class instead, so
> that we have the specialized functionality there?

I got rid of this code now.

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:564: return exitCode != 0;
On 2013/01/24 09:04:28, ricow1 wrote:
> was this wrong before?

I'm not 100% sure. My guess:
We only used 'testCase.lastCommandOutput.unexpectedOutput' on the browser test
command. (I think RunningProcess previously made sure that all commands except
the last one have 'exitCode == 0' or so.

In general this is kind of broken because we create a:
 - BrowserCommandOutput object for a CompilationCommand (the first command in
'testCase.commands')
 - BrowserCommandOutput object for a BrowserTestCommand (the second command in
'testCase.commands')

(That is exactly the reason why we have the branch above!)

Normally we would like to have something like that:
 - CompilationCommandOutput object for a CompilationCommand
 - BrowserCommandOutput object for a BrowserCommand

And both CompilationCommand and BrowserCommand should be passed what we expect
(maybe we expect a warning for CompilationCommand and a runtime-error for
BrowserCommand).
CompilationCommandOutput/BrowserCommandOutput.unexpectedOutput should then make
sure that the result is what we expect.

Or we could make an even bigger change where we make 
 - a TestCase for the compilation 
 - and a different (dependent) TestCase for running the test.

This way we could mark (in the status files) if we expect the compilation to
fail, or the compilation should pass and runtime should fail or whatever.

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:959: command.outputIsUpToDate.then((bool
isUpToDate) {
On 2013/01/24 09:04:28, ricow1 wrote:
> I would just make this function return the future, and not have the completer
as
> an instance variable. You can have _commandComplete as a private method in
here
> if you like.
> Actually I don't even think you need a completer here, you can just return a
> future (or a transformation in the case where you need to run the process)

We need a completer anyway because we get the 'process.onExit' trigger
asynchronously. So let's just keep it.

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1621: var isLastCommand =
((test.commands.length-1) == test.commandOutputs.length);
On 2013/01/24 09:04:28, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/12035053/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1624: // If there is no free browser runner,
put it back into the queue.
On 2013/01/24 09:04:28, ricow1 wrote:
> I don't think this is an optimal solution:
> Before: we would put some tests in a "queue" when selenium was not ready, now
we
> are basically adding new timers as fast as we can possibly compile?
> How about this: We create a real queue that a batch runner scheduler will
poll.
> If this is a browser command, simply put it into that queue (the added benefit
> of this is that we can pull out some of the functionality below that is
specific
> for browser/dartc).
> Additionally, this will eliminate all the bookkeeping of the number of browser
> processes.

As discussed offline, we'll keep it that way.

[ahe, 2013-01-29 09:18:42]

DBC

It might be good to get Mads to look at the process changes.

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart#newcode161
tools/test.dart:161: firstConf['runtime'].contains('ie') ? 1 : maxProcesses;
This is probably good enough for now, but I think it means that you'll throttle
all browsers. So if I run test.py like this:

test.py -mrelease -cdart2js -rff,chrome,ie

Both Firefox and Chrome will be throttled.

[Mads Ager (google), 2013-01-29 10:47:48]

DBC

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart#newcode161
tools/test.dart:161: firstConf['runtime'].contains('ie') ? 1 : maxProcesses;
On 2013/01/29 09:18:42, ahe wrote:
> This is probably good enough for now, but I think it means that you'll
throttle
> all browsers. So if I run test.py like this:
> 
> test.py -mrelease -cdart2js -rff,chrome,ie
> 
> Both Firefox and Chrome will be throttled.

Or none of them will be throttled because we only look at the first
configuration which could be runtime ff in this case. We should avoid passing it
in as an argument to the process queue constructor since this is a configuration
specific option. Can we have a helper method that looks at the configuration for
a given test instead of having a global setting?

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

https://codereview.chromium.org/12035053/diff/16002/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1315: int _numBrowserProcesses = 0;
Maybe split this into numIEProcesses and a utility to determine if a TestCase is
for IE? Each test case knows its configuration and therefore the runtime it is
intended for.

https://codereview.chromium.org/12035053/diff/16002/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1618: new Timer(100, (timer)
{_tryRunTest();});  // Don't lose a process.
Is there a reason for waiting 100ms for this? Would Timer(0, ...) be enough. If
there is space for one now, we should just get started instead of waiting for
100ms.

Also, add space after ';' in the closure.

[kustermann, 2013-01-29 12:10:48]

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart#newcode161
tools/test.dart:161: firstConf['runtime'].contains('ie') ? 1 : maxProcesses;
On 2013/01/29 09:18:42, ahe wrote:
> This is probably good enough for now, but I think it means that you'll
throttle
> all browsers. So if I run test.py like this:
> 
> test.py -mrelease -cdart2js -rff,chrome,ie
> 
> Both Firefox and Chrome will be throttled.

Thanks for catching this peter. Since I never use the "cross product"-feature, I
haven't thought about this case.

https://codereview.chromium.org/12035053/diff/16002/tools/test.dart#newcode161
tools/test.dart:161: firstConf['runtime'].contains('ie') ? 1 : maxProcesses;
On 2013/01/29 10:47:48, Mads Ager wrote:
> On 2013/01/29 09:18:42, ahe wrote:
> > This is probably good enough for now, but I think it means that you'll
> throttle
> > all browsers. So if I run test.py like this:
> > 
> > test.py -mrelease -cdart2js -rff,chrome,ie
> > 
> > Both Firefox and Chrome will be throttled.
> 
> Or none of them will be throttled because we only look at the first
> configuration which could be runtime ff in this case. We should avoid passing
it
> in as an argument to the process queue constructor since this is a
configuration
> specific option. Can we have a helper method that looks at the configuration
for
> a given test instead of having a global setting?

IMHO, we should pass it to the process queue because it is not a configuration
specific setting -- the number of parallel IE processes is fixed for an instance
of ProcessQueue. (As mentioned in my other comment, we could make this a map
(maxBrowserProcesses = {'ie9' : 1, ...}) -- but the code would become a bit more
complicated).

I discussed this offline a bit with rico, and the simplest way is to loop
through all configurations and check if we have an IE or not. If we have one we
pass "maxBrowserProcesses=1".

In general we don't slow test.py down a lot because:
- we do use --runtime=a,b,c very seldom on test.py invocations (personally, I've
never used it)
- even if we do, it will only throttle the browser runs if we run test.py on
windows (if IE was included)
- the majority of the time is spent in dart2js compilations

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

https://codereview.chromium.org/12035053/diff/16002/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1315: int _numBrowserProcesses = 0;
On 2013/01/29 10:47:48, Mads Ager wrote:
> Maybe split this into numIEProcesses and a utility to determine if a TestCase
is
> for IE? Each test case knows its configuration and therefore the runtime it is
> intended for.

I could make "_maxBrowserProcesses" a map from browser to the maximum number of
parallel processes for that browser. 
(i.e. _maxBrowserProcesses = {'ie9': 1, 'ie10: 1})

But this makes the code just more complicated (without a real benefit).

https://codereview.chromium.org/12035053/diff/16002/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1618: new Timer(100, (timer)
{_tryRunTest();});  // Don't lose a process.
On 2013/01/29 10:47:48, Mads Ager wrote:
> Is there a reason for waiting 100ms for this? Would Timer(0, ...) be enough.
If
> there is space for one now, we should just get started instead of waiting for
> 100ms.
> 
> Also, add space after ';' in the closure.

Yes there is a reason for that: Let's assume that all dart2js compilations are
done and only browser test commands are left. If there is only one IE Browser
runner available and it is busy (let's say it takes a few minutes), then the
remaining (maxProcesses - 1) ones will
 - dequeue a test from the queue
 - find out that it is a browser test
 - find out that there is no free browser runner
 - enqueue it again to the queue
If we set the timer to 0ms, the remaining ones (maxProcesses - 1) will repeat
the procedure over and over again (aka busy waiting).

As soon as the IE Test runner is finished with a test, it will call
_tryRunTest() again, so there should be no delay here.

[ricow1, 2013-01-29 13:01:17]

LGTM, lets see what Mads and Peter says to the maxBrowser parameter

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/io/test_...
File tests/standalone/io/test_runner_test.dart (right):

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/io/test_...
tests/standalone/io/test_runner_test.dart:15: final LONG_TIMEOUT = 120;
isn't this even longer than the timeout we have in the normal runner?, i.e.,
will we not get killed before hitting this?

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/standalo...
File tests/standalone/standalone.status (left):

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/standalo...
tests/standalone/standalone.status:9: io/test_runner_test: Pass, Fail # Issue
1947
as asked in the test file, don't we need to mark this as slow in case it uses
more than a minute?

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

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:960: void timeoutHandler(Timer unusedTimer)
{
Timer unusedTimer -> _

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:966: // Hopefully, this means that the
process died on its own.
First of all, I don't think kill will ever throw an exception, secondly if it
did I think this is something we want to log

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1619: 
remove blank line

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1635: void wrapper(TestCase test_arg) {
call this something other than wrapper

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1735: print("Potential flake. Re-running
${testCase.displayName} "
should we write this out to our flaky log?

[Mads Ager (google), 2013-01-29 13:47:39]

I am fine with the max browser parameter, I think you are right that it will not
really have an effect in practice. :)

[kustermann, 2013-01-29 14:58:26]

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/io/test_...
File tests/standalone/io/test_runner_test.dart (right):

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/io/test_...
tests/standalone/io/test_runner_test.dart:15: final LONG_TIMEOUT = 120;
On 2013/01/29 13:01:17, ricow1 wrote:
> isn't this even longer than the timeout we have in the normal runner?, i.e.,
> will we not get killed before hitting this?

Since it shouldn't time out, let's set this to 30 seconds.

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/standalo...
File tests/standalone/standalone.status (left):

https://codereview.chromium.org/12035053/diff/13009/tests/standalone/standalo...
tests/standalone/standalone.status:9: io/test_runner_test: Pass, Fail # Issue
1947
On 2013/01/29 13:01:17, ricow1 wrote:
> as asked in the test file, don't we need to mark this as slow in case it uses
> more than a minute?
As mentioned on the other comment, it shouldn't be slow, therefore we don't mark
it as slow.

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

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:960: void timeoutHandler(Timer unusedTimer)
{
On 2013/01/29 13:01:17, ricow1 wrote:
> Timer unusedTimer -> _

This was old code.

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:966: // Hopefully, this means that the
process died on its own.
On 2013/01/29 13:01:17, ricow1 wrote:
> First of all, I don't think kill will ever throw an exception, secondly if it
> did I think this is something we want to log

This was old code. You're right, ProcessException is only thrown in
Process.start() but not in Process.kill().

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1619: 
On 2013/01/29 13:01:17, ricow1 wrote:
> remove blank line

Done.

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1635: void wrapper(TestCase test_arg) {
On 2013/01/29 13:01:17, ricow1 wrote:
> call this something other than wrapper

This was old code.

https://codereview.chromium.org/12035053/diff/13009/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1735: print("Potential flake. Re-running
${testCase.displayName} "
On 2013/01/29 13:01:17, ricow1 wrote:
> should we write this out to our flaky log?
Let's write it to the debug.log since flaky.log should only contain failing
tests which are marked as flaky in the status files.