test.dart: Add timeout for browsers that are started but do not fetch tests.

test.dart: Add timeout for browsers that are started but do not fetch tests.

BUG=
R=kustermann@google.com

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

[kustermann, 2014-04-01 20:59:49]

The general direction is fine:
- start the timer when browser starts up / test result reported
- stop the timer when browser shuts down / next test was requested

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:693: }
I don't like this, it makes circular dependencies (BrowserTestRunner creates a
BrowserTestingStatus, BrowserTesting status will then ask BrowserTestRunner to
make a timeout handler ...).

Furthermore we should have only one of these. We start a browser, and record
it's current state in this object as long as the browser is alive.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:760: static const Duration
BROWSER_GET_TEST_TIMEOUT = const Duration(seconds: 60);
Move the static variables to the top, before the instance variables.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:904: last: status.currentTest);
The intent (I think) was to have *one* BrowserTestingStatus for *every* launched
browser, not for every single test.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:989: //      browserStatus[new_id] =
new BrowserTestingStatus(browser);
Commented code.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1004: browserStatus[browser.id] = new
BrowserTestingStatus(browser, this);
You need to set this before starting the browser, otherwise we can end up in a
situation where the browser started and requested the first test, but test.dart
doesn't have it in [browserStatus] yet.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1027: });
The terminate() above includes the browse.close() calls, and we don't want to
restart them if we hit the limit.

You should add a return statement after calling terminate()...; !

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1035:
status.getNextTestTimeout.cancel();
Since we cannot reuse the timer, we might as well set it to null after we've
cancelled it.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1103:
status.getNextTestTimeout.cancel();
See above.

[Bill Hesse, 2014-04-02 17:01:44]

This timeout is mostly handled the same as currentTest.timeoutTimer, and is
non-null only when timeoutTimer is null and vice versa.

If currentTest.timeoutTimer was moved up to BrowserTestingStatus.timeoutTimer,
the two timeouts could be unified, with the timeout handler doing different
things depending on whether currentTest was null or not.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:693: }
On 2014/04/01 20:59:49, kustermann wrote:
> I don't like this, it makes circular dependencies (BrowserTestRunner creates a
> BrowserTestingStatus, BrowserTesting status will then ask BrowserTestRunner to
> make a timeout handler ...).
> 
> Furthermore we should have only one of these. We start a browser, and record
> it's current state in this object as long as the browser is alive.

Done.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:904: last: status.currentTest);
On 2014/04/01 20:59:49, kustermann wrote:
> The intent (I think) was to have *one* BrowserTestingStatus for *every*
launched
> browser, not for every single test.

Done.  But I think it was better this way.

This one place where a new status wasn't created was still resetting it to be
almost the same as a new object.  And the timer is set both here and all the
places a new one is made.


And all the other ways to write this are uglier: var status = new ; map[id] =
status; status.x = y(status);

But I've changed it as you suggest.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:989: //      browserStatus[new_id] =
new BrowserTestingStatus(browser);
On 2014/04/01 20:59:49, kustermann wrote:
> Commented code.

moved outside if statement.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1004: browserStatus[browser.id] = new
BrowserTestingStatus(browser, this);
On 2014/04/01 20:59:49, kustermann wrote:
> You need to set this before starting the browser, otherwise we can end up in a
> situation where the browser started and requested the first test, but
test.dart
> doesn't have it in [browserStatus] yet.

Yes.  This means that the cases that didn't set it until here had a potential
bug.  Moved the earlier one outside the if, so all cases get a status assigned. 
Dropped this assignment.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1027: });
On 2014/04/01 20:59:49, kustermann wrote:
> The terminate() above includes the browse.close() calls, and we don't want to
> restart them if we hit the limit.
> 
> You should add a return statement after calling terminate()...; !

Placed fall-through code in an else { }.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1035:
status.getNextTestTimeout.cancel();
On 2014/04/01 20:59:49, kustermann wrote:
> Since we cannot reuse the timer, we might as well set it to null after we've
> cancelled it.

Done.

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1103:
status.getNextTestTimeout.cancel();
On 2014/04/01 20:59:49, kustermann wrote:
> See above.

Done.

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:992: if (status.nextTestTimeout !=
null) {
Why don't we also have to worry about status.currentTest.timeoutTimer here and
in terminate()?  I don't see anything cancelling them, or a guard in the
handler.

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1100:
futures.add(status.browser.close());
Why don't we have to cancel status.currentTest.timeoutTimer here?

[Bill Hesse, 2014-04-03 11:02:18]

Please take a look.

[kustermann, 2014-04-03 12:18:55]

looks okay to me, lgtm

But let rico take a look, he implemented most of this. It's also a bit
complicated and could need a bit of restructuring I think

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/20001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:904: last: status.currentTest);
On 2014/04/02 17:01:45, Bill Hesse wrote:
> On 2014/04/01 20:59:49, kustermann wrote:
> > The intent (I think) was to have *one* BrowserTestingStatus for *every*
> launched
> > browser, not for every single test.
> 
> Done.  But I think it was better this way.
> 
> This one place where a new status wasn't created was still resetting it to be
> almost the same as a new object.  And the timer is set both here and all the
> places a new one is made.
> 
> 
> And all the other ways to write this are uglier: var status = new ; map[id] =
> status; status.x = y(status);
> 
> But I've changed it as you suggest.


I'm not saying that you can't have methods on the BrowserTestingStatus.
Encapsulation would be very nice, in fact.

I'm just saying that you should have the same BrowserTestingStatus instance for
the same BrowserInstance!

You can also create "new Timer()"s in here (in a reset() method, or whatever),
but pass in a closure, instead of it calling back to BrowserController (which
makes a circle).

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:992: if (status.nextTestTimeout !=
null) {
On 2014/04/02 17:01:45, Bill Hesse wrote:
> Why don't we also have to worry about status.currentTest.timeoutTimer here and
> in terminate()?  I don't see anything cancelling them, or a guard in the
> handler.

I have no idea, rico implemented most of this. Maybe it's missing / it's a bug?

https://codereview.chromium.org/218683009/diff/60001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:1100:
futures.add(status.browser.close());
On 2014/04/02 17:01:45, Bill Hesse wrote:
> Why don't we have to cancel status.currentTest.timeoutTimer here?

ditto

https://codereview.chromium.org/218683009/diff/80001/tools/testing/dart/brows...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/218683009/diff/80001/tools/testing/dart/brows...
tools/testing/dart/browser_controller.dart:995: }
You can move the cancelling before browser.close(), right? No need to wait until
it's dead.

https://codereview.chromium.org/218683009/diff/80001/tools/testing/dart/scrub...
File tools/testing/dart/scrub_status_file.dart (right):

https://codereview.chromium.org/218683009/diff/80001/tools/testing/dart/scrub...
tools/testing/dart/scrub_status_file.dart:47:
CO19_STATUS_FILES.forEach(readStatusFile);
I don't think this file is used any more, we have a way more advanced
tools/status_clean.dart script !

[Bill Hesse, 2014-04-03 12:35:28]

Committed patchset #4 manually as r34701 (presubmit successful).