twisted.web.http.Request.parseCookies has incomplete test coverage

[Jean-Paul Calderone]

It should have complete test coverage (found while porting http.py to Python 3).

[moijes12]

Hi

As far as I can see, there are tests for verifying that http.Request parses cookies in the below ways:

1. Extract cookies from header and add them to recieved_cookies.
2. Extract cookies from multiple cookie headers.

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

if cookieheaders is None:

return

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.

[Tom Prince]

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

[moijes12]

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.

1. Test that malformed cookie-pair before, in middle or at end of header are ignored.

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

[Richard Wall]

(In [39650]) Branching to 'parsecookies-coverage-6119'

[Richard Wall]

(In [39651]) Apply 6119.2.patch from moijes12. Refs: #6119

[Richard Wall]

Thanks moijes12,

Notes:

• Merges cleanly
• Coverage now seems complete

Points:

1. Build Failures
   1. Trailing whitespace: ​https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1150/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
      • Fixed r39666
2. source:branches/parsecookies-coverage-6119/twisted/web/test/test_http.py
   1. Should be two blank lines before "def test_parseCookiesNoCookie(self):"
      • Fixed r39666
   2. Builtin types such as None should be marked up with L{}
      • Fixed r39667
   3. 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
   4. test_parseCookiesMalformedCookie should probably be split into two separate tests.
      • Fixed in r39669
   5. parseCookies does an lstrip on the value so add a test for leaving right hand space in the cookie value.
      • Fixed in r39670
   6. And a test for removing leading space from cookie key
      • Fixed in r39672
   7. parseCookies allows cookies with empty values. Add a test for that.
      • Fixed in r39671
3. parseCookie seems inadequate in various ways:
   1. Allows empty keys " = "
   2. Allows spaces in values "foo = bar baz; "
   3. Misinterprets quoted keys '"foo=bar";baz=qux'
   4. 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.

[Richard Wall]

Back up for review.

Build Results:

• ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/parsecookies-coverage-6119&num_builds=2

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

[lvh]

(In [39897]) Merge parsecookies-coverage-6119: add tests for cookie parsing

Author: rwall, moijes12 Reviewer: lvh, rwall, tom.prince Fixes: #6119

The cookie parsing logic previously had no test coverage. Now it does.

[Jean-Paul Calderone]

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.