-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add test coverage for handling port 0 #1182
Conversation
This was wrong in previous yarl releases, but fixed by one of the cleanups in 1.14.0. Add coverage to make sure it continutes to work as 0 is not a default port
This was wrong in previous yarl releases, but fixed by one of the cleanups in 1.14.0. Add coverage to make sure it continutes to work as 0 is not a default port
Isn't 0 only used as an ephemeral port that is resolved to something else by the kernel? |
What was the fix, by the way? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1182 +/- ##
=======================================
Coverage 95.57% 95.58%
=======================================
Files 25 25
Lines 4864 4871 +7
Branches 443 443
=======================================
+ Hits 4649 4656 +7
Misses 189 189
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I don't expect anyone is using it in production, and this likely only affects downstream tests |
I need to go back and figure out which PR fixed it. Its likely a |
I was under impression that using this port in URLs isn't even legal since it doesn't have defined semantics, I think 🤔 |
urllib seems to allow it and it does specifically check the port is between 0 and 65535
|
Ack. Though, I wonder what the use-case for this is. |
I'm not sure there is one. I only noticed because of downstream tests, but none of them actually use 0 in production. |
It looks like it originally regressed in https://github.com/aio-libs/yarl/releases/tag/v1.9.5. I'm guessing #1033 |
Perhaps it's useful in the context of non-HTTP URLs. Like those used with custom handlers in mobile apps and so on.. |
I'm sure someone is using it for something somewhere so its good to have coverage we don't regress it again. |
What do these changes do?
This was wrong in previous yarl releases, but fixed by one of the cleanups in 1.14.0. Add coverage to make sure it continues to work as 0 is not a default port
The problem:
str(URL("http://example.com:0/"))
should behttp://example.com:0/
because0
is not the default port forhttp
yarl 1.9.5 - 1.13.1 produced it as
http://example.com/