Code review comment for lp:~allenap/launchpad/isolate-tests

On Mon, Mar 08, 2010 at 06:14:32PM -0000, Gavin Panella wrote:
> Gavin Panella has proposed merging lp:~allenap/launchpad/isolate-tests into lp:launchpad/devel.
>
> Requested reviews:
> Canonical Launchpad Engineering (launchpad): code
> Related bugs:
> #491870 Use Twisted's thread support instead of the threading module in checkwatches
> https://bugs.launchpad.net/bugs/491870
>
>
> Implement a new class - ZopeIsolatedTestCase - that can be used as a
> mixin with more interesting TestCase subclasses. It runs each test
> method in a forked sub-process, passing the results back to the parent
> process with subunit. This is needed to safely run tests that need to,
> for example, start the Twisted reactor, which cannot be safely restarted
> in the same process once stopped.
>
> This branch also uses ZopeIsolatedTestCase with a couple of job runner
> tests. Additionally, one of these tests, TestJobCronScript, has been
> fixed to override the command line arguments passed to the script class
> (otherwise sys.argv is used, which is stuffed with arguments passed to
> bin/test).

    vote needsinfo

If the job runner tests ran fine previously, why is this
ZopeIsolatedTestCase needed?

> === modified file 'lib/lp/services/job/tests/test_runner.py'
> --- lib/lp/services/job/tests/test_runner.py 2010-02-24 19:52:01 +0000
> +++ lib/lp/services/job/tests/test_runner.py 2010-03-08 18:14:32 +0000

> @@ -300,7 +301,7 @@
> self.entries.append(input)
>
>
> -class TestTwistedJobRunner(TestCaseWithFactory):
> +class TestTwistedJobRunner(ZopeIsolatedTestCase, TestCaseWithFactory):
>
> layer = LaunchpadZopelessLayer
>
> @@ -325,7 +326,7 @@
> self.assertIn('Job ran too long.', oops.value)
>
>
> -class TestJobCronScript(TestCaseWithFactory):
> +class TestJobCronScript(ZopeIsolatedTestCase, TestCaseWithFactory):

Will we ever use ZopeIsolatedTestCase without mixing in
TestCaseWithFactory?

> === modified file 'lib/lp/testing/__init__.py'
> --- lib/lp/testing/__init__.py 2010-02-17 20:54:36 +0000
> +++ lib/lp/testing/__init__.py 2010-03-08 18:14:32 +0000
> @@ -586,6 +591,56 @@
> self.client.open(url=u'http://launchpad.dev:8085')
>
>
> +class ZopeIsolatedTestCase(unittest.TestCase):
> + """Run tests in a sub-process, respecting Zope idiosyncrasies.
> +
> + Use this as a baseclass for test cases, or as a mixin with an
> + interesting `TestCase` subclass.
> +
> + This is basically a reimplementation of subunit's
> + `IsolatedTestCase`, but adjusted to work with Zope. In particular,
> + Zope's TestResult object is responsible for calling testSetUp()
> + and testTearDown() on the selected layer.
> + """

I can see why you named it ZopeIsolatedTestCase, but is that really a
good name? I mean, don't TestCase classes in general run their tests in
an isolated environment?

> +
> + def run(self, result):
> + assert isinstance(result, ZopeTestResult), (
> + "result must be a Zope result object, not %r." % (result,))
> + pread, pwrite = os.pipe()
> + pid = os.fork()
> + if pid == 0:
> + # Child.
> + os.close(pread)
> + fdwrite = os.fdopen(pwrite, 'w', 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(ZopeIsolatedTestCase, self).run(result)
> + fdwrite.flush()
> + sys.stdout.flush()
> + sys.stderr.flush()
> + # Exit hard.
> + os._exit(0)
> + else:
> + # Parent.
> + os.close(pwrite)
> + fdread = os.fdopen(pread, 'rU')
> + # Skip all the Zope stuff - the child process does that -
> + # by wrapping the result's __dict__ in a plain TestResult.
> + assert unittest.TestResult in result.__class__.mro(), (
> + "%r does not inherit from unittest.TestResult" % (result,))
> + base_result = unittest.TestResult()
> + base_result.__dict__ = result.__dict__

This looks like a nasty hack. What happens when you pass in result to
TestProtocolServer directly?

> + # Accept the result from the child process.
> + protocol = subunit.TestProtocolServer(base_result)
> + protocol.readFrom(fdread)
> + fdread.close()
> + os.waitpid(pid, 0)

I'm a bit interested in how this works with layers. Would you run away
screaming if I asked you to add some tests demonstrating this? :)

It'd be interesting to see in which process the setUp(), testSetUp(),
tearDown(), and testTearDown() methods of the layer are executed. I'm
wondering if we can see subtle errors due to doing the wrong things in
there.

Also, how this this TestClass related to TwistedLayer? Oh right, that
leads me to another question. You say that the reactor can't be
restarted easily. But can it be shared between tests, just like the
component architecture?

--
Björn Tillenius | https://launchpad.net/~bjornt