Adding logic to restart the browser if there is an exception

Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry first

BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/202a09250ad4141be8a80cd63881998d8e821f09
Cr-Commit-Position: refs/heads/master@{#407022}

[eyaich, 2016-07-14 18:39:54]

Description was changed from

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

BUG=628022
==========

to

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[eyaich, 2016-07-14 18:47:24]

Description was changed from

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry
first


BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[eyaich, 2016-07-14 18:48:22]

eyaich@google.com changed reviewers:
+ kbr@chromium.org, nednguyen@google.com

[nednguyen, 2016-07-14 19:04:12]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
We still should add "raise" after this so the exception bubbles up & cause a
test failure

[Ken Russell (switch to Gerrit), 2016-07-14 21:38:22]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:39:
self.tab.Navigate('chrome://crash')
Per comments in https://codereview.chromium.org/2148283003 , what if this were
rewritten to conditionally navigate to chrome://inducebrowsercrashforrealz , and
only then call super(...).setUp()? Would that more closely match the real
behavior seen?

[Ken Russell (switch to Gerrit), 2016-07-14 23:06:17]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/14 19:04:12, nednguyen wrote:
> We still should add "raise" after this so the exception bubbles up & cause a
> test failure

Is failing the test a good idea? setUp() is called before the test method, so
there won't be any opportunity to mark a test flaky to retry it. (_RunGpuTest is
what provides for flaky retries)

We should definitely at least print the exception to see what's going wrong.
There's no good reason for browser startup to fail.

Would it be a good idea to just get rid of this method entirely in
GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None) right
before the call to self.RunActualGpuTest ?

[nednguyen, 2016-07-14 23:10:01]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/14 23:06:17, Ken Russell wrote:
> On 2016/07/14 19:04:12, nednguyen wrote:
> > We still should add "raise" after this so the exception bubbles up & cause a
> > test failure
> 
> Is failing the test a good idea? setUp() is called before the test method, so
> there won't be any opportunity to mark a test flaky to retry it. (_RunGpuTest
is
> what provides for flaky retries)
> 
> We should definitely at least print the exception to see what's going wrong.
> There's no good reason for browser startup to fail.
> 
> Would it be a good idea to just get rid of this method entirely in
> GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None) right
> before the call to self.RunActualGpuTest ?
As you mention the problem of marking test flaky & retry, probably this is the
best way to go.

[nednguyen, 2016-07-14 23:16:47]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/14 23:10:01, nednguyen wrote:
> On 2016/07/14 23:06:17, Ken Russell wrote:
> > On 2016/07/14 19:04:12, nednguyen wrote:
> > > We still should add "raise" after this so the exception bubbles up & cause
a
> > > test failure
> > 
> > Is failing the test a good idea? setUp() is called before the test method,
so
> > there won't be any opportunity to mark a test flaky to retry it.
(_RunGpuTest
> is
> > what provides for flaky retries)
> > 
> > We should definitely at least print the exception to see what's going wrong.
> > There's no good reason for browser startup to fail.
> > 
> > Would it be a good idea to just get rid of this method entirely in
> > GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None) right
> > before the call to self.RunActualGpuTest ?
> As you mention the problem of marking test flaky & retry, probably this is the
> best way to go.

Though people can always subclass GpuIntegrationTest & override setUp method.
Maybe we should add a unittest that enforce  no overriding setUp & tearDown

[Ken Russell (switch to Gerrit), 2016-07-15 01:10:37]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/14 23:16:47, nednguyen wrote:
> On 2016/07/14 23:10:01, nednguyen wrote:
> > On 2016/07/14 23:06:17, Ken Russell wrote:
> > > On 2016/07/14 19:04:12, nednguyen wrote:
> > > > We still should add "raise" after this so the exception bubbles up &
cause
> a
> > > > test failure
> > > 
> > > Is failing the test a good idea? setUp() is called before the test method,
> so
> > > there won't be any opportunity to mark a test flaky to retry it.
> (_RunGpuTest
> > is
> > > what provides for flaky retries)
> > > 
> > > We should definitely at least print the exception to see what's going
wrong.
> > > There's no good reason for browser startup to fail.
> > > 
> > > Would it be a good idea to just get rid of this method entirely in
> > > GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None)
right
> > > before the call to self.RunActualGpuTest ?
> > As you mention the problem of marking test flaky & retry, probably this is
the
> > best way to go.
> 
> Though people can always subclass GpuIntegrationTest & override setUp method.
> Maybe we should add a unittest that enforce  no overriding setUp & tearDown 

OK. Let's start by adding:

  if not self.tab:
    self.tab = self.browser.tabs[0]

in GpuIntegrationTest._RunGpuTest, and removing setUp here. That way any
exception that's raised is handled by the normal mechanisms, and will be printed
out, and the browser restarted if it happens. A big question is why the renderer
is crashing, and that change will allow diagnosis.

Regarding such a unittest: that would be great, though I don't know how to write
it. Any advice you can offer would be great.

[nednguyen, 2016-07-15 01:39:55]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/15 01:10:37, Ken Russell wrote:
> On 2016/07/14 23:16:47, nednguyen wrote:
> > On 2016/07/14 23:10:01, nednguyen wrote:
> > > On 2016/07/14 23:06:17, Ken Russell wrote:
> > > > On 2016/07/14 19:04:12, nednguyen wrote:
> > > > > We still should add "raise" after this so the exception bubbles up &
> cause
> > a
> > > > > test failure
> > > > 
> > > > Is failing the test a good idea? setUp() is called before the test
method,
> > so
> > > > there won't be any opportunity to mark a test flaky to retry it.
> > (_RunGpuTest
> > > is
> > > > what provides for flaky retries)
> > > > 
> > > > We should definitely at least print the exception to see what's going
> wrong.
> > > > There's no good reason for browser startup to fail.
> > > > 
> > > > Would it be a good idea to just get rid of this method entirely in
> > > > GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None)
> right
> > > > before the call to self.RunActualGpuTest ?
> > > As you mention the problem of marking test flaky & retry, probably this is
> the
> > > best way to go.
> > 
> > Though people can always subclass GpuIntegrationTest & override setUp
method.
> > Maybe we should add a unittest that enforce  no overriding setUp & tearDown 
> 
> OK. Let's start by adding:
> 
>   if not self.tab:
>     self.tab = self.browser.tabs[0]
> 
> in GpuIntegrationTest._RunGpuTest, and removing setUp here. That way any
> exception that's raised is handled by the normal mechanisms, and will be
printed
> out, and the browser restarted if it happens. A big question is why the
renderer
> is crashing, and that change will allow diagnosis.
> 
> Regarding such a unittest: that would be great, though I don't know how to
write
> it. Any advice you can offer would be great.

We can use what's similar to
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

Basically use telemetry.core.discover to dynamically discover all the subclass
of SeriallyExecutedBrowserTestCase & assert
1) they are subclass of GpuIntegrationTestCase
2) they don't override setup & tearDown
..Other things

[eyaich, 2016-07-15 13:30:27]

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:134:
self._RestartBrowser('failure in setup')
On 2016/07/15 01:39:55, nednguyen wrote:
> On 2016/07/15 01:10:37, Ken Russell wrote:
> > On 2016/07/14 23:16:47, nednguyen wrote:
> > > On 2016/07/14 23:10:01, nednguyen wrote:
> > > > On 2016/07/14 23:06:17, Ken Russell wrote:
> > > > > On 2016/07/14 19:04:12, nednguyen wrote:
> > > > > > We still should add "raise" after this so the exception bubbles up &
> > cause
> > > a
> > > > > > test failure
> > > > > 
> > > > > Is failing the test a good idea? setUp() is called before the test
> method,
> > > so
> > > > > there won't be any opportunity to mark a test flaky to retry it.
> > > (_RunGpuTest
> > > > is
> > > > > what provides for flaky retries)
> > > > > 
> > > > > We should definitely at least print the exception to see what's going
> > wrong.
> > > > > There's no good reason for browser startup to fail.
> > > > > 
> > > > > Would it be a good idea to just get rid of this method entirely in
> > > > > GpuIntegrationTest, and fetch self.tab if necessary (i.e., it's None)
> > right
> > > > > before the call to self.RunActualGpuTest ?
> > > > As you mention the problem of marking test flaky & retry, probably this
is
> > the
> > > > best way to go.
> > > 
> > > Though people can always subclass GpuIntegrationTest & override setUp
> method.
> > > Maybe we should add a unittest that enforce  no overriding setUp &
tearDown 
> > 
> > OK. Let's start by adding:
> > 
> >   if not self.tab:
> >     self.tab = self.browser.tabs[0]
> > 
> > in GpuIntegrationTest._RunGpuTest, and removing setUp here. That way any
> > exception that's raised is handled by the normal mechanisms, and will be
> printed
> > out, and the browser restarted if it happens. A big question is why the
> renderer
> > is crashing, and that change will allow diagnosis.
> > 
> > Regarding such a unittest: that would be great, though I don't know how to
> write
> > it. Any advice you can offer would be great.
> 
> We can use what's similar to
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> Basically use telemetry.core.discover to dynamically discover all the subclass
> of SeriallyExecutedBrowserTestCase & assert
> 1) they are subclass of GpuIntegrationTestCase
> 2) they don't override setup & tearDown
> ..Other things

So I see one problem with this workflow.  

The original errors we were diagnosing were when test n crashed the browser or
the tab and then tests n+1 to num_tests were failing because we try and set the
tab to the 0th tab of the browser which is now in a crashed state.  Therefore,
test n is really where the error happened, and all of the subsequent tests
failed because of it.  

Therefore, we are catching the error in the setup method for the browser being
down so that the n+1 test has a chance to run and pass or fail on its own
instead of failing because a previous test failed.

If we move this logic into the _RunGpuTest method this information will be lost.
 Say for example test n+1 is expected to fail, it now fails on self.tab =
self.browser.tabs[0] in the try/catch in RunGpuTest, but actually succeeds
because the expectation was for it to fail, we just didn't realize it was the
wrong reason for failing.  

I think this logic should stay in the setup since it is actually the setup that
is failing, we can't access the browser because it is previously down, so lets
try and restart it and maybe log a warning.

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right):

https://codereview.chromium.org/2151983002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:39:
self.tab.Navigate('chrome://crash')
On 2016/07/14 21:38:22, Ken Russell wrote:
> Per comments in https://codereview.chromium.org/2148283003 , what if this were
> rewritten to conditionally navigate to chrome://inducebrowsercrashforrealz ,
and
> only then call super(...).setUp()? Would that more closely match the real
> behavior seen?

Done.

[Ken Russell (switch to Gerrit), 2016-07-15 18:05:24]

> So I see one problem with this workflow.  
> 
> The original errors we were diagnosing were when test n crashed the browser or
> the tab and then tests n+1 to num_tests were failing because we try and set
the
> tab to the 0th tab of the browser which is now in a crashed state.  Therefore,
> test n is really where the error happened, and all of the subsequent tests
> failed because of it.  
> 
> Therefore, we are catching the error in the setup method for the browser being
> down so that the n+1 test has a chance to run and pass or fail on its own
> instead of failing because a previous test failed.
> 
> If we move this logic into the _RunGpuTest method this information will be
lost.
>  Say for example test n+1 is expected to fail, it now fails on self.tab =
> self.browser.tabs[0] in the try/catch in RunGpuTest, but actually succeeds
> because the expectation was for it to fail, we just didn't realize it was the
> wrong reason for failing.  
> 
> I think this logic should stay in the setup since it is actually the setup
that
> is failing, we can't access the browser because it is previously down, so lets
> try and restart it and maybe log a warning.

Good points. Agreed, with warning and diagnostics added. LGTM

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:131: except Exception:
Please call exception_formatter.PrintFormattedException here so that we can
start to diagnose what's going wrong when this happens. Also please log
something about a failure happening during browser startup. Thanks.

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:38:
self.tab.Navigate('chrome://inducebrowsercrashforrealz')
Based on another thread, it sounded like I was wrong, and it's the renderer
rather than the browser which crashed. So could you change this back to
chrome://crash? Will that still provoke the failure? Thanks.

[nednguyen, 2016-07-15 18:11:15]

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:131: except Exception:
On 2016/07/15 18:05:24, Ken Russell wrote:
> Please call exception_formatter.PrintFormattedException here so that we can
> start to diagnose what's going wrong when this happens. Also please log
> something about a failure happening during browser startup. Thanks.

Recently I found out about pythons' log.exception(msg) that renders
exception_formatter module in telemetry unnecessary :-P

[Ken Russell (switch to Gerrit), 2016-07-16 01:09:54]

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:131: except Exception:
On 2016/07/15 18:11:15, nednguyen wrote:
> On 2016/07/15 18:05:24, Ken Russell wrote:
> > Please call exception_formatter.PrintFormattedException here so that we can
> > start to diagnose what's going wrong when this happens. Also please log
> > something about a failure happening during browser startup. Thanks.
> 
> Recently I found out about pythons' log.exception(msg) that renders
> exception_formatter module in telemetry unnecessary :-P

Aha. Very good, we should file a cleanup bug. :)

[eyaich, 2016-07-21 17:58:07]

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:131: except Exception:
On 2016/07/16 01:09:54, Ken Russell wrote:
> On 2016/07/15 18:11:15, nednguyen wrote:
> > On 2016/07/15 18:05:24, Ken Russell wrote:
> > > Please call exception_formatter.PrintFormattedException here so that we
can
> > > start to diagnose what's going wrong when this happens. Also please log
> > > something about a failure happening during browser startup. Thanks.
> > 
> > Recently I found out about pythons' log.exception(msg) that renders
> > exception_formatter module in telemetry unnecessary :-P
> 
> Aha. Very good, we should file a cleanup bug. :)

Done.

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right):

https://codereview.chromium.org/2151983002/diff/40001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:38:
self.tab.Navigate('chrome://inducebrowsercrashforrealz')
On 2016/07/15 18:05:24, Ken Russell wrote:
> Based on another thread, it sounded like I was wrong, and it's the renderer
> rather than the browser which crashed. So could you change this back to
> chrome://crash? Will that still provoke the failure? Thanks.

Done.

[nednguyen, 2016-07-21 18:01:56]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
I still think we should reraise after this, otherwise this will not surface the
failure.

[eyaich, 2016-07-21 18:35:36]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
On 2016/07/21 18:01:55, nednguyen wrote:
> I still think we should reraise after this, otherwise this will not surface
the
> failure.

The failure is on the test prior, so the exception that we care about has
already been raised for that test.  This is simply catching the state of the
browser that failed from the last tests and restarting it so this test has a
chance to pass.  Raising the error again here would fail this test and defeat
the whole purpose of this CL in the first place.

[nednguyen, 2016-07-21 18:40:30]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
On 2016/07/21 18:35:36, eyaich (ooo july18-july20) wrote:
> On 2016/07/21 18:01:55, nednguyen wrote:
> > I still think we should reraise after this, otherwise this will not surface
> the
> > failure.
> 
> The failure is on the test prior, so the exception that we care about has
> already been raised for that test.  This is simply catching the state of the
> browser that failed from the last tests and restarting it so this test has a
> chance to pass.  Raising the error again here would fail this test and defeat
> the whole purpose of this CL in the first place.

I think that we may still have case in which:

test N not fail
test N + 1 's setUp fail.

I would also expect most of the test failure from test N should also trigger a
browser restart in test N already:
https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati...

[eyaich, 2016-07-21 18:48:02]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
On 2016/07/21 18:40:30, nednguyen wrote:
> On 2016/07/21 18:35:36, eyaich (ooo july18-july20) wrote:
> > On 2016/07/21 18:01:55, nednguyen wrote:
> > > I still think we should reraise after this, otherwise this will not
surface
> > the
> > > failure.
> > 
> > The failure is on the test prior, so the exception that we care about has
> > already been raised for that test.  This is simply catching the state of the
> > browser that failed from the last tests and restarting it so this test has a
> > chance to pass.  Raising the error again here would fail this test and
defeat
> > the whole purpose of this CL in the first place.
> 
> I think that we may still have case in which:
> 
> test N not fail
> test N + 1 's setUp fail.
> 
> I would also expect most of the test failure from test N should also trigger a
> browser restart in test N already:
>
https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati...

How would test N+1's setUp fail unless tab[0] wasn't available, ie the browser
wasn't up from either 1) a previous failure or 2) the browser isn't able to be
started at all in which case all tests in that process would fail.  

If we raise an exception again here what is the point of this try catch block
other than to restart the browser again?  I guess it would restart it for the
N+2 test, but then N+1 might fail unnecessarily. 

Also, if if test N was expected to fail the browser wouldn't restart regardless
of the failure reason.  Side note: why don't we restart the browser after the
expected failure case, won't the browser be down after the exception?

[nednguyen, 2016-07-21 18:57:51]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
On 2016/07/21 18:48:02, eyaich wrote:
> On 2016/07/21 18:40:30, nednguyen wrote:
> > On 2016/07/21 18:35:36, eyaich (ooo july18-july20) wrote:
> > > On 2016/07/21 18:01:55, nednguyen wrote:
> > > > I still think we should reraise after this, otherwise this will not
> surface
> > > the
> > > > failure.
> > > 
> > > The failure is on the test prior, so the exception that we care about has
> > > already been raised for that test.  This is simply catching the state of
the
> > > browser that failed from the last tests and restarting it so this test has
a
> > > chance to pass.  Raising the error again here would fail this test and
> defeat
> > > the whole purpose of this CL in the first place.
> > 
> > I think that we may still have case in which:
> > 
> > test N not fail
> > test N + 1 's setUp fail.
> > 
> > I would also expect most of the test failure from test N should also trigger
a
> > browser restart in test N already:
> >
>
https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati...
> 
> How would test N+1's setUp fail unless tab[0] wasn't available, ie the browser
> wasn't up from either 1) a previous failure or 2) the browser isn't able to be
> started at all in which case all tests in that process would fail.  

Note that all the communication between telemetry & chrome happens
asynchronously, so it's possible that:

Test N : Browser is aive
Test N : Telemetry tells Browser to do X
Test N : Browser does X
Test N : Telemetry poke browser to get the state of the page, browser return
"foo"
Some where between test N & test N + 1: Browser crashes
Test N + 1.setUp:Telemetry contacts the browser again, but it's already crashed
& throw CrashException.

> 
> If we raise an exception again here what is the point of this try catch block
> other than to restart the browser again?  I guess it would restart it for the
> N+2 test, but then N+1 might fail unnecessarily. 

Yes, in case there is the exception, I would prefer N+1 fails here rather than
nothing fails.  When there is failure, adding an extra failure is bad, but not
as bad as comparing to no failure at all if there is a crash here that wasn't
shown in previous test run.

> 
> Also, if if test N was expected to fail the browser wouldn't restart
regardless
> of the failure reason.  Side note: why don't we restart the browser after the
> expected failure case, won't the browser be down after the exception?

[eyaich, 2016-07-21 19:04:10]

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_integration_test.py (right):

https://codereview.chromium.org/2151983002/diff/60001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_integration_test.py:135:
self._RestartBrowser('failure in setup')
On 2016/07/21 18:57:51, nednguyen wrote:
> On 2016/07/21 18:48:02, eyaich wrote:
> > On 2016/07/21 18:40:30, nednguyen wrote:
> > > On 2016/07/21 18:35:36, eyaich (ooo july18-july20) wrote:
> > > > On 2016/07/21 18:01:55, nednguyen wrote:
> > > > > I still think we should reraise after this, otherwise this will not
> > surface
> > > > the
> > > > > failure.
> > > > 
> > > > The failure is on the test prior, so the exception that we care about
has
> > > > already been raised for that test.  This is simply catching the state of
> the
> > > > browser that failed from the last tests and restarting it so this test
has
> a
> > > > chance to pass.  Raising the error again here would fail this test and
> > defeat
> > > > the whole purpose of this CL in the first place.
> > > 
> > > I think that we may still have case in which:
> > > 
> > > test N not fail
> > > test N + 1 's setUp fail.
> > > 
> > > I would also expect most of the test failure from test N should also
trigger
> a
> > > browser restart in test N already:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/gpu_integrati...
> > 
> > How would test N+1's setUp fail unless tab[0] wasn't available, ie the
browser
> > wasn't up from either 1) a previous failure or 2) the browser isn't able to
be
> > started at all in which case all tests in that process would fail.  
> 
> Note that all the communication between telemetry & chrome happens
> asynchronously, so it's possible that:
> 
> Test N : Browser is aive
> Test N : Telemetry tells Browser to do X
> Test N : Browser does X
> Test N : Telemetry poke browser to get the state of the page, browser return
> "foo"
> Some where between test N & test N + 1: Browser crashes
> Test N + 1.setUp:Telemetry contacts the browser again, but it's already
crashed
> & throw CrashException.
> 
> > 
> > If we raise an exception again here what is the point of this try catch
block
> > other than to restart the browser again?  I guess it would restart it for
the
> > N+2 test, but then N+1 might fail unnecessarily. 
> 
> Yes, in case there is the exception, I would prefer N+1 fails here rather than
> nothing fails.  When there is failure, adding an extra failure is bad, but not
> as bad as comparing to no failure at all if there is a crash here that wasn't
> shown in previous test run.
> 
> > 
> > Also, if if test N was expected to fail the browser wouldn't restart
> regardless
> > of the failure reason.  Side note: why don't we restart the browser after
the
> > expected failure case, won't the browser be down after the exception?

Ok I agree that not reporting an error at all is worse than reporting an extra
one.  Added the raise.

[nednguyen, 2016-07-21 19:22:15]

lgtm

[eyaich, 2016-07-21 19:42:25]

The CQ bit was checked by eyaich@google.com

[eyaich, 2016-07-21 19:42:26]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2151983002/#ps80001
(title: "Raising error after restarting the browser")

[commit-bot: I haz the power, 2016-07-21 19:43:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 19:43:06]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-21 19:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 19:59:07]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[eyaich, 2016-07-21 20:28:31]

The CQ bit was checked by eyaich@google.com

[commit-bot: I haz the power, 2016-07-21 20:28:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 20:28:50]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[eyaich, 2016-07-21 20:38:48]

The CQ bit was checked by eyaich@google.com

[eyaich, 2016-07-21 20:38:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2151983002/#ps100001
(title: "Syncing client")

[commit-bot: I haz the power, 2016-07-21 20:39:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 20:39:10]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-21 22:08:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 22:08:32]

Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Ken Russell (switch to Gerrit), 2016-07-21 22:47:49]

On 2016/07/21 22:08:32, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

Sorry, I don't know what's going on there. Will ask around. Re-CQ'ing.

[Ken Russell (switch to Gerrit), 2016-07-21 22:47:57]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-07-21 22:48:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 22:48:38]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[Ken Russell (switch to Gerrit), 2016-07-21 23:32:31]

On 2016/07/21 22:47:49, Ken Russell wrote:
> On 2016/07/21 22:08:32, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
> 
> Sorry, I don't know what's going on there. Will ask around. Re-CQ'ing.

Filed http://crbug.com/630452 about these failures and suppressing them in
https://codereview.chromium.org/2170153002 .

[commit-bot: I haz the power, 2016-07-22 01:33:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 01:33:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nednguyen, 2016-07-22 01:51:23]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2016-07-22 01:52:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 01:52:11]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
     tryserver.chromium.mac
     tryserver.chromium.win
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-22 03:26:47]

Description was changed from

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry
first


BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry
first


BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-22 03:26:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-22 03:29:37]

Description was changed from

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry
first


BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adding logic to restart the browser if there is an exception
in the setUp of GpuIntegrationTest

Note: I am not certain there is a good way to test the setup
functionality of GpuIntegrationTest in a unittest.  Given that
we are using a Fakes, I have added a hack to simulate throwing
an error in the setup method, but it is not how it would behave
in practice.  Any suggestions for a better way to unittest this
are appreciated.

Dependent on https://codereview.chromium.org/2148283003 landing in telemetry
first

BUG=628022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/202a09250ad4141be8a80cd63881998d8e821f09
Cr-Commit-Position: refs/heads/master@{#407022}
==========

[commit-bot: I haz the power, 2016-07-22 03:29:38]

Patchset 6 (id:??) landed as
https://crrev.com/202a09250ad4141be8a80cd63881998d8e821f09
Cr-Commit-Position: refs/heads/master@{#407022}