testtools-dev team mailing list archive
-
testtools-dev team
-
Mailing list archive
-
Message #00109
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