Don't replicate timeout calculation for browser tests. They do their own timing

Don't replicate timeout calculation for browser tests. They do their own timing
from the time that the browser is actually opened.
Committed: https://code.google.com/p/dart/source/detail?r=13989

[Emily Fortuna, 2012-10-23 22:23:47]

This changes the logic added in https://codereview.chromium.org/11091070/

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_op...
File tools/testing/dart/test_options.dart (left):

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_op...
tools/testing/dart/test_options.dart:568: timeout *= 12; // Allow additional
time for browser testing to run.
This file is just a revert of a previous debugging test I added.

[Emily Fortuna, 2012-10-23 22:40:42]

-maar.cz. accidentally got added from autocomplete

[Jennifer Messerly, 2012-10-23 22:50:19]

lgtm

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

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:614: !(testCase.usesWebDriver &&
!testCase.configuration['noBatch'])) {
might want to add a property for:

    testCase.usesWebDriver && !testCase.configuration['noBatch']

perhaps "testCase.usesBatchRunner"?

[Emily Fortuna, 2012-10-23 23:11:25]

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

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:614: !(testCase.usesWebDriver &&
!testCase.configuration['noBatch'])) {
On 2012/10/23 22:50:19, John Messerly wrote:
> might want to add a property for:
> 
>     testCase.usesWebDriver && !testCase.configuration['noBatch']
> 
> perhaps "testCase.usesBatchRunner"?

But the dartc tests also "usesBatchRunner"...

[Jennifer Messerly, 2012-10-24 01:11:20]

On 2012/10/23 23:11:25, Emily Fortuna wrote:
>
https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
> tools/testing/dart/test_runner.dart:614: !(testCase.usesWebDriver &&
> !testCase.configuration['noBatch'])) {
> On 2012/10/23 22:50:19, John Messerly wrote:
> > might want to add a property for:
> > 
> >     testCase.usesWebDriver && !testCase.configuration['noBatch']
> > 
> > perhaps "testCase.usesBatchRunner"?
> 
> But the dartc tests also "usesBatchRunner"...

usesBatchWebDriver? :)

[Mads Ager (google), 2012-10-24 05:56:14]

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

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:614: !(testCase.usesWebDriver &&
!testCase.configuration['noBatch'])) {
I don't understand why this should change. If we are at the last step which is
running in the browser, the timer should have been cancelled and we should not
get the timedOut case here. If we get the timedOut case here we have not started
the browser part yet and one of the steps before starting the browser timed out
and the test itself times out.

Do the browser test cases have separate test output implementations that
interpret the timedout information and exit code differently? Does it not notice
that the test is timing out if the exit code is 0?

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:637: timeoutTimer.cancel();
Here we cancel the timer before starting the browser. So if it fires, it is one
of the steps before the browser that times out.

[ahe, 2012-10-24 06:02:39]

This does not look good to me, but I think Emily is on to something here:

If a test contains several steps, it might timeout while waiting for the next
step to start running. What we're not accounting for is the length of pauses
between the steps. 

This strengthens my concern that each step should have its own time out, not the
overall test.

[Mads Ager (google), 2012-10-24 06:55:02]

On 2012/10/24 06:02:39, ahe wrote:
> This does not look good to me, but I think Emily is on to something here:
> 
> If a test contains several steps, it might timeout while waiting for the next
> step to start running. What we're not accounting for is the length of pauses
> between the steps. 
> 
> This strengthens my concern that each step should have its own time out, not
the
> overall test.

I agree with that but it is a separate issue. The issue here is that browser
tests apparently do not deal correctly with one of the early steps timing out.
Let's fix that first. :)

[ahe, 2012-10-24 07:46:17]

On 2012/10/24 06:55:02, Mads Ager wrote:
> On 2012/10/24 06:02:39, ahe wrote:
> > This does not look good to me, but I think Emily is on to something here:
> > 
> > If a test contains several steps, it might timeout while waiting for the
next
> > step to start running. What we're not accounting for is the length of pauses
> > between the steps. 
> > 
> > This strengthens my concern that each step should have its own time out, not
> the
> > overall test.
> 
> I agree with that but it is a separate issue. The issue here is that browser
> tests apparently do not deal correctly with one of the early steps timing out.
> Let's fix that first. :)

I don't think that is matching the data I have seen. I have seen a test like
dart/tests/compiler/dart2js_extra/12_test.dart time out but
dart/tests/compiler/dart2js_extra/43_test.dart completing fine.

[Mads Ager (google), 2012-10-24 12:03:47]

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

https://codereview.chromium.org/11189150/diff/2001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:614: !(testCase.usesWebDriver &&
!testCase.configuration['noBatch'])) {
On 2012/10/24 05:56:14, Mads Ager wrote:
> I don't understand why this should change. If we are at the last step which is
> running in the browser, the timer should have been cancelled and we should not
> get the timedOut case here. If we get the timedOut case here we have not
started
> the browser part yet and one of the steps before starting the browser timed
out
> and the test itself times out.
> 
> Do the browser test cases have separate test output implementations that
> interpret the timedout information and exit code differently? Does it not
notice
> that the test is timing out if the exit code is 0?

Something is completely off here. I don't see how usesWebDriver can ever be
true? It is false by default in testCase. BrowserTestCase does not pass in a
usesWebDriver argument, so for all browser test cases it is false too. For other
test cases, we create the test cases passing in a function instead of a
boolean?!?

Emily, could you have a look at this. I think what has been added here makes no
difference at all because the usesWebDriver 'bool' is never true.

[Mads Ager (google), 2012-10-24 12:44:15]

I have attempted to fix the immediate issues that I see in this change list:

https://codereview.chromium.org/11267008/