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

python 3: Add python 3 support for html tests #22402

Closed
wants to merge 1 commit into from

Conversation

ziransun
Copy link
Member

[1] Fix some TypeErrors. Calling get API in reuqest.headers
() returns binary string values.
In python 2, bytes is alias to str type. In Python 3, str type is
different from bytes. This change uses utf-8 encoding for the request.headers
values using for Py2 and Py3 compability.
[2] Add python 3 support for accessing headers content in raw_headers
(xhr/resources/inspect-headers.py). Type of raw_headers is class
HTTPMessage. In Python 2, HTTPMessage derives from mimetools.Message while
in Python 3, it derives from email.message.Message. Interfaces from
mimetools.Message and email.message.Message are very different.
In Python 3, HTTPMessage doesn't have 'headers" attribute or equivalent.
[ref: https://bugs.python.org/issue4773]
[3] Fix some imports for python2 and python 3 compatibility

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of all the encode/decode operations here. There might be a better way to accomplish casting from one type to another.

cookie = request.cookies.first(id, None)
cookie_value = cookie.value if cookie is not None else "no"
line = 'cors = ' + cors + ' | cookie = ' + cookie_value;
line = 'cors = ' + cors.decode("utf-8") + ' | cookie = ' + cookie_value
Copy link
Member

Choose a reason for hiding this comment

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

Is this casting a byte sequence to a string, basically? Are there really UTF-8 bytes in there?

else:
header_list = raw_headers.headers
for line in header_list:
if not PY3:
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it okay to be more specific? raw_headers derives from different classes in Python 2 and Python 3. Here is to handle them differently.

Copy link
Member

Choose a reason for hiding this comment

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

In particular, it seems like the code isn't executed at all in Python 3.

Copy link
Member Author

Choose a reason for hiding this comment

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

raw_headers derives from different classes in python 2 and python 3. In Python 3, HTTPMessage doesn't have 'headers" attribute or equivalent as we described in the commit message (see the reference as well). That's why this part code is not executed in Python 3.

Copy link
Member

Choose a reason for hiding this comment

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

We can't just drop testing coverage. We probably need a better solution to how we deal with header logic migration in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here a few new lines added for python 3 to perform similar functionality. It basically hasn't touched/removed any original python 2 code.

Copy link
Member

Choose a reason for hiding this comment

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

Where does the Python 3 code test for a missing CRLF?

[1] Fix some TypeErrors. Calling get API in reuqest.headers
(<class wptserve.request.RequestHeaders>) returns binary string values.
In python 2, bytes is alias to str type. In Python 3, str type is
different from bytes. This change uses utf-8 encoding for the request.headers
values using for Py2 and Py3 compability.
[2] Add python 3 support for accessing headers content in raw_headers
(xhr/resources/inspect-headers.py). Type of raw_headers is class
HTTPMessage. In Python 2, HTTPMessage derives from mimetools.Message while
in Python 3, it derives from email.message.Message. Interfaces from
mimetools.Message and email.message.Message are very different.
In Python 3, HTTPMessage doesn't have 'headers" attribute or equivalent.
[ref: https://bugs.python.org/issue4773]
[3] Fix some imports for python2 and python 3 compatibility
@ziransun
Copy link
Member Author

Code has been updated as per review comments. Please review. Thanks!

result = request.body == 'foo=bara'
elif request.headers.get('Content-Type') == 'text/plain':
result = request.body == 'qux=baz\r\n'
if ensure_str(request.headers.get('Content-Type')) == 'application/x-www-form-urlencoded':
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to use b"" here (and below) on the other side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't this kind information be string?

Copy link
Member

Choose a reason for hiding this comment

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

Why?

import tempfile

from six import BytesIO, binary_type, ensure_str, text_type, iteritems, PY3
from six.moves.http_cookies import BaseCookie
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved? @jgraham should probably look at this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reordered these lines due to pylint complains.

else:
header_list = raw_headers.headers
for line in header_list:
if not PY3:
Copy link
Member

Choose a reason for hiding this comment

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

In particular, it seems like the code isn't executed at all in Python 3.

@stephenmcgruer stephenmcgruer assigned annevk and unassigned KyleJu Mar 30, 2020
@jgraham
Copy link
Contributor

jgraham commented Apr 1, 2020

I think per the decision not to work on converting handlers until we have a better testing story, we should close this one for now. Feel free to reopen if I got that wrong.

@jgraham jgraham closed this Apr 1, 2020
ziransun added a commit to ziransun/wpt that referenced this pull request May 1, 2020
…orting guide

This is a rework on web-platform-tests#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49
ziransun added a commit to ziransun/wpt that referenced this pull request May 4, 2020
…orting guide

This is a rework on web-platform-tests#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49
ziransun added a commit to ziransun/wpt that referenced this pull request May 7, 2020
…orting guide

This is a rework on web-platform-tests#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49
ziransun added a commit to ziransun/wpt that referenced this pull request May 11, 2020
…orting guide

This is a rework on web-platform-tests#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49
stephenmcgruer pushed a commit that referenced this pull request May 26, 2020
… guide (#23363)

This is a rework on #22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49

Co-authored-by: Robert Ma <[email protected]>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 30, 2020
… Python file handlers p…, a=testonly

Automatic update from web-platform-tests
Python 3: Port some html tests following Python file handlers porting guide (#23363)

This is a rework on web-platform-tests/wpt#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49

Co-authored-by: Robert Ma <[email protected]>
--

wpt-commits: e60c41610959f414e24fe2360151d94953436906
wpt-pr: 23363
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request May 30, 2020
… Python file handlers p…, a=testonly

Automatic update from web-platform-tests
Python 3: Port some html tests following Python file handlers porting guide (#23363)

This is a rework on web-platform-tests/wpt#22402
This change follows the porting guide discussed in RFC49:
web-platform-tests/rfcs#49

Co-authored-by: Robert Ma <[email protected]>
--

wpt-commits: e60c41610959f414e24fe2360151d94953436906
wpt-pr: 23363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants