testtools-dev team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

On Sun, Oct 10, 2010 at 9:49 PM, Robert Collins
<robertc@xxxxxxxxxxxxxxxxx> wrote:
> 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.
>

The Twisted-specific bits (specifically _Spinner, DeferredNotFired,
extract_result, UnhandledErrorInDeferred, trap_unhandled_errors,
UncleanReactorError, TimeoutError) could well find a home in Twisted.
Some of them already have analogues.

However, I'm not in a rush to push those changes through:
  * _Spinner cannot go into Twisted as-is until Twisted stops using
setUpClass. It would have to become less strict. I don't want that for
my testtools-using code.
  * Putting extract_result into Twisted has been discussed in the
past, but it has been discouraged because it encourages newbies to
abuse it.
  * trap_unhandled_errors is kind of a hack. It only works here
because we're controlling the reactor and making assumptions about
test isolation.

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

You mean it should be a lambda or you mean it should be a method? It's
all the same to me, as long as it's not a part of the interface.

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

Changed. Note that KeyboardInterrupt will not be raised for Ctrl-C
while the reactor is running.

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

OK. Will fix this.

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

We aren't squashing them, Twisted is. It installs a SIGINT handler
that overrides the default one. Without the default, Ctrl-C won't
raise KeyboardInterrupt.

> 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
> :)
>

I'd be very reluctant to put this code into the standard library as
is. It's experimental, and the Twisted community frowns strongly on
anything that turns async code to sync. However, if you just mean the
RunTest mechanism, I'd be less reluctant, but still think it needs
more usage out in the wild before standardizing. (e.g. there's no
convenient syntax for using a different RunTest object)

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