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

Replace all tmpdir fixtures with tmp_path (#3551) #3955

Merged
merged 1 commit into from Aug 2, 2019

Conversation

vaneseltine
Copy link
Contributor

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of tmpdir.join(foo) to tmp_path / foo.

This is intended to comprehensively address and close #3551, and
should have no side effects. This does not affect end users.

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address aio-libs#3551 and should have
no side effects.
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3955      +/-   ##
==========================================
- Coverage   97.77%   97.75%   -0.03%     
==========================================
  Files          43       43              
  Lines        8761     8761              
  Branches     1375     1375              
==========================================
- Hits         8566     8564       -2     
- Misses         83       84       +1     
- Partials      112      113       +1
Impacted Files Coverage Δ
aiohttp/web_fileresponse.py 96.55% <0%> (-1.15%) ⬇️

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 2c38ce2...de85b78. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3955      +/-   ##
==========================================
- Coverage   97.77%   97.75%   -0.03%     
==========================================
  Files          43       43              
  Lines        8761     8761              
  Branches     1375     1375              
==========================================
- Hits         8566     8564       -2     
- Misses         83       84       +1     
- Partials      112      113       +1
Impacted Files Coverage Δ
aiohttp/web_fileresponse.py 96.55% <0%> (-1.15%) ⬇️

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 2c38ce2...de85b78. Read the comment docs.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Thanks!

@asvetlov asvetlov merged commit 8960063 into aio-libs:master Aug 2, 2019
@@ -802,7 +802,7 @@ def test_static_route_path_existence_check() -> None:
tr = request.transport
sock = tr.get_extra_info('socket')
sock.setsockopt(socket.SOL_SOCKET, socket.SO_SNDBUF, 1024)
ret = web.FileResponse(pathlib.Path(str(tmpdir.join(filename))))
ret = web.FileResponse(pathlib.Path(str((tmp_path / filename))))
Copy link
Contributor

Choose a reason for hiding this comment

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

here and in a few other places now the path is turned into string and than back to Path.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've missed this.
Please feel free to make a PR for the fix.
The problem is minor though if all lines belong to tests only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm working on a PR now to more comprehensively address path handling across the test suite.

Mostly that's replacing os.path calls with their appropriate pathlib.Path counterparts.

There are also a couple of uses of ad-hoc temporary directories that I'm updating to tmp_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironically, to support 3.5 some pathlib.Path(str(tmp_path))) turns out to be necessary.

pytest uses pathlib2 to provide tmp_path for Python 3.5, which is mostly fine but it does create a couple of corner cases of incompatibility where pathlib2.Path fails isinstance checks that demand str or PurePath. See pytest-dev/pytest/issues/5017

@vaneseltine vaneseltine deleted the update_to_tmp_path branch August 2, 2019 12:39
vaneseltine added a commit to vaneseltine/aiohttp that referenced this pull request Aug 2, 2019
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`.

Note on `test_static_route_user_home`:

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

Note 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.
@vaneseltine vaneseltine restored the update_to_tmp_path branch August 2, 2019 16:42
@vaneseltine vaneseltine deleted the update_to_tmp_path branch August 2, 2019 16:42
vaneseltine added a commit to vaneseltine/aiohttp that referenced this pull request Aug 2, 2019
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`.

Note on `test_static_route_user_home`:

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

Note 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.
vaneseltine added a commit to vaneseltine/aiohttp that referenced this pull request Aug 2, 2019
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.
vaneseltine added a commit to vaneseltine/aiohttp that referenced this pull request Aug 2, 2019
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
vaneseltine added a commit to vaneseltine/aiohttp that referenced this pull request Aug 3, 2019
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
webknjaz pushed a commit that referenced this pull request Aug 8, 2019
* 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
@webknjaz webknjaz added backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot labels Jan 28, 2024
Copy link
Contributor

patchback bot commented Jan 28, 2024

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

❌ Failed to cleanly apply 8960063 on top of patchback/backports/3.10/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955

Backporting merged PR #3955 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. 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
  3. 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/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955 upstream/3.10
  4. Now, cherry-pick PR Replace all tmpdir fixtures with tmp_path (#3551) #3955 contents into that branch:
    $ git cherry-pick -x 8960063ef4137d6c547a687a45ed55b943e9b8d1
    If it'll yell at you with something like fatal: Commit 8960063ef4137d6c547a687a45ed55b943e9b8d1 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 8960063ef4137d6c547a687a45ed55b943e9b8d1
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Replace all tmpdir fixtures with tmp_path (#3551) #3955 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.10/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955
  7. 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.

Copy link
Contributor

patchback bot commented Jan 28, 2024

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

❌ Failed to cleanly apply 8960063 on top of patchback/backports/3.9/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955

Backporting merged PR #3955 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. 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
  3. 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/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955 upstream/3.9
  4. Now, cherry-pick PR Replace all tmpdir fixtures with tmp_path (#3551) #3955 contents into that branch:
    $ git cherry-pick -x 8960063ef4137d6c547a687a45ed55b943e9b8d1
    If it'll yell at you with something like fatal: Commit 8960063ef4137d6c547a687a45ed55b943e9b8d1 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 8960063ef4137d6c547a687a45ed55b943e9b8d1
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Replace all tmpdir fixtures with tmp_path (#3551) #3955 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.9/8960063ef4137d6c547a687a45ed55b943e9b8d1/pr-3955
  7. 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.

@psf-chronographer psf-chronographer bot added bot:chronographer:provided There is a change note present in this PR labels Jan 28, 2024
webknjaz pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
)

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address aio-libs#3551 and should have
no side effects.

(cherry picked from commit 8960063)
webknjaz pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
)

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address aio-libs#3551 and should have
no side effects.

(cherry picked from commit 8960063)
webknjaz pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
)

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address aio-libs#3551 and should have
no side effects.

(cherry picked from commit 8960063)
webknjaz pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
)

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address aio-libs#3551 and should have
no side effects.

(cherry picked from commit 8960063)
webknjaz added a commit that referenced this pull request Jan 28, 2024
…mp_path (#3551) (#8075)

**This is a backport of PR #3955 as merged into master
(8960063).**

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address and close #3551, and
should have no side effects. This does not affect end users.

Co-authored-by: Matt VanEseltine <vaneseltine@gmail.com>
webknjaz added a commit that referenced this pull request Jan 28, 2024
…p_path (#3551) (#8076)

**This is a backport of PR #3955 as merged into master
(8960063).**

tmp_path is the replacement fixture in pytest for tmpdir; tmp_path
uses the builtin pathlib.Path class. As it says on the tin, this
commit replaces every instance of tmpdir in the test suite with
tmp_path. Aside from s/tmpdir/tmp_path/ this also required changing
instances of `tmpdir.join(foo)` to `tmp_path / foo`.

This is intended to comprehensively address and close #3551, and
should have no side effects. This does not affect end users.

Co-authored-by: Matt VanEseltine <vaneseltine@gmail.com>
webknjaz pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
* 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 pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
* 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 pushed a commit to webknjaz/aiohttp that referenced this pull request Jan 28, 2024
* 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 added a commit that referenced this pull request Jan 28, 2024
…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)

<!-- Thank you for your contribution! -->

## 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 &lt;Name&gt; &lt;Surname&gt;.
  * 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>
webknjaz added a commit that referenced this pull request Jan 28, 2024
…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)

<!-- Thank you for your contribution! -->

## 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 &lt;Name&gt; &lt;Surname&gt;.
  * 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot 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.

Replace tmpdir fixture with tmp_path in tests
5 participants