Possible bug in nevow.testutil.FakeRequest setHeaders and getHeaders
Recently my nevow web server's unit tests started to fail, because the tests used request.getHeader() to check that request.setHeader() had been called correctly. This used to work. I can see from testutil.py that setHeaders sets self.headers, but getHeaders gets a value from received_headers, and there is a test in test_testutil.py to make sure it does. So is this really how it is meant to work? Is setHeader really meant to set something that getHeader can't get? If so, what's the correct method for testing the result of setHeader? To reproduce the problem, run this with "trial": from nevow.testutil import FakeRequest, TestCase class TestHeaders(TestCase): def setUp(self): self.request = FakeRequest() def test_set(self): self.request.setHeader('location', 'somewhere') self.failUnlessEqual(self.request.getHeader('location'), 'somewhere') Peter.
On 05:23 pm, peter.westlake@pobox.com wrote:
Recently my nevow web server's unit tests started to fail, because the tests used request.getHeader() to check that request.setHeader() had been called correctly. This used to work.
I can see from testutil.py that setHeaders sets self.headers, but getHeaders gets a value from received_headers, and there is a test in test_testutil.py to make sure it does. So is this really how it is meant to work? Is setHeader really meant to set something that getHeader can't get? If so, what's the correct method for testing the result of setHeader?
It's correct that setHeader and getHeader operate on different data sets. Less confusing names for what the two methods do would have been setResponseHeader and getRequestHeader respectively. In newer versions of Twisted, the requests have two new attributes, requestHeaders and responseHeaders, with various methods for inspection and modification. Nevow's Request class should inherit these. However Nevow's FakeRequest probably doesn't. The right way to test for headers is probably to fix the FakeRequest class so that it is more inspectable, and to verify that it actually behaves in the same way as a real request object.
To reproduce the problem, run this with "trial":
from nevow.testutil import FakeRequest, TestCase
class TestHeaders(TestCase): def setUp(self): self.request = FakeRequest()
def test_set(self): self.request.setHeader('location', 'somewhere') self.failUnlessEqual(self.request.getHeader('location'), 'somewhere')
Peter.
Jean-Paul
On Tue, 28 Sep 2010 17:52 +0000, exarkun@twistedmatrix.com wrote: ...
It's correct that setHeader and getHeader operate on different data sets. Less confusing names for what the two methods do would have been setResponseHeader and getRequestHeader respectively.
In newer versions of Twisted, the requests have two new attributes, requestHeaders and responseHeaders, with various methods for inspection and modification. Nevow's Request class should inherit these. However Nevow's FakeRequest probably doesn't.
The right way to test for headers is probably to fix the FakeRequest class so that it is more inspectable, and to verify that it actually behaves in the same way as a real request object.
Thanks for the explanation! I've done this for now: class FakeRequest(nevow.testutil.FakeRequest): def getResponseHeader(self, key): return self.headers.get(key.lower()) Should I put in a ticket requesting something like this? Peter.
On 12:48 pm, peter.westlake@pobox.com wrote:
On Tue, 28 Sep 2010 17:52 +0000, exarkun@twistedmatrix.com wrote: ...
It's correct that setHeader and getHeader operate on different data sets. Less confusing names for what the two methods do would have been setResponseHeader and getRequestHeader respectively.
In newer versions of Twisted, the requests have two new attributes, requestHeaders and responseHeaders, with various methods for inspection and modification. Nevow's Request class should inherit these. However Nevow's FakeRequest probably doesn't.
The right way to test for headers is probably to fix the FakeRequest class so that it is more inspectable, and to verify that it actually behaves in the same way as a real request object.
Thanks for the explanation! I've done this for now:
class FakeRequest(nevow.testutil.FakeRequest): def getResponseHeader(self, key): return self.headers.get(key.lower())
Should I put in a ticket requesting something like this?
Certainly. Things are a little mixed up here though, let's see... Twisted Web provides a base Request class. Nevow subclasses it to create the request class all Nevow applications get. Then Twisted Web also provides a FakeRequest, which isn't very good, and I think there is a ticket (in the Twisted tracker) for improving. Then Nevow provides its own FakeRequest, not a subclass of Twisted's FakeRequest. There probably needs to be a Nevow ticket for doing something about Nevow's FakeRequest class - in the short term, fixing these minor issues; but in the longer term, getting rid of it, and hopefully NevowRequest as well, makes a lot of sense. Jean-Paul
On Wed, 29 Sep 2010 13:00 +0000, exarkun@twistedmatrix.com wrote:
On 12:48 pm, peter.westlake@pobox.com wrote: ...
class FakeRequest(nevow.testutil.FakeRequest): def getResponseHeader(self, key): return self.headers.get(key.lower())
Should I put in a ticket requesting something like this?
Certainly. Things are a little mixed up here though, let's see...
Twisted Web provides a base Request class. Nevow subclasses it to create the request class all Nevow applications get. Then Twisted Web also provides a FakeRequest, which isn't very good, and I think there is a ticket (in the Twisted tracker) for improving. Then Nevow provides its own FakeRequest, not a subclass of Twisted's FakeRequest. There probably needs to be a Nevow ticket for doing something about Nevow's FakeRequest class - in the short term, fixing these minor issues; but in the longer term, getting rid of it, and hopefully NevowRequest as well, makes a lot of sense.
The ticket for set/getHeader is http://divmod.org/trac/ticket/3029. Getting rid of NevowRequest and Nevow's FakeRequest presumably means adding their extra functionality to the ones in Twisted Web, so would that ticket go into Nevow, Twisted, or both? Peter.
On 02:57 pm, peter.westlake@pobox.com wrote:
On Wed, 29 Sep 2010 13:00 +0000, exarkun@twistedmatrix.com wrote:
On 12:48 pm, peter.westlake@pobox.com wrote: ...
class FakeRequest(nevow.testutil.FakeRequest): def getResponseHeader(self, key): return self.headers.get(key.lower())
Should I put in a ticket requesting something like this?
Certainly. Things are a little mixed up here though, let's see...
Twisted Web provides a base Request class. Nevow subclasses it to create the request class all Nevow applications get. Then Twisted Web also provides a FakeRequest, which isn't very good, and I think there is a ticket (in the Twisted tracker) for improving. Then Nevow provides its own FakeRequest, not a subclass of Twisted's FakeRequest. There probably needs to be a Nevow ticket for doing something about Nevow's FakeRequest class - in the short term, fixing these minor issues; but in the longer term, getting rid of it, and hopefully NevowRequest as well, makes a lot of sense.
The ticket for set/getHeader is http://divmod.org/trac/ticket/3029.
Getting rid of NevowRequest and Nevow's FakeRequest presumably means adding their extra functionality to the ones in Twisted Web, so would that ticket go into Nevow, Twisted, or both?
First there would be tickets (plural) for adding features to Twisted's Request and FakeRequest from Nevow. Then there would be tickets (or maybe just a ticket) for deleting the corresponding (then redundant) code from Nevow. Jean-Paul
On Sep 29, 2010, at 11:31 AM, exarkun@twistedmatrix.com wrote:
First there would be tickets (plural) for adding features to Twisted's Request and FakeRequest from Nevow. Then there would be tickets (or maybe just a ticket) for deleting the corresponding (then redundant) code from Nevow.
My two cents here would be to move in the direction of making Request itself a data structure which could be manipulated in memory, rather than having it tightly bound to the HTTP connection, and then having a discrete test double.
On 07:57 pm, glyph@twistedmatrix.com wrote:
On Sep 29, 2010, at 11:31 AM, exarkun@twistedmatrix.com wrote:
First there would be tickets (plural) for adding features to Twisted's Request and FakeRequest from Nevow. Then there would be tickets (or maybe just a ticket) for deleting the corresponding (then redundant) code from Nevow.
My two cents here would be to move in the direction of making Request itself a data structure which could be manipulated in memory, rather than having it tightly bound to the HTTP connection, and then having a discrete test double.
Yea, that sounds good too. Jean-Paul
On Wed, 29 Sep 2010 20:53 +0000, exarkun@twistedmatrix.com wrote:
On 07:57 pm, glyph@twistedmatrix.com wrote:
On Sep 29, 2010, at 11:31 AM, exarkun@twistedmatrix.com wrote:
First there would be tickets (plural) for adding features to Twisted's Request and FakeRequest from Nevow. Then there would be tickets (or maybe just a ticket) for deleting the corresponding (then redundant) code from Nevow.
These are in as http://twistedmatrix.com/trac/ticket/4662 and http://twistedmatrix.com/trac/ticket/4663.
My two cents here would be to move in the direction of making Request itself a data structure which could be manipulated in memory, rather than having it tightly bound to the HTTP connection, and then having a discrete test double.
I put this into the comments for 4662. Peter.
participants (3)
-
exarkun＠twistedmatrix.com -
Glyph Lefkowitz -
Peter Westlake