Force close of browser Windows when complete.

Force close of browser Windows when complete.
Committed: https://code.google.com/p/dart/source/detail?r=4241

[Emily Fortuna, 2012-02-14 00:19:55]

Please do look carefully at how I'm doing this. I'm not thrilled with how I
implement it currently, but it seems to be the "less bad" of the implementations
I came up with (see Patch 1 for a more general solution that seemed to
ultimately not have much added value and the extra code). In both cases the
TestSuite needs a reference to the ProcessQueue, since the ProcessQueue is the
one that knows when all the tests are done, but the TestSuite only learns if it
has a BrowserTest over the course of en-queuing test. I'm open to other
implementation ideas if you have them.

[Jennifer Messerly, 2012-02-14 02:17:25]

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

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:544:
testSuite.notifyIfBrowserCleanupNeeded(this);
an alternate idea: instead of changing test_suite, could you change _runTest in
this class?

From there you can access test.configuration, which should give you:

if (test is BrowserTestCase) { // or test.configuration['component'] ==
'webdriver'
  browserUsed = test.configuration['browser'];
}

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:584: _progress.allDone();
is it okay that we're potentially calling allDone here multiple times? in other
words, if chromedriver and chrome are being killed, each one will call
_progress.allDone()

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:616: }
it'd be good to pull this pattern out into a function

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:572: if ((_allDoneCallbackListener != null))
{
extra parens

[Bill Hesse, 2012-02-14 12:46:00]

I like the approach in the first patch set better, and I have an idea for a
cleaner design:

ProcessQueue has
Map<CleanupTestCase> cleanupHandlers
(this is a Map so that duplicate handlers can have the same key, and so avoid
being registered multiple times)

void registerCleanup(String key, CleanupTestCase cleanupProcess)
(This callback will be passed to test suites in forAllTests)

In the termination code:
for (cleanup in cleanupHandlers.values) {
   increment activeCleanupCount
   start CleanupTestCase with handlers:

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

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:584: _progress.allDone();
On 2012/02/14 02:17:25, John Messerly wrote:
> is it okay that we're potentially calling allDone here multiple times? in
other
> words, if chromedriver and chrome are being killed, each one will call
> _progress.allDone()

I don't think this is OK.

[Bill Hesse, 2012-02-14 12:49:53]

I wasn't done:
with handlers:
   errorHandler:  decrement activeCleanupCount, if zero then allDone()
   exitHandler:  decrement activeCleanupCount, if zero then allDone()
   timeoutHandler:  decrement activeCleanupCount, if zero then allDone()

CleanupTestCase has an executable, arguments, and a timeout - it can easily be a
subclass of TestCase.

registerCleanup(key, cleanup) {
  cleanupHandlers[key] = cleanup;
}


On 2012/02/14 12:46:00, Bill Hesse wrote:
> I like the approach in the first patch set better, and I have an idea for a
> cleaner design:
> 
> ProcessQueue has
> Map<CleanupTestCase> cleanupHandlers
> (this is a Map so that duplicate handlers can have the same key, and so avoid
> being registered multiple times)
> 
> void registerCleanup(String key, CleanupTestCase cleanupProcess)
> (This callback will be passed to test suites in forAllTests)
> 
> In the termination code:
> for (cleanup in cleanupHandlers.values) {
>    increment activeCleanupCount
>    start CleanupTestCase with handlers:
> 
>
https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
> tools/testing/dart/test_runner.dart:584: _progress.allDone();
> On 2012/02/14 02:17:25, John Messerly wrote:
> > is it okay that we're potentially calling allDone here multiple times? in
> other
> > words, if chromedriver and chrome are being killed, each one will call
> > _progress.allDone()
> 
> I don't think this is OK.

[Emily Fortuna, 2012-02-14 23:55:44]

Hello all,
Please take another look. 

Bill, I initially wrote this in the more general way, but then I couldn't think
of a situation where we'd need to do so many things after all the tests had
completed that we'd need a list (or map, or set) to store them all in. Do you
have a good use case for writing it the other way? I'm certainly open to
changing it, and will certainly go the direction of my original patch set if we
can come up with a compelling reason. 
Additionally, there's the added complication of making the function closures
hashable if we store them in a set or map (I originally had written the code
this way, too). 

Bill is my understanding of your suggestion for the cleanup function(s) to make
it like a special type of TestCase that is run last? Thanks for the
clarification.

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

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:544:
testSuite.notifyIfBrowserCleanupNeeded(this);
On 2012/02/14 02:17:25, John Messerly wrote:
> an alternate idea: instead of changing test_suite, could you change _runTest
in
> this class?
> 
> From there you can access test.configuration, which should give you:
> 
> if (test is BrowserTestCase) { // or test.configuration['component'] ==
> 'webdriver'
>   browserUsed = test.configuration['browser'];
> }

Done.

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:616: }
On 2012/02/14 02:17:25, John Messerly wrote:
> it'd be good to pull this pattern out into a function

Done.

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9369051/diff/9001/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:572: if ((_allDoneCallbackListener != null))
{
On 2012/02/14 02:17:25, John Messerly wrote:
> extra parens

Done.

[Jennifer Messerly, 2012-02-15 00:51:59]

lgtm.

Let's get this checked in to make the build bots hopefully more stable. But feel
free to refactor & generalize after.