[Twisted-Python] Testing Strategies (was Re: Streaming Requests)

[Glyph Lefkowitz]

> On Jan 9, 2017, at 4:13 AM, Jean-Paul Calderone <exarkun at twistedmatrix.com> wrote:
> 
> On Mon, Jan 9, 2017 at 4:52 AM, Glyph Lefkowitz <glyph at twistedmatrix.com <mailto:glyph at twistedmatrix.com>> wrote:
> On Jan 8, 2017, at 4:34 PM, Jean-Paul Calderone <exarkun at twistedmatrix.com <mailto:exarkun at twistedmatrix.com>> wrote:
>> 
>> Here's one example I know of off the top of my head, <https://github.com/ScatterHQ/flocker/blob/master/flocker/restapi/testtools.py#L316 <https://github.com/ScatterHQ/flocker/blob/master/flocker/restapi/testtools.py#L316>>.  This one isn't a from-scratch re-implementation of the implicit Request interface but a Request subclass, instead.  However, it still interacts with a lot of important pieces of Request which aren't part of IRequest.
> 
> Mark already addressed how he won't be breaking this use-case (which is hugely important, and core to the whole idea of a compatibility policy, so that is as it should be).
> 
> However, this kind of test-mocking is, I think, ultimately done at the wrong layer.  It's trying to override some very vaguely-specified internals in the middle of an implementation.
> 
> 
> Absolutely.

Yay! Glad to have some consensus on this.

> Really, twisted.web should provide its own testing tools, of course.  But if you're going to implement something that does this sort of overriding, I think the idiom to follow would be treq.testing: https://github.com/twisted/treq/blob/fcf5deb976c955ca6ef6484f414d25839932940e/src/treq/testing.py <https://github.com/twisted/treq/blob/fcf5deb976c955ca6ef6484f414d25839932940e/src/treq/testing.py>, rather than any of the various implementations of DummyRequest (including more than a few I'm sure I've written).
> 
> Though, note, the link I gave above was support code for something very similar to this treq code: https://github.com/ScatterHQ/flocker/blob/master/flocker/restapi/testtools.py#L460 <https://github.com/ScatterHQ/flocker/blob/master/flocker/restapi/testtools.py#L460>
> 
> To clarify your point a bit (I think):
> 
> MemoryAgent (from Flocker) provides a testing IAgent by implementing an IRequest that does in-memory resource traversal to dispatch requests and generate responses.
> RequestTraversalAgent (from treq) provides a testing IAgent by implementing (using) iosim to do in-memory protocol/transport interacts to drive an in-memory HTTP conversation that runs all of the regular Twisted Web HTTP processing machinery.
> RequestTraversalAgent's approach is better because the protocol/transport interface is better defined.  Because it's better defined, RequestTraversalAgent doesn't have to touch pieces that we might want to consider implementation details (whether they're _-prefixed or not).  It also invokes a larger portion of the real implementation code making it more likely to be a realistic simulation of real-world use of the code.

100% agree.

> Having spelled this out, what occurs to me now is that RequestTraversalAgent is really just a step or so up the ladder and there's further to go.

I generally agree with this next bit, but I have my own spin.  So, one point at a time:

> For example, RequestTraversalAgent only needs to exist at all because `iosim` has a distinct interface.  This distinct interface means you need a RequestTraversalAgent-like thing for each reactor-using thing for which you want to provide a test double.

I think that there are pieces of RequestTraversalAgent for which this is true; the parts where it has to instantiate its own FakeTransport objects and IPv4Addresses and whatever.  This could be streamlined a lot more.

But at the higher level, each layer will want its own public API entrypoint, to facilitate the orchestration and setup of domain-specific client/server objects and reduce boilerplate for testing at a given level.

> If this adaption behavior were bundled up differently then I think we'd get a lot more in-memory test doubles for free (or closer to it - we'd be another rung up the ladder, at least).

This I definitely agree with.  Each abstraction-level-specific fake should be a paper-thin layer over the one below, and each abstraction layer should cover a fair amount of ground for itself.  For example, you can test Klein apps trivially with treq.testing, without adding very much extra logic at all.

> That seems like it would be a big win given how few of these testing helpers Twisted has historically managed to provide (suggesting it's just too hard to do so given the current tools).

Here I think there are a few mitigating factors:

I don't actually think that it's that hard using current tools.  It's pretty easy, in fact, to build up from the bottom and layer subsequent things on top of another.
The way I see the problem that we're facing is that we realized we should be doing this many years after doing the actual implementation, and we realized this several layers up.  So we built mid-tier test mocks and then tried to build on top of them, rather than heading straight for the bottom layer.  This has lead to awkwardness like needing to monkeypatch HostnameEndpoint to avoid deferToThread calls.
So, even now, we have an anemic bottom layer.  task.Clock is pretty good, but MemoryReactor still leaves a lot to be desired.
Therefore, I don't think that we really need to build that many different abstraction layers; if we really focus on nailing down (A) MemoryReactor+iosim+something with addresses so 'connectTCP' can be hooked up to 'listenTCP' when both are given the same address, and (B) the vast majority of what folks want to be able to test is HTTP API type stuff, so integrating treq.testing (treq is license-compatible, so we can just move the code if we want) would get us a _long_ way there, and would popularize this style with a lot of other developers who might then be motivated to help improve the more obscure corners, like, say, DNS.

I've been slowly improving the testability of Twisted's core; the recent name-resolver change made a LOT of things easier.  We should be making deterministicResolvingReactor and twisted.threads (along with MemoryWorker) public, and a lot of the circuitous routes that higher-level code needed to traverse to get to "no I/O" will just go away.  I'm probably forgetting a few other building blocks, but I've noticed that each additional one that I remove makes the task more like exponentially than linearly easier.

> This is a set of ideas that I've been gradually arriving at over a long period of time, but it's probably high time for some public discussion by now, even if it's just everybody saying "yeah, that sounds good" :-).
> 
> So, it pretty much sounds good, though with the above refinements. :)

Cool.  I suspect I haven't even covered all of it with my refine-refinements above, but at this point it seems clear we can all pull in the same direction.  I wonder if a themed sprint at some point in the next year might be worthwhile?

> 
> On a related note, 'proto_helpers' is in a super awkward place.  'twisted.testing', anyone? :)
> 
> Yes.  I almost suggested this about a week ago when I was preparing to contribute some testing code but then realized I couldn't contribute the code after all. :(

Well, now it's just a matter of time :).

-g
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20170111/5edb52b1/attachment-0002.html>