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

Remove use of six.integer_types/string_types/text_type #28789

Merged
merged 1 commit into from May 10, 2021
Merged

Conversation

@foolip
Copy link
Member

@foolip foolip commented May 3, 2021

These are constants corresponding to just int/str/str in Python 3:
https://six.readthedocs.io/#constants

This is simple search-replace with the exception of
resources/test/conftest.py, where element.text is already a string. If
it were None or bytes, then JSON parsing would fail anyway.

In service-workers/cache-storage/resources/vary.py, the reason why
str(cookie_vary) works was non-obvious. Correct the documentation in
tools/wptserve/wptserve/request.py to show that the cookie_vary value
would be a CookieValue, not a binary string.

Since no use of six remains in tools/wptserve/, it's removed as a
dependency in wptserve's setup.py.

Part of #28776.

@jgraham
Copy link
Contributor

@jgraham jgraham commented May 5, 2021

I was about to ask if you understood the WebDriver failures, but now you pushed a new version (and of course there's no good interdiff) :)

tools/wptserve/setup.py Show resolved Hide resolved
webdriver/tests/new_session/response.py Show resolved Hide resolved
@foolip
Copy link
Member Author

@foolip foolip commented May 5, 2021

@jgraham I didn't change anything, I just wanted to rebase on top of other six changes to ensure no new lint failures. I just commented on part of the webdriver diffs.

Overall, any times I've touched webdriver/ recently the claimed differences have been large. There seems to be a lot of flakiness, have you seen the same? To be expected?

@foolip
Copy link
Member Author

@foolip foolip commented May 5, 2021

Understanding the CI failures. wpt-chrome-dev-stability and wpt-firefox-nightly-stability timed out in repetition 5 / 10 because too many tests (171) are affected.

There are a lot of apparent changes in the webdriver test results, but there could be flakiness. Combining the two set of before/after runs to look for consistent regressions:
https://wpt.fyi/results/webdriver/tests?q=seq%28status%3Apass%20status%3A%21pass%20status%3Apass%20status%3A%21pass%29&run_id=5768588818382848&run_id=5719745376550912&run_id=5076726092660736&run_id=5716229543165952

Yes, I did break stuff! "FAIL message: NameError: name 'six' is not defined"

I'll iterate until there are no unexplained regressions and the request review.

@foolip
Copy link
Member Author

@foolip foolip commented May 5, 2021

Care must be taken when merging this and #28757. Both change webdriver/tests/support/asserts.py, and the second to land also needs to remove the import six statement. But I don't know which will land first yet.

These are constants corresponding to just int/str/str in Python 3:
https://six.readthedocs.io/#constants

This is simple search-replace with the exception of
resources/test/conftest.py, where element.text is already a string. If
it were None or bytes, then JSON parsing would fail anyway.

In service-workers/cache-storage/resources/vary.py, the reason why
str(cookie_vary) works was non-obvious. Correct the documentation in
tools/wptserve/wptserve/request.py to show that the cookie_vary value
would be a CookieValue, not a binary string.

Since no use of six remains in tools/wptserve/, it's removed as a
dependency in wptserve's setup.py.

Part of #28776.
@foolip foolip force-pushed the foolip/six_constants branch from ff350f9 to 89be305 May 6, 2021
@foolip
Copy link
Member Author

@foolip foolip commented May 6, 2021

OK, this has now been rebased and the issue in #28789 (comment) sorted. Ready for final review.

@foolip foolip merged commit 3d43b6e into master May 10, 2021
38 of 41 checks passed
@foolip foolip deleted the foolip/six_constants branch May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment