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

Björn Tillenius wrote:
> > > > 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.

I've looked into calling the run() method manually, but it means that we
would only be able to test layer.testSetUp(), test.setUp(), the test
method itself, test.tearDown() and layer.testTearDown(). I guess that's
not bad, but it misses out on layer.setUp() and layer.tearDown(), which
are quite important. The way it's structured at the moment means that
everything gets tested, and tested with the real test objects (i.e. the
runner and result) that zope.testing provides.

I have tried to replicate the zope.testing set up - by using Runner from
zope.testing.testresult.runner - but it's fiddly and adds a ton of noise
to the tests and the output. And I haven't yet figured out how to get it
to work properly.

> Having tests in setUp and tearDown isn't ideal.

It's not ideal, but failures here get reported as test failures, rather
than causing a complete test run failure. Unfortunately, failures in
layer methods do cause the run to fail.

To summarise, I'd like to leave the tests as they are. They demonstrate
the operation of ZopeTestInSubProcess right from layer set-up to
tear-down, which I think is valuable. I'll add some more comments to
make it clearer to future developers.

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

Thank you for asking so many good questions :) Have a good break.