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

On Fri, Mar 12, 2010 at 11:27:26AM -0000, Gavin Panella 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.

Ok.

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

Ok.

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

As long as it's not used too much, it's not crucial to have it.

However, I don't see any tests for this. Oh, now I found them, they just
don't look like tests. Hmm. Some more comments would help, to understand
why, and what's being tested. I would have expected a test that manually
calls the run() method. Would that be hard? It would mean that you would
get much better failure messages, in case of something went wrong.
Having tests in setUp and tearDown isn't ideal.

Sorry, but you'll have to ask someone else to carry on reviewing this
branch, since I'm unavailable next week.

    vote abstain

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