Merge ~cjwatson/launchpad:refactor-zope-test-in-subprocess into launchpad:master

[Colin Watson]

Refactor ZopeTestInSubProcess as a RunTest variant

In terms of the testtools API, lp.testing.ZopeTestInSubProcess really makes more sense as a version of RunTest (used via `run_tests_with`), so turn it into lp.testing.RunIsolatedTest.

The layer's testSetUp and testTearDown methods are now called in the parent process rather than in the child. Layers are responsible for dealing with their own isolation, and none of Launchpad's layers need subprocess isolation; furthermore, running layer methods in child processes caused problems for some upcoming changes to DatabaseLayer.

[Ioana Lasc (ilasc)]

review: Approve

diff --git a/lib/lp/bugs/scripts/checkwatches/tests/test_core.py b/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
index ba87699..b66b03e 100644
--- a/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
+++ b/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 """Checkwatches unit tests."""
 
@@ -43,8 +43,9 @@ from lp.registry.interfaces.product import IProductSet
 from lp.services.config import config
 from lp.services.log.logger import BufferLogger
 from lp.testing import (
+    RunIsolatedTest,
+    TestCase,
     TestCaseWithFactory,
-    ZopeTestInSubProcess,
     )
 from lp.testing.dbuser import switch_dbuser
 from lp.testing.layers import LaunchpadZopelessLayer
@@ -388,8 +389,7 @@ class TestSerialScheduler(TestSchedulerBase, unittest.TestCase):
         self.scheduler.run()
 
 
-class TestTwistedThreadScheduler(
-    TestSchedulerBase, ZopeTestInSubProcess, unittest.TestCase):
+class TestTwistedThreadScheduler(TestSchedulerBase, TestCase):
     """Test TwistedThreadScheduler.
 
     By default, updateBugTrackers() runs jobs serially, but a
@@ -397,7 +397,10 @@ class TestTwistedThreadScheduler(
     for running several jobs in parallel, is TwistedThreadScheduler.
     """
 
+    run_tests_with = RunIsolatedTest
+
     def setUp(self):
+        super(TestTwistedThreadScheduler, self).setUp()
         self.scheduler = checkwatches.TwistedThreadScheduler(
             num_threads=5, install_signal_handlers=False)
 
@@ -451,8 +454,7 @@ class CheckwatchesMasterForThreads(CheckwatchesMaster):
         return [(ExternalBugTrackerForThreads(self.output_file), bug_watches)]
 
 
-class TestTwistedThreadSchedulerInPlace(
-    ZopeTestInSubProcess, TestCaseWithFactory):
+class TestTwistedThreadSchedulerInPlace(TestCaseWithFactory):
     """Test TwistedThreadScheduler in place.
 
     As in, driving as much of the bug watch machinery as is possible
@@ -460,6 +462,7 @@ class TestTwistedThreadSchedulerInPlace(
     """
 
     layer = LaunchpadZopelessLayer
+    run_tests_with = RunIsolatedTest
 
     def test(self):
         # Prepare test data.
diff --git a/lib/lp/services/job/tests/test_runner.py b/lib/lp/services/job/tests/test_runner.py
index 62a2376..3e2e19d 100644
--- a/lib/lp/services/job/tests/test_runner.py
+++ b/lib/lp/services/job/tests/test_runner.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for job-running facilities."""
@@ -52,8 +52,8 @@ from lp.services.timeout import (
     )
 from lp.services.webapp import errorlog
 from lp.testing import (
+    RunIsolatedTest,
     TestCaseWithFactory,
-    ZopeTestInSubProcess,
     )
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import LaunchpadZopelessLayer
@@ -616,10 +616,11 @@ class LeaseHeldJob(StaticJobSource):
         raise LeaseHeld()
 
 
-class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
+class TestTwistedJobRunner(TestCaseWithFactory):
 
     # Needs AMQP
     layer = LaunchpadZopelessLayer
+    run_tests_with = RunIsolatedTest
 
     def setUp(self):
         super(TestTwistedJobRunner, self).setUp()
diff --git a/lib/lp/testing/__init__.py b/lib/lp/testing/__init__.py
index 81e86f1..cc14cc8 100644
--- a/lib/lp/testing/__init__.py
+++ b/lib/lp/testing/__init__.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import, print_function
@@ -40,6 +40,7 @@ __all__ = [
     'run_script',
     'run_with_login',
     'run_with_storm_debug',
+    'RunIsolatedTest',
     'StormStatementRecorder',
     'test_tales',
     'TestCase',
@@ -53,7 +54,6 @@ __all__ = [
     'with_person_logged_in',
     'ws_object',
     'YUIUnitTestCase',
-    'ZopeTestInSubProcess',
     ]
 
 from contextlib import contextmanager
@@ -125,7 +125,6 @@ from zope.security.proxy import (
     isinstance as zope_isinstance,
     removeSecurityProxy,
     )
-from zope.testrunner.runner import TestResult as ZopeTestResult
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.security import IAuthorization
@@ -1197,30 +1196,45 @@ def _harvest_yui_test_files(file_path):
                 yield os.path.join(dirpath, filename)
 
 
-class ZopeTestInSubProcess:
+class _StartedTestResult(testtools.TestResultDecorator):
+    """A TestResult for a test that has already been started.
+
+    The startTest and stopTest methods do nothing.  This is used by
+    RunIsolatedTest, which calls them in the parent process instead.
+    """
+
+    def startTest(self, test):
+        pass
+
+    def stopTest(self, test):
+        pass
+
+
+class RunIsolatedTest(testtools.RunTest):
     """Run tests in a sub-process, respecting Zope idiosyncrasies.
 
-    Use this as a mixin with an interesting `TestCase` to isolate
-    tests with side-effects. Each and every test *method* in the test
-    case is run in a new, forked, sub-process. This will slow down
-    your tests, so use it sparingly. However, when you need to, for
-    example, start the Twisted reactor (which cannot currently be
-    safely stopped and restarted in process) it is invaluable.
-
-    This is basically a reimplementation of subunit's
-    `IsolatedTestCase` or `IsolatedTestSuite`, but adjusted to work
-    with Zope. In particular, Zope's TestResult object is responsible
-    for calling testSetUp() and testTearDown() on the selected layer.
+    Set this as `run_tests_with` on an interesting `TestCase` to isolate
+    tests with side-effects.  Each and every test *method* in the test case
+    is run in a new, forked, sub-process.  This will slow down your tests,
+    so use it sparingly.  However, when you need to, for example, start the
+    Twisted reactor (which cannot currently be safely stopped and restarted
+    in process) it is invaluable.
+
+    This is basically a reimplementation of subunit's `IsolatedTestCase` or
+    `IsolatedTestSuite`, but adjusted to work with Zope, ensuring that layer
+    methods (testSetUp and testTearDown) are called in the parent process.
     """
 
     def run(self, result):
-        # The result must be an instance of Zope's TestResult because
-        # we construct a super() of it later on. Other result classes
-        # could be supported with a more general approach, but it's
-        # unlikely that any one approach is going to work for every
-        # class. It's better to fail early and draw attention here.
-        assert isinstance(result, ZopeTestResult), (
-            "result must be a Zope result object, not %r." % (result, ))
+        result.startTest(self.case)
+        try:
+            return self._run_started(
+                _StartedTestResult(
+                    testtools.ExtendedToOriginalDecorator(result)))
+        finally:
+            result.stopTest(self.case)
+
+    def _run_started(self, result):
         pread, pwrite = os.pipe()
         # We flush __stdout__ and __stderr__ at this point in order to avoid
         # bug 986429; they get copied in full when we fork, which means that
@@ -1238,13 +1252,10 @@ class ZopeTestInSubProcess:
             # Child.
             os.close(pread)
             fdwrite = os.fdopen(pwrite, 'wb', 1)
-            # Send results to both the Zope result object (so that
-            # layer setup and teardown are done properly, etc.) and to
-            # the subunit stream client so that the parent process can
-            # obtain the result.
-            result = testtools.MultiTestResult(
-                result, subunit.TestProtocolClient(fdwrite))
-            super(ZopeTestInSubProcess, self).run(result)
+            # Send results to the subunit stream client so that the parent
+            # process can obtain the result.
+            super(RunIsolatedTest, self).run(
+                subunit.TestProtocolClient(fdwrite))
             fdwrite.flush()
             # See note above about flushing.
             sys.__stdout__.flush()
@@ -1257,17 +1268,10 @@ class ZopeTestInSubProcess:
             # Parent.
             os.close(pwrite)
             fdread = os.fdopen(pread, 'rb')
-            # Skip all the Zope-specific result stuff by using a
-            # super() of the result. This is because the Zope result
-            # object calls testSetUp() and testTearDown() on the
-            # layer, and handles post-mortem debugging. These things
-            # do not make sense in the parent process. More
-            # immediately, it also means that the results are not
-            # reported twice; they are reported on stdout by the child
-            # process, so they need to be suppressed here.
-            result = super(ZopeTestResult, result)
-            # Accept the result from the child process.
-            protocol = subunit.TestProtocolServer(result)
+            # Accept the result from the child process, but don't write a
+            # duplicate copy to stdout.
+            protocol = subunit.TestProtocolServer(
+                result, stream=subunit.DiscardStream())
             protocol.readFrom(fdread)
             fdread.close()
             os.waitpid(pid, 0)
diff --git a/lib/lp/testing/tests/test_zope_test_in_subprocess.py b/lib/lp/testing/tests/test_run_isolated_test.py
similarity index 71%
rename from lib/lp/testing/tests/test_zope_test_in_subprocess.py
rename to lib/lp/testing/tests/test_run_isolated_test.py
index 9f78f82..7071df8 100644
--- a/lib/lp/testing/tests/test_zope_test_in_subprocess.py
+++ b/lib/lp/testing/tests/test_run_isolated_test.py
@@ -1,11 +1,11 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test `lp.testing.ZopeTestInSubProcess`.
+"""Test `lp.testing.RunIsolatedTest`.
 
 How does it do this?
 
-A `TestCase`, mixed-in with `ZopeTestInSubProcess`, is run by the Zope
+A `TestCase`, using `run_tests_with = RunIsolatedTest`, is run by the Zope
 test runner. This test case sets its own layer, to keep track of the
 PIDs when certain methods are called. It also records pids for its own
 methods. Assertions are made as these methods are called to ensure that
@@ -18,9 +18,11 @@ __metaclass__ = type
 
 import functools
 import os
-import unittest
 
-from lp.testing import ZopeTestInSubProcess
+from lp.testing import (
+    RunIsolatedTest,
+    TestCase,
+    )
 
 
 def record_pid(method):
@@ -36,13 +38,12 @@ def record_pid(method):
     return wrapper
 
 
-class TestZopeTestInSubProcessLayer:
-    """Helper to test `ZopeTestInSubProcess`.
+class TestRunIsolatedTestLayer:
+    """Helper to test `RunIsolatedTest`.
 
     Asserts that layers are set up and torn down in the expected way,
-    namely that setUp() and tearDown() are called in the parent
-    process, and testSetUp() and testTearDown() are called in the
-    child process.
+    namely that setUp(), testSetUp(), testTearDown(), and tearDown() are
+    called in the parent process.
 
     The assertions for tearDown() and testTearDown() must be done here
     because the test case runs before these methods are called. In the
@@ -50,7 +51,7 @@ class TestZopeTestInSubProcessLayer:
     testSetUp() are done here too.
 
     This layer expects to be *instantiated*, which is not the norm for
-    Zope layers. See `TestZopeTestInSubProcess` for its use.
+    Zope layers. See `TestRunIsolatedTest` for its use.
     """
 
     @record_pid
@@ -68,15 +69,15 @@ class TestZopeTestInSubProcessLayer:
 
     @record_pid
     def testSetUp(self):
-        # Runs in the child process.
-        assert self.pid_in___init__ != self.pid_in_testSetUp, (
-            "layer.testSetUp() called in parent process.")
+        # Runs in the parent process.
+        assert self.pid_in___init__ == self.pid_in_testSetUp, (
+            "layer.testSetUp() not called in parent process.")
 
     @record_pid
     def testTearDown(self):
-        # Runs in the child process.
-        assert self.pid_in_testSetUp == self.pid_in_testTearDown, (
-            "layer.testTearDown() not called in same process as testSetUp().")
+        # Runs in the parent process.
+        assert self.pid_in___init__ == self.pid_in_testTearDown, (
+            "layer.testTearDown() not called in parent process.")
 
     @record_pid
     def tearDown(self):
@@ -85,31 +86,33 @@ class TestZopeTestInSubProcessLayer:
             "layer.tearDown() not called in parent process.")
 
 
-class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
-    """Test `ZopeTestInSubProcess`.
+class TestRunIsolatedTest(TestCase):
+    """Test `RunIsolatedTest`.
 
     Assert that setUp(), test() and tearDown() are called in the child
     process.
 
     Sets its own layer attribute. This layer is then responsible for
     recording the PID at interesting moments. Specifically,
-    layer.testSetUp() must be called in the same process as
-    test.setUp().
+    test.setUp(), test.test(), and test.tearDown() must all be called in
+    the same child process.
     """
 
+    run_tests_with = RunIsolatedTest
+
     @record_pid
     def __init__(self, method_name='runTest'):
         # Runs in the parent process.
-        super(TestZopeTestInSubProcess, self).__init__(method_name)
-        self.layer = TestZopeTestInSubProcessLayer()
+        super(TestRunIsolatedTest, self).__init__(method_name)
+        self.layer = TestRunIsolatedTestLayer()
 
     @record_pid
     def setUp(self):
         # Runs in the child process.
-        super(TestZopeTestInSubProcess, self).setUp()
-        self.assertEqual(
-            self.layer.pid_in_testSetUp, self.pid_in_setUp,
-            "setUp() not called in same process as layer.testSetUp().")
+        super(TestRunIsolatedTest, self).setUp()
+        self.assertNotEqual(
+            self.layer.pid_in___init__, self.pid_in_setUp,
+            "setUp() called in parent process.")
 
     @record_pid
     def test(self):
@@ -121,7 +124,7 @@ class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
     @record_pid
     def tearDown(self):
         # Runs in the child process.
-        super(TestZopeTestInSubProcess, self).tearDown()
+        super(TestRunIsolatedTest, self).tearDown()
         self.assertEqual(
             self.pid_in_setUp, self.pid_in_tearDown,
             "tearDown() not run in same process as setUp().")