Adding logic to restart the browser if there is an exception

content/test/gpu/gpu_tests/gpu_integration_test.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import logging
 
 from telemetry.testing import serially_executed_browser_test_case
 
 from gpu_tests import exception_formatter
 from gpu_tests import gpu_test_expectations
@@ skipping 108 matching lines @@
 
   @classmethod
   def _CreateExpectations(cls):
     # Subclasses **must** override this in order to provide their test
     # expectations to the harness.
     #
     # Do not call this directly. Call GetExpectations where necessary.
     raise NotImplementedError
 
   def setUp(self):
-    self.tab = self.browser.tabs[0]
+    try:
+      self.tab = self.browser.tabs[0]
+    except Exception:
+      # restart the browser to make sure a failure in a test doesn't
+      # propagate to the next test iteration.
+      logging.exception("Failure during browser startup")
+      self._RestartBrowser('failure in setup')

[nednguyen, 2016/07/21 18:01:55]

I still think we should reraise after this, otherwise this will not surface the
failure.

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

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]

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]

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]

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.
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.

[eyaich, 2016/07/21 19:04:09]

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