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

Split up Windows tests relying on urlunparse behaviour #12788

Merged

Conversation

matthewhughes934
Copy link
Contributor

@matthewhughes934 matthewhughes934 commented Jun 23, 2024

There was a behavioural change to urllib.parse.urlunparse[1] that
affects some of our tests on Windows. With the understanding that the
new behaviour is indeed desired, split up some tests relying on this
behaviour depending on the version of Python.

The sample URL used to check this behaviour was taken from a test in the
upstream change (with the new behaviour this URL will round-trip
parsing)

[1] python/cpython#113563

@matthewhughes934
Copy link
Contributor Author

matthewhughes934 commented Jun 23, 2024

Ah, there's another case I missed. Will fix that up

EDIT: fixed and squashed into the single commit

@matthewhughes934 matthewhughes934 force-pushed the fix-failing-tests-cpython-behaviour branch 3 times, most recently from 042d655 to ebdd028 Compare June 23, 2024 17:50
@matthewhughes934 matthewhughes934 marked this pull request as ready for review June 23, 2024 18:14
@ichard26 ichard26 requested a review from uranusjr June 23, 2024 19:35
Comment on lines 1379 to 1387
# versions containing fix/backport from https://github.com/python/cpython/pull/113563
# which changed the behavior of `urllib.parse.urlun{parse,split}`
has_new_urlun_behavior = (
# https://github.com/python/cpython/commit/387ff96e95b9f8a8cc7e646523ba3175b1350669
(sys.version_info[:2] == (3, 12) and sys.version_info >= (3, 12, 4))
or
# https://github.com/python/cpython/commit/872000606271c52d989e53fe4cc9904343d81855
(sys.version_info[:2] == (3, 13) and sys.version_info >= (3, 13, 0, "beta", 2))
)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of trying to check versions, would it be better if we just try to parse one URL to determine the version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of trying to check versions, would it be better if we just try to parse one URL to determine the version?

Good idea, that will save us having to update these versions. Trying that with f9e5477

There was a behavioural change to `urllib.parse.urlunparse`[1] that
affects some of our tests on Windows. With the understanding that the
new behaviour is indeed desired, split up some tests relying on this
behaviour depending on the version of Python.

The sample URL used to check this behaviour was taken from a test in the
upstream change (with the new behaviour this URL will round-trip
parsing)

[1] python/cpython#113563
@matthewhughes934 matthewhughes934 force-pushed the fix-failing-tests-cpython-behaviour branch from f9e5477 to 16f0617 Compare June 24, 2024 11:52
@ichard26
Copy link
Member

Windows is not really my domain anymore, but I just wanted to say thank you for helping us unbreak CI. It's been 🔴 a lot in the past few months, and getting it to stay 🍏 has been a real chore, so thank you :)

@pradyunsg pradyunsg merged commit 5c389ec into pypa:main Jun 25, 2024
28 checks passed
@matthewhughes934 matthewhughes934 deleted the fix-failing-tests-cpython-behaviour branch June 25, 2024 16:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants