Merge ~cjwatson/launchpad:improve-web-browser-layer-handling into launchpad:master

[Colin Watson]

Improve handling of Firefox subprocesses in tests

Firefox is run via Selenium by some test cases. Since it's rather heavyweight, we run it in a layer so that it can be started up and shut down just once for a group of test cases.

I happened to notice recently that Firefox wasn't actually being shut down correctly, and continued to run long after any test cases that used it had finished. This turned out to be because the `YUITestLayer` teardown process was calling `selenium.webdriver.Firefox.close` (which closes the current window) rather than `selenium.webdriver.Firefox.quit` (which quits the driver and closes every associated window).

In the process, I also noticed that `lp.testing.tests.test_html5browser` started a separate browser for each individual test case, which seemed inefficient. I've split out a new `WebBrowserLayer` to fix this, which can be used to run tests of just the web browser part without bringing in the extra baggage of `FunctionalLayer` the way that `YUITestLayer` does.

I've verified that the process management now works correctly by running `while :; do date; ps xf; sleep 2; echo; done` in parallel with the test suite, and checking both that it uses a common browser instance and that it cleans it up when the relevant layer is torn down.

[Guruprasad (lgp171188)]

LGTM 👍

review: Approve

diff --git a/lib/lp/testing/__init__.py b/lib/lp/testing/__init__.py
index 9f9ccdc..d8cbff9 100644
--- a/lib/lp/testing/__init__.py
+++ b/lib/lp/testing/__init__.py
@@ -1108,8 +1108,11 @@ class AbstractYUITestCase(TestCase):
         The tests are run during `setUp()`, but failures need to be reported
         from here.
         """
-        assert self.layer.browser
-        results = self.layer.browser.run_tests(
+        # Circular import.
+        from lp.testing.layers import WebBrowserLayer
+
+        assert WebBrowserLayer.browser
+        results = WebBrowserLayer.browser.run_tests(
             self.html_uri,
             timeout=self.suite_timeout,
             incremental_timeout=self.incremental_timeout,
diff --git a/lib/lp/testing/html5browser.py b/lib/lp/testing/html5browser.py
index c559c64..678eeab 100644
--- a/lib/lp/testing/html5browser.py
+++ b/lib/lp/testing/html5browser.py
@@ -125,4 +125,4 @@ class Browser:
         return json.loads(results)
 
     def close(self):
-        self.driver.close()
+        self.driver.quit()
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index ce68de6..cf94f50 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -1938,8 +1938,8 @@ class ZopelessAppServerLayer(LaunchpadZopelessLayer):
         LayerProcessController.postTestInvariants()
 
 
-class YUITestLayer(FunctionalLayer):
-    """The layer for all YUITests cases."""
+class WebBrowserLayer(BaseLayer):
+    """A layer that runs a web browser."""
 
     browser = None
 
@@ -1966,6 +1966,32 @@ class YUITestLayer(FunctionalLayer):
         pass
 
 
+class YUITestLayer(FunctionalLayer, WebBrowserLayer):
+    """The layer for all YUI test cases."""
+
+    browser = None
+
+    @classmethod
+    @profiled
+    def setUp(cls):
+        pass
+
+    @classmethod
+    @profiled
+    def tearDown(cls):
+        pass
+
+    @classmethod
+    @profiled
+    def testSetUp(cls):
+        pass
+
+    @classmethod
+    @profiled
+    def testTearDown(cls):
+        pass
+
+
 class YUIAppServerLayer(MemcachedLayer):
     """The layer for all YUIAppServer test cases."""
 
diff --git a/lib/lp/testing/tests/test_html5browser.py b/lib/lp/testing/tests/test_html5browser.py
index bdc6229..d6ebc11 100644
--- a/lib/lp/testing/tests/test_html5browser.py
+++ b/lib/lp/testing/tests/test_html5browser.py
@@ -4,12 +4,14 @@
 from tempfile import NamedTemporaryFile
 
 from lp.testing import TestCase
-from lp.testing.html5browser import Browser
+from lp.testing.layers import WebBrowserLayer
 
 
 class TestBrowser(TestCase):
     """Verify Browser methods."""
 
+    layer = WebBrowserLayer
+
     def setUp(self):
         super().setUp()
         self.file = NamedTemporaryFile(
@@ -51,11 +53,9 @@ class TestBrowser(TestCase):
         self.file.flush()
         self.file_uri = "file://{}".format(self.file.name)
         self.addCleanup(self.file.close)
-        self.browser = Browser()
-        self.addCleanup(self.browser.close)
 
     def test_load_test_results(self):
-        results = self.browser.run_tests(self.file_uri, timeout=10000)
+        results = self.layer.browser.run_tests(self.file_uri, timeout=10000)
         self.assertEqual(results.status, results.Status.SUCCESS)
         self.assertEqual(
             results.results,
@@ -66,7 +66,7 @@ class TestBrowser(TestCase):
         )
 
     def test_timeout_error(self):
-        results = self.browser.run_tests(self.file_uri, timeout=1500)
+        results = self.layer.browser.run_tests(self.file_uri, timeout=1500)
         self.assertEqual(results.status, results.Status.TIMEOUT)
         self.assertIsNone(results.results)
         self.assertEqual(
@@ -75,7 +75,7 @@ class TestBrowser(TestCase):
         )
 
     def test_incremental_timeout_success(self):
-        results = self.browser.run_tests(
+        results = self.layer.browser.run_tests(
             self.file_uri, timeout=10000, incremental_timeout=3000
         )
         self.assertEqual(results.status, results.Status.SUCCESS)
@@ -88,7 +88,7 @@ class TestBrowser(TestCase):
         )
 
     def test_incremental_timeout_error(self):
-        results = self.browser.run_tests(
+        results = self.layer.browser.run_tests(
             self.file_uri, timeout=10000, incremental_timeout=1500
         )
         self.assertEqual(results.status, results.Status.TIMEOUT)