#6119 defect closed fixed (fixed)
twisted.web.http.Request.parseCookies has incomplete test coverage
Reported by: | Jean-Paul Calderone | Owned by: | lvh |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | |
Cc: | jknight | Branch: |
branches/parsecookies-coverage-6119
branch-diff, diff-cov, branch-cov, buildbot |
Author: | rwall |
Description
It should have complete test coverage (found while porting http.py to Python 3).
Attachments (2)
Change History (13)
comment:1 Changed 10 years ago by
Cc: | jknight added |
---|
Changed 9 years ago by
Attachment: | 6119.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|
comment:3 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
parseCookies
doesn't have an interesting return value, so there is no reason to assert anything about it. But you are right that that code path isn't covered. If there isn't any headers, then no cookies should be parsed.- The
except ValueError:
is also not covered. You should test that malformed cookie-pair's are ignored (and other cookies set before or after are still properly parsed.
Changed 9 years ago by
Attachment: | 6119.2.patch added |
---|
comment:4 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
I am attaching a patch which has the test covering the "ValueError" codepath i.e. it tests that malformed-cookie pair's are ignored. Here I have added 2 tests in a single test function.
- Test that malformed cookie-pair before, in middle or at end of header are ignored.
- Test that malformed URLs in multiple headers cookies are ignored as in 1.
I have used the code in test_parseCookie and test_parseCookiesMultipleHeaders for achieving this.
comment:5 Changed 9 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/parsecookies-coverage-6119 |
(In [39650]) Branching to 'parsecookies-coverage-6119'
comment:7 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks moijes12,
Notes:
- Merges cleanly
- Coverage now seems complete
Points:
- Build Failures
- source:branches/parsecookies-coverage-6119/twisted/web/test/test_http.py
- Should be two blank lines before "def test_parseCookiesNoCookie(self):"
- Fixed r39666
- Builtin types such as None should be marked up with L{}
- Fixed r39667
- test_parseCookiesNoCookie tests for a None return value, but
the function always returns None. So perhaps there should be separate
tests for return type and another test that
req.received_cookies is empty if no cookies are supplied.
- Fixed in r39668
- test_parseCookiesMalformedCookie should probably be split into
two separate tests.
- Fixed in r39669
- parseCookies does an lstrip on the value so add a test for
leaving right hand space in the cookie value.
- Fixed in r39670
- And a test for removing leading space from cookie key
- Fixed in r39672
- parseCookies allows cookies with empty values. Add a test for that.
- Fixed in r39671
- Should be two blank lines before "def test_parseCookiesNoCookie(self):"
- parseCookie seems inadequate in various ways:
- Allows empty keys " = "
- Allows spaces in values "foo = bar baz; "
- Misinterprets quoted keys '"foo=bar";baz=qux'
- Raise a ticket
- Created #6694
Ok. This started as a code review but I also fixed some things as a went.
So I'll force a new build and put it back up for review.
-RichardW.
comment:8 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
Back up for review.
Build Results:
comment:9 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to lvh |
Hi rwall,
Thanks for working on this!
The twistedchecker buildbot noted an issue:
* Module twisted.web.test.test_http W9010:1486,0: Trailing whitespace found in the end of line
Other than that, these tests look very reasonable, modulo the already existing bogus behavior which is addressed in another ticket.
Since these are two very minor changes, I'll do them.
comment:10 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:11 Changed 9 years ago by
The branch is also introducing tests that pass None
as a false value for a flag parameter (Request.__init__
queued
). This works but is weird. The code should pass False
instead.
Hi
As far as I can see, there are tests for verifying that http.Request parses cookies in the below ways:
I think there is not much to add here except add a check to see that parseCookies function returns None in case there is no cookie header. This is implemented by the below line in parseCookie
I have added the test for the above check. May be we could check that recieved_cookies is empty if the cookie header was not contained in the request. We could implement that if required.