Improve test suite handling of paths, temp files #3957

vaneseltine · 2019-08-02T23:31:54Z

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."

vaneseltine · 2019-08-02T23:57:15Z

Ah, I see the url issue in AppVeyor. I'll have to fire up my Windows box.

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

codecov-io · 2019-08-03T01:21:46Z

Codecov Report

Merging #3957 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3957      +/-   ##
==========================================
- Coverage   97.77%   97.75%   -0.03%     
==========================================
  Files          43       43              
  Lines        8763     8763              
  Branches     1375     1375              
==========================================
- Hits         8568     8566       -2     
- Misses         83       84       +1     
- Partials      112      113       +1

Impacted Files	Coverage Δ
aiohttp/web_fileresponse.py	`96.59% <0%> (-1.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e76bd8a...5928b41. Read the comment docs.

vaneseltine · 2019-08-03T01:24:41Z

P.S. I'm not sure what I or the chronographer are doing wrong to set off Timeline Protection, but please let me know if I should do something to fix it.

webknjaz · 2019-08-06T21:59:18Z

@vaneseltine all's fine with that check now.

tests/test_helpers.py

tests/test_web_urldispatcher.py

tests/test_urldispatch.py

asvetlov · 2019-08-08T08:56:20Z

@webknjaz please merge when ready and you have no objections/notes

patchback · 2024-01-28T20:13:18Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 79fe204 on top of patchback/backports/3.9/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957

Backporting merged PR #3957 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957 upstream/3.9

Now, cherry-pick PR Improve test suite handling of paths, temp files #3957 contents into that branch:
```
$ git cherry-pick -x 79fe204522ecf91e9c1cf1a3547c03f821106a74
```
If it'll yell at you with something like fatal: Commit 79fe204522ecf91e9c1cf1a3547c03f821106a74 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 79fe204522ecf91e9c1cf1a3547c03f821106a74
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Improve test suite handling of paths, temp files #3957 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2024-01-28T20:13:18Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 79fe204 on top of patchback/backports/3.10/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957

Backporting merged PR #3957 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957 upstream/3.10

Now, cherry-pick PR Improve test suite handling of paths, temp files #3957 contents into that branch:
```
$ git cherry-pick -x 79fe204522ecf91e9c1cf1a3547c03f821106a74
```
If it'll yell at you with something like fatal: Commit 79fe204522ecf91e9c1cf1a3547c03f821106a74 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 79fe204522ecf91e9c1cf1a3547c03f821106a74
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Improve test suite handling of paths, temp files #3957 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/79fe204522ecf91e9c1cf1a3547c03f821106a74/pr-3957

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* 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)

…hs, temp files (#8083) **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 - [X] I think the code is well written - [X] Unit tests for the changes exist - [X] Documentation reflects the changes - [X] 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. - [X] 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." Co-authored-by: Matt VanEseltine <matvan@umich.edu>

…s, temp files (#8084) **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 - [X] I think the code is well written - [X] Unit tests for the changes exist - [X] Documentation reflects the changes - [X] 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. - [X] 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." --------- Co-authored-by: Matt VanEseltine <matvan@umich.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

vaneseltine requested review from asvetlov and webknjaz as code owners August 2, 2019 23:31

vaneseltine force-pushed the update_test_suite_path_handling branch from fdff4e7 to bd2f13f Compare August 3, 2019 00:59

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 3, 2019