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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

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

annevk
annevk reviewed Mar 24, 2020
View reviewed changes
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.

html/semantics/embedded-content/media-elements/track/track-element/cors/support/cors-tester.py Outdated Show resolved Hide resolved
html/semantics/embedded-content/media-elements/track/track-element/cors/support/cors-tester.py Outdated
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?

html/semantics/embedded-content/the-img-element/404-response-with-actual-image-data.py Outdated Show resolved Hide resolved
xhr/resources/inspect-headers.py
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?

@ziransun
python 3: Add python 3 support for html tests
dba610c 
[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
@wpt-pr-bot wpt-pr-bot requested a review from zqzhang March 24, 2020 20:05
@ziransun
Copy link
Member Author

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

annevk
annevk reviewed Mar 25, 2020
View reviewed changes
html/semantics/forms/form-submission-0/resources/form-submission.py
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?

tools/wptserve/wptserve/request.py
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.

xhr/resources/inspect-headers.py
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
@Hexcles Hexcles mentioned this pull request Apr 7, 2020
Merged
ziransun added a commit to ziransun/wpt that referenced this pull request May 1, 2020
@ziransun
[WIP] Python 3: Port some html tests following Python file handlers p…
4367655 
…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 ziransun mentioned this pull request May 1, 2020
Merged
ziransun added a commit to ziransun/wpt that referenced this pull request May 4, 2020
@ziransun
[WIP] Python 3: Port some html tests following Python file handlers p…
f38f3b0 
…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
@ziransun
[WIP] Python 3: Port some html tests following Python file handlers p…
008e913 
…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
@ziransun
[WIP] Python 3: Port some html tests following Python file handlers p…
fab340c 
…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
@ziransun @Hexcles
Python 3: Port some html tests following Python file handlers porting…
e60c416 
… 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 <bob1211@gmail.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 30, 2020
@ziransun @moz-wptsync-bot
Bug 1634749 [wpt PR 23363] - Python 3: Port some html tests following…
6be75de 
… 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 <bob1211@gmail.com>
--

wpt-commits: e60c41610959f414e24fe2360151d94953436906
wpt-pr: 23363
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request May 30, 2020
@ziransun @moz-wptsync-bot
Bug 1634749 [wpt PR 23363] - Python 3: Port some html tests following…
34af4fc 
… 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 <bob1211@gmail.com>
--

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

@annevk annevk annevk left review comments

@caitp caitp Awaiting requested review from caitp

@domenic domenic Awaiting requested review from domenic

@foolip foolip Awaiting requested review from foolip

@Hexcles Hexcles Awaiting requested review from Hexcles

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@KyleJu KyleJu Awaiting requested review from KyleJu

@LukeZielinski LukeZielinski Awaiting requested review from LukeZielinski

@Manishearth Manishearth Awaiting requested review from Manishearth

@mikewest mikewest Awaiting requested review from mikewest

@stephenmcgruer stephenmcgruer Awaiting requested review from stephenmcgruer

@tkent-google tkent-google Awaiting requested review from tkent-google

@wisniewskit wisniewskit Awaiting requested review from wisniewskit

@zcorpan zcorpan Awaiting requested review from zcorpan

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees

@annevk annevk

Labels
cookies html infra wptserve xhr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ziransun @jgraham @annevk @KyleJu @wpt-pr-bot