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

Small python3 compatibility fixes #21038

Merged
merged 5 commits into from Jan 14, 2020

Conversation

@marmeladema
Copy link
Contributor

marmeladema commented Jan 4, 2020

I found some problems again when trying to run servo wpt tests with Python3 (see servo/servo#25377)

This is step in the right direction but not sure yet if it will suffice.

@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from 62497c4 to f18d200 Jan 4, 2020
@marmeladema marmeladema mentioned this pull request Jan 4, 2020
2 of 2 tasks complete
@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch 2 times, most recently from 3d19adb to 26a6849 Jan 4, 2020
@wpt-pr-bot wpt-pr-bot added the fetch label Jan 4, 2020
@wpt-pr-bot wpt-pr-bot added the assets label Jan 4, 2020
@wpt-pr-bot wpt-pr-bot requested review from deniak and hillbrad Jan 4, 2020
@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from 8393085 to 26a6849 Jan 4, 2020
@annevk
Copy link
Member

annevk commented Jan 4, 2020

Have we been adding these kind of wrapper functions to much code already?

I think either this needs some inline documentation or linting as otherwise this will regress again. It also makes this code harder to maintain which isn't great.

@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 4, 2020

six has been used in various places to provide compatibility with python3 in the past few months. I can try to add unit tests that will be run with py36 but it will not cover all the cases I am fixing as I encounter them.

Imo, the right way is to start running some of the tests with py3, not only the lint and unit tests. Would that be ok ?

EDIT: my goal is to run those tests with python3, i think its enough for now to detect regressions.

@annevk
Copy link
Member

annevk commented Jan 6, 2020

I had two concerns, regressions and maintainability. It sounds like there's a plan to deal with regressions, but not necessarily maintenance.

Would it be a big ask that for the test files we have an explicit # ensure_str is for the Py2 -> Py3 transition comment alongside/before usage so it's clear why it's there and when it can be removed?

@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 6, 2020

I could add it here but that would be weird to add it only here and not everywhere it's being used.

If you really want comments, I guess I can deal with it ...

Imo everything from the six module is for py2/py3 compatibility as it's the sole purpose of this module and it's well known enough that it should not be a surprise.

@annevk
Copy link
Member

annevk commented Jan 6, 2020

I might be an outlier, but the main reason I write Python these days is for these small WPT server scripts. It's certainly new to me.

@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 6, 2020

Is there a dev documentation where I could add what is six and why it's being used?

@jgraham
jgraham approved these changes Jan 7, 2020
Copy link
Contributor

jgraham left a comment

I'm sure this will regress, but I think the ultimate solution will be to run tests with both Python 3 and Python 2 and ensure that we get the expected results in each case. Documentation is good, but in practice most people won't read it.

Copy link
Member

annevk left a comment

I would like to see documentation. Again, especially for things that affect writing tests.

Not really sure why documentation is suddenly not considered important. When onboarding people it's super helpful to have good documentation and I personally refer to https://web-platform-tests.org/ quite often for patterns I don't often use.

@jgraham
Copy link
Contributor

jgraham commented Jan 7, 2020

My point of view is that in the short term we "don't care" about regressions here. Since we don't really have any way to enforce correctness we have to be content to accept that authors can and will land things that don't work with Python 3. Therefore at present we aren't imposing any new requirements on anyone, and there isn't a strong need to add documentation at this stage.

Once we decide that enough code works with Python 3 to make it a going concern we'll need an RFC decision to make Py3 an officially supported environment, and that will require documentation and a strategy to keep things working in both environments (e.g. running the tests in both, linting, etc.).

So this isn't about "not considering documentation important". It's about not blocking forward progress on changes to make the code work in order to deal with issues that can be safely deferred to the time when they are actual requirements.

That said, if there were a docs change PR now I'd accept it as long as it didn't imply that Py3 support was actually required.

@annevk
Copy link
Member

annevk commented Jan 7, 2020

Again, my main concern is not regressions, it's maintenance.

@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 7, 2020

I will change the document @annevk pointed me to. Just did not had time yet 👍

@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from 26a6849 to 8ebf6d9 Jan 13, 2020
@wpt-pr-bot wpt-pr-bot added the docs label Jan 13, 2020
@wpt-pr-bot wpt-pr-bot requested a review from sideshowbarker Jan 13, 2020
@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from 8ebf6d9 to fdaabfc Jan 13, 2020
@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 13, 2020

@annevk @jgraham just rebased and added a note relative to python3 compatibility in the python handlers doc as requested. Sorry for the time it took, i was pretty busy.

@annevk
annevk approved these changes Jan 13, 2020
Copy link
Member

annevk left a comment

Thank you and no need to apologize.

@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from fdaabfc to 648ac39 Jan 14, 2020
@jgraham
Copy link
Contributor

jgraham commented Jan 14, 2020

Building the docs:

Warning, treated as error:
/github/workspace/docs/writing-tests/python-handlers/index.md:48:None:any reference target not found: /tools/third_party/six/six.py
@marmeladema marmeladema force-pushed the marmeladema:wptrunner-python3 branch from 648ac39 to 0b1bb4b Jan 14, 2020
@marmeladema
Copy link
Contributor Author

marmeladema commented Jan 14, 2020

@jgraham i've removed the link in the doc because its not really feasible to include a link that refers to something in tools/third_party/*

@jgraham jgraham merged commit 440014f into web-platform-tests:master Jan 14, 2020
18 of 20 checks passed
18 of 20 checks passed
build-and-publish
Details
build-and-tag
Details
Azure Pipelines Build #20200114.23 failed
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS failed
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (tools/ unittests: Windows Python 3) tools/ unittests: Windows Python 3 succeeded
Details
Azure Pipelines (tools/ unittests: Windows) tools/ unittests: Windows succeeded
Details
Azure Pipelines (tools/ unittests: macOS) tools/ unittests: macOS succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows) tools/wpt/ tests: Windows succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS) tools/wpt/ tests: macOS succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows) tools/wptrunner/ unittests: Windows succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS) tools/wptrunner/ unittests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@marmeladema marmeladema deleted the marmeladema:wptrunner-python3 branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.