Skip to content
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

Green up the tests #396

Merged
merged 3 commits into from
Sep 16, 2024
Merged

Green up the tests #396

merged 3 commits into from
Sep 16, 2024

Conversation

twm
Copy link
Contributor

@twm twm commented Sep 16, 2024

Remove an invalid HTTP header name from a test case. The header name itself appears to be dead code — the code under test only parses the Content-Type header.

Fixes #395.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Let's have this merged :)

@@ -207,14 +207,16 @@ def test_content_application_json_default_encoding(self):

def test_text_content_unicode_headers(self):
"""
Header parsing is robust against unicode header names and values.
Parsing of the Content-Type header is robust in the face of
Unicode gunk in the header value, which is ignored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with the code from here... so feel free to ignore this comment.

I don't understand the docstring for this test...

I mean, I understand what it wanted to do, but I don't see how this is put into practice in the test code itself.

Suggested change
Unicode gunk in the header value, which is ignored.
Unicode junk in the header value, which is ignored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also be a unicode hunk (in this case meant as a byte sequence \xe1\x9b\x83 - which is the byte representation of the utf-8 code point of the given character).

All those bytes are invalid for http header value.

Copy link
Contributor Author

@twm twm Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly — "ᛃ" is the "gunk". This is basically a smoke test that there isn't a .decode('ascii') in the header processing that might blow up.

@twm
Copy link
Contributor Author

twm commented Sep 16, 2024

Thanks for taking a look @adiroiban and @pmisik!

@twm twm merged commit 1de1793 into trunk Sep 16, 2024
16 checks passed
@twm twm deleted the 395-headers-tests branch September 16, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update tests for Twisted Headers changes
3 participants