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

Revision history for this message
Gavin Panella (allenap) wrote :

> On Wed, 10 Mar 2010 14:15:31 -0000
> Björn Tillenius <email address hidden> wrote:
>
> ...
> > > If it's mixed-in with a TestSuite I think it gets more complicated.
>
> I haven't written tests for this yet. I may not do so (and restrict
> ZopeTestInSubProcess to just TestCase for now).

I've removed the suggestion that it can be mixed-in with a TestSuite,
with the following commit message:

  Don't advertise ZopeTestInSubProcess's use with a TestSuite. When
  one suite is added to another with addTests() the *tests* are
  copied, not the suite, so behaviour in a custom TestSuite is
  lost. This is too fragile and likely to cause confusion.

> ...
> > > > 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.
>
> I haven't thought about this yet, but I will.

Now that this works, and seems to work well in the situations I'm
using it (I am also using it in a dependent branch for checkwatches),
I'm reluctant to spend more time on figuring out how to do it in a
layer.

Both the job runner code and my checkwatches code start and stop the
reactor as part of their operation, so trying to fudge them to use an
already-started reactor skews the test and means more moving parts.

Forking is cheap as chips so, although I warn in the docstring that it
will slow down tests, I doubt it will make much of a difference,
especially when testing network code, database code, etc. I haven't
measured it though.

...
> > How well does this play with profiling? Do you still get all profiling
> > information if you specify --profile=cProfile?
>
> Not tried this yet. Not actually sure how to test this; it's a very
> long time since I did any profiling.

It looks like no profiling information is collected from the
sub-process. I'm not sure how to fix this. How important is this to
you? Can this branch be landed without that support, with a bug filed
to record that it needs to be added?

Gavin.

« Back to merge proposal