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.

Sign up for GitHub

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

Fix manifest writing in python3 #22139

Merged
merged 10 commits into from
Mar 13, 2020
Merged

Fix manifest writing in python3 #22139

merged 10 commits into from
Mar 13, 2020

Conversation

svillar
Copy link
Contributor

@svillar svillar commented Mar 9, 2020

The main problem is that the json module cannot process binary data. That was not a problem in python2 where binary==str but it was causing issues in python3. The data causing issues were the file hashes. From now on the hashes are converted to str when converting data to json.

LukeZielinski
LukeZielinski previously requested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@LukeZielinski LukeZielinski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like the tests/test_wpt.py unit tests are failing with TypeErrors, please take a look at those (appears to be different sites between linux and windows)

tools/manifest/sourcefile.py Outdated Show resolved Hide resolved
@svillar
Copy link
Contributor Author

svillar commented Mar 9, 2020

Looks like the tests/test_wpt.py unit tests are failing with TypeErrors, please take a look at those (appears to be different sites between linux and windows)

Taking a look now. They appear only when there is no previous MANIFEST.json file, perhaps it requires some more adjustments here and there.

@svillar svillar added the python3 label Mar 9, 2020
@svillar svillar closed this Mar 10, 2020
@svillar svillar reopened this Mar 10, 2020
@svillar
Copy link
Contributor Author

svillar commented Mar 10, 2020

I've filled a bug in virtualenv for the remaining issue pypa/virtualenv#1710.

I think I'll add an xfail for py3 && Windows as well for the test_wpt tests until the issue mentioned above is clarified.

@LukeZielinski
Copy link
Contributor

Punting to @jgraham since I'll be OoO soon and he also expressed an opinion on the ensure_str usage in #22061

jgraham
jgraham requested changes Mar 10, 2020
View reviewed changes
tools/manifest/item.py Outdated Show resolved Hide resolved
tools/manifest/sourcefile.py Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot added the wptrunner The automated test runner, commonly called through ./wpt run label Mar 12, 2020
jgraham
jgraham reviewed Mar 12, 2020
View reviewed changes
tools/manifest/item.py Outdated Show resolved Hide resolved
jgraham
jgraham approved these changes Mar 13, 2020
View reviewed changes
tools/wptrunner/requirements.txt Show resolved Hide resolved
@jgraham jgraham merged commit d4cdf40 into web-platform-tests:master Mar 13, 2020
@svillar svillar deleted the testwpt3 branch March 13, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgraham jgraham jgraham approved these changes

@LukeZielinski LukeZielinski LukeZielinski left review comments

@Hexcles Hexcles Awaiting requested review from Hexcles

@KyleJu KyleJu Awaiting requested review from KyleJu

@stephenmcgruer stephenmcgruer Awaiting requested review from stephenmcgruer

Assignees

@jgraham jgraham

Labels
infra manifest python3 wpt wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@svillar @LukeZielinski @jgraham @wpt-pr-bot