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

On Tue, Mar 09, 2010 at 04:23:44PM -0000, Gavin Panella wrote:
> > > -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/

In the future, could you please attach the changes instead? Makes it
easier to review and comment.

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

I would probably have SubProcess, or Fork, in the name, to make it more
clear in what way it provides the isolation.

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

Ok, that certainly looks better.

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

Cool, I'm looking forward to seeing the tests :) These are quite
important properties that should be documented and tested.

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

If it could, than maybe it would be better to use a layer instead, since
that what we use elsewhere.

Some other questions popped into my mind. What happens if you specify
-D, and a test in the forked process fails?

How well does this play with profiling? Do you still get all profiling
information if you specify --profile=cProfile?

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