← Back to team overview

testtools-dev team mailing list archive

Re: [Merge] lp:~jml/testtools/deferred-support into lp:testtools

 

Its awesome that you've done this.

I have one small suggestion; perhaps the twisted specific bits of this
should be in twisted.trial? There aren't, AFAIK, any third party
implementations of deferreds, yet.

Some code thoughts inline below:

On Mon, Oct 11, 2010 at 5:49 AM, Jonathan Lange <jml@xxxxxxxxx> wrote:
> === modified file 'NEWS'
> --- NEWS        2010-09-18 02:10:58 +0000
> +++ NEWS        2010-10-10 16:49:39 +0000
> +class SynchronousDeferredRunTest(RunTest):
> +    """Runner for tests that return synchronous Deferreds."""
> +
> +    def _run_user(self, function, *args):
> +        d = defer.maybeDeferred(function, *args)
> +        def got_exception(failure):
> +            return self._got_user_exception(
> +                (failure.type, failure.value, failure.tb))
> +        d.addErrback(got_exception)
> +        result = extract_result(d)
> +        return result

There's no reason for got_exception to be an inner function here.

There was a bare except: I trimmed out, doh. Anyhow, except Exception:
would be better.

> +        # XXX: This can call addError on result multiple times. Not sure if
> +        # this is a good idea.

^ - definitely a bad idea, we avoid this in the normal sync code,
instead we trigger multiple exceptions which get accumulated via
details.

> +    def _run_user(self, function, *args):
> +        # XXX: I think this traps KeyboardInterrupt, and I think this is a bad
> +        # thing. Perhaps we should have a maybeDeferred-like thing that
> +        # re-raises KeyboardInterrupt. Or, we should have our own exception
> +        # handler that stops the test run in the case of KeyboardInterrupt. But
> +        # of course, the reactor installs a SIGINT handler anyway.

Squashing KeyboardInterrupt and SystemExit would be bad :). And if
we're catching one, we're probably catching the other.

It would be nice to have less of a parallel implementation feel, but
c'est la vie. I haven't read your tests in detail, but the conceptual
stuff seems fine.

It would be good, I think, to point voidspace at this patch as well,
because with this working, we're at point of being able to propose a
patch to core to let regular python run twisted test cases with less
of a complete-reimplementation, which is pretty cool. So I've CC'd him
:)

-Rob
-- 
https://code.launchpad.net/~jml/testtools/deferred-support/+merge/38080
Your team testtools developers is requested to review the proposed merge of lp:~jml/testtools/deferred-support into lp:testtools.



Follow ups

References