Improve handling empty segments in urls according to RFC3986

[commonism]

What do these changes do?

The changes honor empty segments when joining urls e.g.

URL("https://web.archive.org/web/") / "https://github.com/"

Picking up the PR #1023 to get this merged.

Are there changes in behavior for the user?

The changes to URL.join and URL.joinpath do not strip empty segments any longer.

Related issue number

Fixes #984 Fixes #926

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [x] Documentation reflects the changes

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 76.59574% with 11 lines in your changes missing coverage. Please review.

Project coverage is 62.06%. Comparing base (0baa61a) to head (cc1b9fa). Report is 145 commits behind head on master.

Files with missing lines	Patch %	Lines
tests/test_url.py	70.96%	9 Missing :warning:
yarl/_url.py	87.50%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
+ Coverage   61.96%   62.06%   +0.09%     
==========================================
  Files          38       38              
  Lines        6510     6550      +40     
  Branches      353      352       -1     
==========================================
+ Hits         4034     4065      +31     
- Misses       2448     2457       +9     
  Partials       28       28              

Flag	Coverage Δ
CI-GHA	62.03% <76.59%> (+0.09%)	:arrow_up:
MyPy	25.01% <46.80%> (+0.36%)	:arrow_up:
OS-Linux	99.24% <100.00%> (+<0.01%)	:arrow_up:
OS-Windows	99.57% <100.00%> (+<0.01%)	:arrow_up:
OS-macOS	99.00% <100.00%> (+<0.01%)	:arrow_up:
Py-3.10.11	98.91% <100.00%> (+<0.01%)	:arrow_up:
Py-3.10.14	99.12% <100.00%> (+<0.01%)	:arrow_up:
Py-3.11.9	99.12% <100.00%> (+<0.01%)	:arrow_up:
Py-3.12.4	99.12% <100.00%> (+<0.01%)	:arrow_up:
Py-3.8.10	98.85% <100.00%> (+<0.01%)	:arrow_up:
Py-3.8.18	99.06% <100.00%> (+<0.01%)	:arrow_up:
Py-3.9.13	98.85% <100.00%> (+<0.01%)	:arrow_up:
Py-3.9.19	99.06% <100.00%> (+<0.01%)	:arrow_up:
Py-pypy7.3.11	99.38% <100.00%> (+<0.01%)	:arrow_up:
Py-pypy7.3.16	99.41% <100.00%> (+<0.01%)	:arrow_up:
VM-macos-latest	99.00% <100.00%> (+<0.01%)	:arrow_up:
VM-ubuntu-latest	99.24% <100.00%> (+<0.01%)	:arrow_up:
VM-windows-latest	99.57% <100.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[commonism]

@Dreamsorcerer can you help me figure out the issue with the codecov/ checks?

I can see it ran the unit tests, but failed to create proper coverage files? e.g. https://app.codecov.io/gh/aio-libs/yarl/pull/1026/blob/tests/test_url.py#L859

[Dreamsorcerer]

@Dreamsorcerer can you help me figure out the issue with the codecov/ checks?

I can see it ran the unit tests, but failed to create proper coverage files? e.g. https://app.codecov.io/gh/aio-libs/yarl/pull/1026/blob/tests/test_url.py#L859

I'd change the fail option to true, then it'll be obvious if any uploads have failed (and which jobs need to be rerun): https://github.com/aio-libs/yarl/blob/master/.github/workflows/ci-cd.yml#L340

[commonism]

I'd change the fail option to true, then it'll be obvious if any uploads have failed (and which jobs need to be rerun): https://github.com/aio-libs/yarl/blob/master/.github/workflows/ci-cd.yml#L340

It's not an upload failing. Test (3.12, Y, ubuntu-latest, false) All tests ran, upload succeeded.

Yet the only tests which are run evaluated are the tests which have a annotation to return None def test_authority_full()

You can see test_url.py and tests/test_url.py exist. https://app.codecov.io/gh/aio-libs/yarl/pull/1026/tree

Same issue: https://app.codecov.io/gh/aio-libs/yarl/pull/1025/tree Same issue: https://app.codecov.io/gh/aio-libs/yarl/pull/1023/tree Same issue: https://app.codecov.io/gh/aio-libs/yarl/pull/1019/tree Okay: https://app.codecov.io/gh/aio-libs/yarl/pull/1018/tree Okay: https://app.codecov.io/gh/aio-libs/yarl/pull/1016/tree

Same problem: https://app.codecov.io/gh/aio-libs/yarl/pull/1015/blob/tests/test_url.py

[Dreamsorcerer]

Yet the only tests which are run evaluated are the tests which have a annotation to return None

Oh, that's because webknjaz added mypy coverage to the results. I think that's just confusing...

If you'd like to improve the situation, then you can add tests/ to coverage in pytest.ini and look at enabling the options we use elsewhere in .mypy.ini while fixing up the errors it highlights (maybe add a couple of options in each PR). You can use aiohttp-jinja2 as a reference: https://github.com/aio-libs/aiohttp-jinja2

[commonism]

Yet the only tests which are run evaluated are the tests which have a annotation to return None

Oh, that's because webknjaz added mypy coverage to the results. I think that's just confusing...

MyPy has it's own flag, the flag I chose here is Py-3.12.4, still showing the mypy results? For Base the file is missing.

May be better to have somebody look into this who has an idea how this is supposed to work. Currently it's broken for every PR.

[Dreamsorcerer]

Yeah, but it's clearly mypy coverage. See above if you want to improve either coverage.

[commonism]

To my understanding …

The coverage reported by mypy is "tests/test_url.py", the coverage reported by python 3.12.4 is for "test_url.py".

The reported codecov numbers are combined values for unit testing and type checking. The low value report is due to the mypy/typing use and does not reflect unit testing code coverage.

Due to a large number of single file posts, codecov may rate limit (seen in the scheduled CI runs), I guess this could be prevented by limiting the number of requests, combining all reports in a single upload.

[Dreamsorcerer]

Oh, wait, I see what you mean. Codecov wouldn't load anything last time I looked. So, it looks like changing from codecov v3 to v4 resulted in the file paths changing for some reason...

[Dreamsorcerer]

Well, copying the config from aiohttp-jinja2, as I suggested, resolves the path, but now the yarl/ coverage seems to have disappeared: #1028.

[webknjaz]

@Dreamsorcerer there's a few points to check:

• what gets into codecov — "uploads" at https://app.codecov.io/gh/aio-libs/yarl/commit/7bca6cf1ba3673018b9065c6102f791a0a7516c0/tree?flags%5B0%5D=Py-3.12.4 has https://api.codecov.io/upload/gh/aio-libs/yarl/download?path=v4/raw/2024-06-14/F2143C32CD85E0BC827E552A5E512211/7bca6cf1ba3673018b9065c6102f791a0a7516c0/3eded8ad-f926-4cb5-b6a6-5cbce8a67443/48384b50-8891-45cb-8a42-87d70c33fd3e.txt which reveals what the Codecov CLI uploader sends to their server — it does only contain base filenames in the XML part, which is surprising
• then, I'd check what coverage.py produces in coverage.xml (it should be the same that it's in the payload
• if it's the fault of the coverage library and not Codecov, I'd try older versions — we've been attempting to update it recently in Ansible and hit some regressions related to forked subprocesses, for example

It is a good idea to bisect Codecov @ https://app.codecov.io/gh/aio-libs/yarl/commits.

Looks like https://app.codecov.io/gh/aio-libs/yarl/commit/60737a8bf57625cffb09674b788ab2f64320a448 is the last commit with proper paths and the following https://app.codecov.io/gh/aio-libs/yarl/commit/e74c1599d47e21523d3e78ad78966d9630b12cc0 is broken.

So it looks like https://github.com/aio-libs/yarl/pull/1007 might've influenced it, which could only come from the MyPy bump there or unpinned deps.

Comparing the last working (https://api.codecov.io/upload/gh/aio-libs/yarl/download?path=v4/raw/2024-06-06/F2143C32CD85E0BC827E552A5E512211/60737a8bf57625cffb09674b788ab2f64320a448/2bd0dbd5-af59-44bd-8e2b-479f8d54ef00.txt) and the first broken (https://api.codecov.io/upload/gh/aio-libs/yarl/download?path=v4/raw/2024-06-06/F2143C32CD85E0BC827E552A5E512211/e74c1599d47e21523d3e78ad78966d9630b12cc0/6a695c6c-3879-48c0-bf3d-62e2d036817a/d9f3f16e-284b-43ce-a191-ce045b11982f.txt) uploads (both from Py-3.11.9 @ ubuntu-latest), the first thing I noticed is that the file list before the XML chunk that was present earlier, disappeared. The second thing is that the older working upload had a # path=./coverage.xml comment and the newer one had # path=/home/runner/work/yarl/yarl/coverage.xml. Both have <coverage version="7.5.3" which means that the coverage.py version hasn't changed in between.

This points to some env changes, which might be versions of pytest/pytest-cov/codecov-action/codecov-cli etc.

[webknjaz]

@Dreamsorcerer you could also try producing an html report locally to see if coverage.py behaves well stand-alone.

[webknjaz]

Oh... There's actually more commits in between 60737a8bf57625cffb09674b788ab2f64320a448 and e74c1599d47e21523d3e78ad78966d9630b12cc0: https://github.com/aio-libs/yarl/commits/e74c1599d47e21523d3e78ad78966d9630b12cc0/.

And among those, there's a series of action bumps, which probably didn't have a chance to make uploads, being cancelled by newer merges starting more CI jobs. I compared our two commits' logs and the confirmed my suspicion:

- Download action repository 'codecov/[email protected]' (SHA:eaaf4bedf32dbdc6b720b63067d99c4d77d6047d)
+ Download action repository 'codecov/codecov-action@v4' (SHA:125fc84a9a348dbcf27191600683ec096ec9021c)

(https://github.com/aio-libs/yarl/actions/runs/9401318235/job/25893141828#step:1:41 vs. https://github.com/aio-libs/yarl/actions/runs/9405561693/job/25907242226#step:1:41)

With that in mind, my new suspect is... :drum: :long_drum: :drum: ... https://github.com/aio-libs/yarl/pull/988!

[webknjaz]

Good news is that if you're persistent enough, it's possible to get them to merge your bugfix: https://github.com/codecov/uploader/pulls?q=sort%3Aupdated-desc+is%3Apr+author%3Awebknjaz+is%3Amerged / https://github.com/codecov/codecov-cli/pulls?q=sort%3Aupdated-desc+is%3Apr+is%3Amerged+author%3Awebknjaz. (at least two of those are fixes for “innocent” refactoring, by the way, hence https://github.com/orgs/aio-libs/discussions/36)

[webknjaz]

To conclude, the current workaround for viewing the coverage would be make cov; python -Im webbrowser htmlcov/index.html.

[commonism]

Tried v3.1.4 - https://github.com/aio-libs/yarl/actions/runs/9691011881/job/26741939301 Can't say anything about the results as uploading fails due to rate limiting - https://github.com/codecov/codecov-action/issues/1293 may be related.

[webknjaz]

Can't say anything about the results as uploading fails due to rate limiting - codecov/codecov-action#1293 may be related.

I ended up hardcoding the token in pytest the other day (https://github.com/pytest-dev/pytest/pull/12516), to fight this problem. I think, we should do it everywhere for now.

[webknjaz]

By the way, another configuration problem is that passing the token was never implemented for the linters job: https://github.com/aio-libs/yarl/pull/988/files#r1631478435 — I haven't yet addressed that.

[webknjaz]

I've added the tokens and made a few updates. The root cause of the file paths mismatch is still a mystery. Will have to see if the codecov config presence has something to do with it.

[webknjaz]

Could you move the Codecov action update to a separate PR? Also, the change notes are RST so it would be nice to use that markup. The RTD preview might be broken if you don't. It's a blocker, FYI.

[commonism]

Updated accordingly. Where is the RTD preview located?

[webknjaz]

@commonism scroll down to the CI checks in this PR, and you'll see the status from Read The Docs. Click on the Details link. When it's red or in progress, it'll take you to the build log page. But once it succeeds, it leads to the preview.

[commonism]

Coverage looks better when looking into the Actions results. https://github.com/aio-libs/yarl/actions/runs/9738655009

[webknjaz]

Coverage looks better when looking into the Actions results.

Yes, I integrated putting that into the job summary directly from whatever coverage.py collected. There's no codecov integration at that point, so it doesn't stand in the way in that specific place.