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?

Jono once told me - and an IRC discussion between Aaron, Jono and
myself yesterday reconfirmed this - that reactor restarting is not
supported, and is considered by Twisted developers to not be a working
feature right now. They want it to work, but it's not high on the
list. Jono suggested subunit.IsolatedTestCase as a possible solution.

Apart from that, I discovered than one of my new tests for
checkwatches ran fine on its own, or when run alongside many other
tests, but hung indefinitely when run in the same process as the job
runner tests.

> > === 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?

Yes, possibly. I've already made some changes to this branch since the
merge proposal went out, one of which was to not inherit from
anything. The reason is that it can be mixed in with TestSuite (and
its subclasses) too, without alteration, to provide process isolation
for a whole suite. I've updated the docs to explain the behaviour.

Full diff of changes since the proposal:
  http://paste.ubuntu.com/391864/

> > === 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?

ZopeIsolatedTestCase is now known as ZopeIsolatedTest. The original
name form came from subunit.IsolatedTestCase, which does a very
similar job. I'm happy to rename it though.

> > +
> > + 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?

Ah ha :) I've changed this to use super() now. Again, it's in the
pastebinned diff. Still somewhat hackish, but less so I think.

If I don't do this, then test layers are set up and torn down in both
the parent and the child process, post mortems will be attempted in
both parent and child, etc. It gets ugly. The most obvious problem is
that errors are reported to the console twice.

>
>
> > + # 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? :)

Not at all :)

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

I believe setUp() and tearDown() are run in the parent process, but
testSetUp() and testTearDown() are run the child. The latter pair are
called from the TestResult object.

If it's mixed-in with a TestSuite I think it gets more complicated.

So: testing.

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

That's an interesting idea. reactor.run() blocks, but I don't know of
any reason why it couldn't be started in a thread. I might get sent to
hell for it, but at least it's warmer than here.

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