Adding logic to restart the browser if there is an exception

content/test/gpu/gpu_tests/gpu_integration_test.py

Index: content/test/gpu/gpu_tests/gpu_integration_test.py
diff --git a/content/test/gpu/gpu_tests/gpu_integration_test.py b/content/test/gpu/gpu_tests/gpu_integration_test.py
index 21a283be021fd1b200f1974ed6ae2319de4bd986..2f9128aacc9484a8835a4a55b6c61933e90bdb65 100644
--- a/content/test/gpu/gpu_tests/gpu_integration_test.py
+++ b/content/test/gpu/gpu_tests/gpu_integration_test.py
@@ -126,4 +126,9 @@ class GpuIntegrationTest(
     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.
+      self._RestartBrowser('failure in setup')

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

We still should add "raise" after this so the exception bubbles up & cause a
test failure

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

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]

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]

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]

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]

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]

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.