-
-
Notifications
You must be signed in to change notification settings - Fork 937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(test_cookies): case sensitivity #558
Conversation
I got these errors while implementing the test suite in the AUR package for falcon. It looks to be missing proper capitalization. Output has been tweaked to be less than 100 characters per line. ====================================================================== FAIL: test_response_base_case (tests.test_cookies.TestCookies) tests.test_cookies.TestCookies.test_response_base_case ---------------------------------------------------------------------- testtools.testresult.real._StringException: Traceback (most recent call last): File "/home/carl/aur/python-falcon/falcon-0.3.0/tests/test_cookies.py", line 60, in test_response_base_case self.srmock.headers) File "/usr/lib/python3.4/site-packages/testtools/testcase.py", line 356, in assertIn self.assertThat(haystack, Contains(needle), message) File "/usr/lib/python3.4/site-packages/testtools/testcase.py", line 435, in assertThat raise mismatch_error testtools.matchers._impl.MismatchError: ('set-cookie', 'foo=bar; Domain=example.com; httponly; Path=/; secure') not in [ ('content-length', '0'), ('set-cookie', 'foo=bar; Domain=example.com; HttpOnly; Path=/; Secure') ] ====================================================================== FAIL: test_response_complex_case (tests.test_cookies.TestCookies) tests.test_cookies.TestCookies.test_response_complex_case ---------------------------------------------------------------------- testtools.testresult.real._StringException: Traceback (most recent call last): File "/home/carl/aur/python-falcon/falcon-0.3.0/tests/test_cookies.py", line 67, in test_response_complex_case self.srmock.headers) File "/usr/lib/python3.4/site-packages/testtools/testcase.py", line 356, in assertIn self.assertThat(haystack, Contains(needle), message) File "/usr/lib/python3.4/site-packages/testtools/testcase.py", line 435, in assertThat raise mismatch_error testtools.matchers._impl.MismatchError: ('set-cookie', 'foo=bar; httponly; Max-Age=300; secure') not in [ ('content-type', 'application/json; charset=utf-8'), ('set-cookie', 'bar=baz; Secure'), ('set-cookie', 'foo=bar; HttpOnly; Max-Age=300; Secure') ]
I'm not sure why the Travis build failed, but here is what I do know.
If I'm off base here feel free to just close this out. Otherwise I would love feedback on how to improve the pull request. Thanks! |
@carlgeorge I tested with 2.7.9, 3.4.2, and 3.4.3. It appears that the capitalization change occurred in 3.4.3. There may be a more elegant way to work around this, but my first thought is to explicitly pull out the 'set-cookie' header value, then lowercase in order to normalize capitalization, e.g.,: actual_value = self.srmock.headers_dict['set-cookie']
self.assertEqual(actual_value.lower(),
'foo=bar; domain=example.com; httponly; path=/; secure') Thanks! |
I don't think that approach will work in the
Let me brainstorm on this and see if I can come up with a cleaner solution. |
This is a more robust solution than I attempted in commit f0aa9c6.
self.srmock.headers) | ||
value = "foo=bar; Domain=example.com; HttpOnly; Path=/; Secure" | ||
upper = ("set-cookie", value) in self.srmock.headers | ||
lower = ("set-cookie", value.lower()) in self.srmock.headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain and Path are capitalized in both CPython 3.4.2 and 3.4.3; it's only HttpOnly and Secure that change.
@carlgeorge Just following up to see if you've had a chance to review my latest comments? |
@kgriffs Yes, I've just been busy. I'll try to fix this PR soon. |
@kgriffs Latest commit seems to work, all Travis builds have passed. |
@carlgeorge This is looking good. What do you think about @desiderius comment in #573 re using unittest.skipif()? |
@kgriffs It would depend on how it is refactored I think. Even if you refactor the test later, I don't see a reason not to merge this fix for now. @desiderius I'm curious, when you mentioned using skipIf, is your intention to check for the new capitalization on 3.4.3+, and just skip it on older versions? Or just skip the check on 3.4.3+ and only test the older versions? |
@carlgeorge I was thinking about something like splitting the @unittest.skipIf(sys.version_info < (3, 4, 3), 'Change in attribute capitalization')
def test_head_request_cookie_contains_httponly(self) @unittest.skipIf(sys.version_info >= (3, 4, 3), 'Change in attribute capitalization')
def test_head_request_cookie_contains_HttpOnly(self)
# ... 💭 maybe the test names differing only in capitalization is so so, but you get the idea. |
@desiderius That seems inefficient to me, because those two test would be the same except for the Regardless, it's up to @kgriffs to decide which approach to take. |
@carlgeorge If by efficiency you mean fast, then you're certainly right that tests must be fast. My thought was more about long term maintainability than anything else. Anyway working code speak stronger than wishful thinking and your commit is here. So as you said latter a refactoring can be made someday if necessary. |
Merged manually. We can refactor in another patch if needed. |
@carlgeorge, @desiderius: Thanks for your help! |
I got these errors while implementing the test suite in the AUR package for falcon. It looks to be
missing proper capitalization. Output has been tweaked to be less than 100 characters per line.