Expose twisted.web.test._util._render to help third-party code in testing resrources.

[tom.prince]

It looks like as currently implemented, twisted.web.test._util._render may not work in the NOT_DONE_YET case, if the request it is passed is a real request.

1. It is a queued request, and so the deferred never fires.

2. It isn't, and request.content == None doesn't exist causing an error in Request._cleanup

I'm not sure how best to handle this, or if this is the right approach for adding this testing utility.

[tom.prince]

initial patch

[dreid]

Thanks for looking at this tom.

There is a big problem with your patch in that it doesn't include the added file twisted/web/test/util.py I'm going to go ahead and review the rest of your patch as is to save time and hopefully a review cycle.

1. render needs a docstring.
2. All test methods should have docstrings.
3. Your ticket description mentions that render doesn't work with a real request. If this is an actual issue there should presumably be a test which attempts to use render with a real request, however I'm skeptical that is an issue. Possibly render should just be documented as taking a DummyRequest?
4. Lines 223-224 in twisted/web/test/test_static.py are aggressively wrapped. self.assertEqual(res, res2) would be fine.
5. pyflakes

twisted/web/test/_util.py:9: 'succeed' imported but unused
twisted/web/test/_util.py:10: 'server' imported but unused
twisted/web/test/test_distrib.py:13: redefinition of unused 'pwd' from line 11
twisted/web/test/test_distrib.py:17: 'log' imported but unused
twisted/web/test/test_distrib.py:22: 'http' imported but unused
twisted/web/test/test_http.py:9: 'urllib' imported but unused
twisted/web/test/test_http.py:1445: local variable 'transport' is assigned to but never used
twisted/web/test/test_soap.py:11: redefinition of unused 'SOAPpy' from line 9
twisted/web/test/test_util.py:392: undefined name 'DummyRequest'
twisted/web/test/test_util.py:404: undefined name 'DummyRequest'
twisted/web/test/test_util.py:416: undefined name 'DummyRequest'
twisted/web/test/test_vhost.py:68: redefinition of function 'cbRendered' from line 60
twisted/web/test/test_web.py:604: 'google' imported but unused
twisted/web/test/test_webclient.py:28: 'IPv4Address' imported but unused
twisted/web/test/test_webclient.py:35: '_parse' imported but unused
twisted/web/test/test_webclient.py:44: redefinition of unused 'ssl' from line 42
twisted/web/test/test_webclient.py:46: 'ContextType' imported but unused
twisted/web/test/test_webclient.py:1722: local variable 'endpoint' is assigned to but never used
twisted/web/test/test_webclient.py:1742: local variable 'endpoint' is assigned to but never used
twisted/web/test/test_webclient.py:1781: local variable 'result' is assigned to but never used
twisted/web/test/test_webclient.py:2690: local variable 'headers' is assigned to but never used
twisted/web/test/test_webclient.py:2691: local variable 'result' is assigned to but never used
twisted/web/test/test_wsgi.py:915: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:955: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1019: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1054: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1091: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1120: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1148: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1195: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1233: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1473: local variable 'request' is assigned to but never used
twisted/web/test/test_wsgi.py:1526: local variable 'request' is assigned to but never used
twisted/web/test/test_xml.py:625: local variable 'd' is assigned to but never used
twisted/web/test/test_xmlrpc.py:25: 'twisted' imported but unused

the subset of pyflakes errors caused by this patch (and should be addressed in the next patch) appear to be:

twisted/web/test/_util.py:9: 'succeed' imported but unused
twisted/web/test/_util.py:10: 'server' imported but unused
twisted/web/test/test_util.py:392: undefined name 'DummyRequest'
twisted/web/test/test_util.py:404: undefined name 'DummyRequest'
twisted/web/test/test_util.py:416: undefined name 'DummyRequest'
twisted/web/test/test_vhost.py:68: redefinition of function 'cbRendered' from line 60

[tom.prince]

Replying to dreid:

1. Your ticket description mentions that render doesn't work with a real request. If this is an actual issue there should presumably be a test which attempts to use render with a real request, however I'm skeptical that is an issue. Possibly render should just be documented as taking a DummyRequest?

Quoting from DummyRequest:

This implementation only handles a few of the most common behaviors of resources. It can handle a render method that returns a string or C{NOT_DONE_YET}. It doesn't know anything about the semantics of request methods (eg HEAD) nor how to set any particular headers. Basically, it's largely broken, but sufficient for some tests at least. It should B{not} be expanded to do all the same stuff L{Request} does. Instead, L{DummyRequest} should be phased out and L{Request} (or some other real code factored in a different way) used.

Which seems to suggest the opposite.

[tom.prince]

twisted/web/test/util.py - (silly svn)

[tom.prince]

Also, suggesting the use of DummyRequest instead of Request would require adding tests and documentation for it, since render is intended to be a public api for external code to use when writing tests.

[dreid]

Ok, fair enough, don't require the use of DummyRequest. Also have some tests that use Request.

[tom.prince]

patch with http.Request, instead of DummyRequest.

[tom.prince]

<dreid> tomprince: OK, so test_render_NOT_DONE_YET hangs.
<dreid> Exciting.
<tomprince> dreid: And I tracked down why, more or less, but I don't know how best to solve it.
<dreid> tomprince: OK, so why do you think it hangs?
<tomprince> Excatly what I said in my original comment.
<tomprince> A request with queued=True never triggers the notifyFinish deferred, since _cleanup isn't called in that case.
<dreid> tomprince: Hrmm… maybe you actually want to do request.render(getChildForRequest(resource, request)) instead of just resource.render?
<dreid> Or just request.render(resource) if you don't ever want util.render() to invoke child lookup machinery.
<dreid> No, that's a lie i think.
<tomprince> Is there such a thing.
<dreid> Yeah but it does too much magic I think.
<tomprince> Two possiblities occur to me: 1) http.Request might need to be modified to support this 2) DummyRequest should in fact be used, notwithstanding the comment in DummyRequest.render
<tomprince> Hmm. Should I be using server.Request, instead of http.Request?
<tomprince> Wait, I am, but just wasn't looking at it.
<dreid> tomprince: What happens if you use Request(DummyChannel(), False)?
<dreid> tomprince: I don't think it really makes sense to use a queued request in the tests?
<dreid> But that shouldn't have anything to do with it because you're monkey patching request.write.
<tomprince> https://gist.github.com/2048761
<dreid> tomprince: ok, so render might need to setup request.content as an empty stringio unless request.content is already set.
<tomprince> I'll try that out.
<dreid> So to test post for example, you'd do r = Request(…); r.method = "POST"; r.content = StringIO(…); render(r, resource)
<dreid> Also I'm pretty sure you don't want to monkey patch request.write
<dreid> because request.write does a lot of stuff.
<tomprince> Well, I was wanting to grab just the body.
<dreid> But that's not the fully rendered request. :\
<tomprince> But maybe it would be better to hook up a Response, and return that.
<tomprince> On the other hand, my goal is to make something that makes it easy to test things.
<tomprince> One of things I *don't* want is to expose the wire format of an http response to the user.
<dreid> No, I think you're right.  If you want to check codes or headers you should assert those things as they are set on the request.
<dreid> Like the RedirectToTestCase does.
<dreid> tomprince: So… I'm not sure if it's reasonable to make this support DummyRequest and Request at once.

[tom.prince]

Simpified patch.