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

[PR #3957/79fe2045 backport][3.9] Improve test suite handling of paths, temp files #8084

Conversation

webknjaz
Copy link
Member

This is a backport of PR #3957 as merged into master (79fe204).

  • Improve test suite handling of paths, temp files

This updates most uses of os.path to instead use pathlib.Path. Relatedly, and following up from #3955 (which replaced pytest's tmpdir fixture with tmp_path), this removes most ad-hoc tempfile creation in favor of the tmp_path fixture. Following conversion, unnecessary os and tempfile imports were removed.

Most pathlib changes involve straightforward changes from os functions such as os.mkdir or os.path.abspath to their equivalent methods in pathlib.Path.

Changing ad-hoc temporary path to tmp_path involved removing the tmp_dir_path fixture and replacing its functionality with tmp_path in test_save_load and test_guess_filename_with_tempfile.

On test_static_route_user_home function:

  • I think that the intention of this test is to ensure that aiohttp correctly expands the home path if passed in a string. I refactored it to pathlib.Path and cut out duplication of relative_to() calls. But if it's not doing anything but expanding ~, then it's testing the functionality of pathlib.Path, not aiohttp.

On unix_sockname fixture:

This fixture uses tempfile.TemporaryDirectory. Because it's a somewhat complicated fixture used across multiple test modules, I left it as-is for now.

On str(tmp_path) and even pathlib.Path(str(tmp_path)):

pytest uses pathlib2 to provide tmp_path for Python 3.5 (only). This is mostly fine but it fails on a couple of corner cases, such as os.symlink() which blocks all but str and PurePath via isinstance type checking. In several cases, this requires conversion to string or conversion to string and then into pathlib.Path to maintain code compatibility. See: pytest-dev/pytest/issues/5017

  • Correct test_guess_filename to use file object

  • Update symlink in tests; more guess_filename tests

(cherry picked from commit 79fe204)

What do these changes do?

This updates most uses of os.path to instead use pathlib.Path.
Relatedly, and following up from #3955 (which replaced pytest's tmpdir
fixture with tmp_path), this removes most ad-hoc tempfile creation in
favor of the tmp_path fixture. Following conversion, unnecessary os
and tempfile imports were removed.

Most pathlib changes involve straightforward changes from os functions
such as os.mkdir or os.path.abspath to their equivalent methods in
pathlib.Path.

Changing ad-hoc temporary path to tmp_path involved removing the
tmp_dir_path fixture and replacing its functionality with tmp_path
in test_save_load and test_guess_filename_with_tempfile.

On test_static_route_user_home function:

  • I think that the intention of this test is to ensure that aiohttp
    correctly expands the home path if passed in a string. I refactored it
    to pathlib.Path and cut out duplication of relative_to() calls.
    But if it's not doing anything but expanding ~, then it's testing the
    functionality of pathlib.Path, not aiohttp.

On unix_sockname fixture:

This fixture uses tempfile.TemporaryDirectory. Because it's a somewhat
complicated fixture used across multiple test modules, I left it as-is
for now.

On str(tmp_path) and even pathlib.Path(str(tmp_path)):

pytest uses pathlib2 to provide tmp_path for Python 3.5 (only).
This is mostly fine but it fails on a couple of corner cases, such as
os.symlink() which blocks all but str and PurePath via isinstance
type checking. In several cases, this requires conversion to string or
conversion to string and then into pathlib.Path to maintain code
compatibility. See: pytest-dev/pytest/issues/5017

Are there changes in behavior for the user?

These changes only affect the test suite and have no impact on the end user.

Related issue number

This is intended to address discussion following the simplistic changes from tmpdir to tmp_path of #3955.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

* Improve test suite handling of paths, temp files

This updates most uses of `os.path` to instead use `pathlib.Path`.
Relatedly, and following up from aio-libs#3955 (which replaced pytest's `tmpdir`
fixture with `tmp_path`), this removes most ad-hoc tempfile creation in
favor of the `tmp_path` fixture. Following conversion, unnecessary `os`
and `tempfile` imports were removed.

Most pathlib changes involve straightforward changes from `os` functions
such as `os.mkdir` or `os.path.abspath` to their equivalent methods in
`pathlib.Path`.

Changing ad-hoc temporary path to `tmp_path` involved removing the
`tmp_dir_path` fixture and replacing its functionality with `tmp_path`
in `test_save_load` and `test_guess_filename_with_tempfile`.

On `test_static_route_user_home` function:

* I think that the intention of this test is to ensure that aiohttp
correctly expands the home path if passed in a string. I refactored it
to `pathlib.Path` and cut out duplication of `relative_to()` calls.
But if it's not doing anything but expanding `~`, then it's testing the
functionality of `pathlib.Path`, not aiohttp.

On `unix_sockname` fixture:

This fixture uses `tempfile.TemporaryDirectory`. Because it's a somewhat
complicated fixture used across multiple test modules, I left it as-is
for now.

On `str(tmp_path)` and even `pathlib.Path(str(tmp_path))`:

pytest uses `pathlib2` to provide `tmp_path` for Python 3.5 (only).
This is mostly fine but it fails on a couple of corner cases, such as
`os.symlink()` which blocks all but `str` and `PurePath` via isinstance
type checking. In several cases, this requires conversion to string or
conversion to string and then into `pathlib.Path` to maintain code
compatibility. See: pytest-dev/pytest/issues/5017

* Correct test_guess_filename to use file object

* Update symlink in tests; more guess_filename tests

(cherry picked from commit 79fe204)
@webknjaz webknjaz requested a review from asvetlov as a code owner January 28, 2024 21:44
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 28, 2024
@webknjaz webknjaz enabled auto-merge (squash) January 28, 2024 21:45
Copy link

codecov bot commented Jan 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (24a6d64) 97.30% compared to head (a501b4c) 97.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##              3.9    #8084      +/-   ##
==========================================
- Coverage   97.30%   97.30%   -0.01%     
==========================================
  Files         108      108              
  Lines       32632    32620      -12     
  Branches     3872     3871       -1     
==========================================
- Hits        31754    31742      -12     
  Misses        671      671              
  Partials      207      207              
Flag Coverage Δ
CI-GHA 97.21% <100.00%> (-0.01%) ⬇️
OS-Linux 96.91% <100.00%> (-0.01%) ⬇️
OS-Windows 94.41% <99.06%> (-0.01%) ⬇️
OS-macOS 96.72% <100.00%> (+0.19%) ⬆️
Py-3.10.11 94.31% <99.06%> (+<0.01%) ⬆️
Py-3.10.13 96.69% <100.00%> (-0.01%) ⬇️
Py-3.11.7 96.42% <100.00%> (-0.01%) ⬇️
Py-3.12.1 96.50% <100.00%> (+0.03%) ⬆️
Py-3.8.10 94.29% <99.06%> (-0.01%) ⬇️
Py-3.8.18 96.62% <100.00%> (-0.01%) ⬇️
Py-3.9.13 94.30% <99.06%> (+<0.01%) ⬆️
Py-3.9.18 96.68% <100.00%> (-0.01%) ⬇️
Py-pypy7.3.15 96.22% <100.00%> (-0.01%) ⬇️
VM-macos 96.72% <100.00%> (+0.19%) ⬆️
VM-ubuntu 96.91% <100.00%> (-0.01%) ⬇️
VM-windows 94.41% <99.06%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@webknjaz webknjaz merged commit 94462ee into aio-libs:3.9 Jan 28, 2024
27 of 30 checks passed
@webknjaz webknjaz deleted the patchback/backports/3.9/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957 branch April 19, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants