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

Pathlib compat fails under Python 3.5 #5017

Closed
khaeru opened this issue Apr 1, 2019 · 6 comments
Closed

Pathlib compat fails under Python 3.5 #5017

khaeru opened this issue Apr 1, 2019 · 6 comments
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@khaeru
Copy link

khaeru commented Apr 1, 2019

pytest's compatibility code chooses to use pathlib2 except for Python 3.6. This means that pathlib2.Path objects are returned for fixtures/fixture methods like tmp_path_factory.mktemp(). When the user tests code using Python 3.5's built-in pathlib, the built-in package chokes on the objects from pathlib2.

Put the following in test_foo.py and execute using pytest under Python 3.5:

from pathlib import Path  # available in Python 3.5


def foo(p):
    p2 = p / 'foo.txt'
    print(p2.exists())


def bar(p):
    p2 = Path(p)
    print(p2.exists())


def test_foo(tmp_path_factory):
    foo(tmp_path_factory.mktemp('baz1'))


def test_bar(tmp_path_factory):
    bar(tmp_path_factory.mktemp('baz2'))

…this occurs:

$ pytest test_foo.py
============================================================================== test session starts ===============================================================================
platform linux -- Python 3.5.2, pytest-4.4.0, py-1.5.2, pluggy-0.9.0
Matplotlib: 2.1.2
Freetype: 2.6.1
rootdir: /home/gidden/work/iiasa/message/ixmp, inifile: setup.cfg
plugins: mpl-0.9, cov-2.6.0
collected 2 items
                                                                                                                                                                
test_foo.py .F                                                                                                                                                             [100%]

==================================================================================== FAILURES ====================================================================================
____________________________________________________________________________________ test_bar ____________________________________________________________________________________
tmp_path_factory = TempPathFactory(_given_basetemp=None, _trace=<pluggy._tracing.TagTracerSub object at 0x7f0779c3f1d0>, _basetemp=PosixPath('/tmp/pytest-of-gidden/pytest-22'))
    def test_bar(tmp_path_factory):
    
>       bar(tmp_path_factory.mktemp('baz2'))
test_foo.py:24: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
test_foo.py:13: in bar
    p2 = Path(p)
/usr/lib/python3.5/pathlib.py:969: in __new__
    self = cls._from_parts(args, init=False)
/usr/lib/python3.5/pathlib.py:651: in _from_parts
    drv, root, parts = self._parse_args(args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cls = <class 'pathlib.PosixPath'>, args = (PosixPath('/tmp/pytest-of-gidden/pytest-22/baz20'),)
    @classmethod
    def _parse_args(cls, args):
        # This is useful when you don't want to create an instance, just
        # canonicalize some constructor arguments.
        parts = []
        for a in args:
            if isinstance(a, PurePath):
                parts += a._parts
            elif isinstance(a, str):
                # Force-cast str subclasses to str (issue #21127)
                parts.append(str(a))
            else:
                raise TypeError(
                    "argument should be a path or str object, not %r"
>                   % type(a))
E               TypeError: argument should be a path or str object, not <class 'pathlib2.PosixPath'>
/usr/lib/python3.5/pathlib.py:643: TypeError
======================================================================= 1 failed, 1 passed in 0.13 seconds =======================================================================

The same command succeeds under Python 3.6.

Discovered by @gidden

@nicoddemus
Copy link
Member

Hi @khaeru, thanks for the report.

We decided to use pathlib2 on py<3.6 because the standard pathlib was missing a few methods IIRC. @RonnyPfannschmidt knows the details.

A workaround is to use Path(str(p)) instead of Path(p), but I believe you already know that.

@nicoddemus nicoddemus added the type: question general question, might be closed after 2 weeks of inactivity label Apr 1, 2019
@khaeru
Copy link
Author

khaeru commented Apr 1, 2019

Hi @nicoddemus — yes, that's what we're doing for now.

However, it was somewhat cryptic, so still took us some sleuthing to figure out was going on. Only when the pytest-returned pathlib2 objects hit pathlib code (e.g. "bar" in the snippet above) does this happen; in other cases ("foo") it does not.

For less experienced pytest users, it might help to:

  • explicitly state in the documentation which of pathlib/pathlib2 is used, and/or
  • catch/re-raise with an explanatory message/hint.

@RonnyPfannschmidt
Copy link
Member

Its a unfortunate fact that pathlib pre python 3.6 is a broken mess

@nicoddemus
Copy link
Member

I will see if we can drop pathlib2 on Python 3; I think we can live with the missing bits in Python 3.4 and 3.5.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Apr 2, 2019
@nicoddemus nicoddemus added type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature and removed type: question general question, might be closed after 2 weeks of inactivity labels May 1, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 5, 2019
@nicoddemus
Copy link
Member

Hi @khaeru,

We have decided that reverting to standard pathlib in Python 3.5 will be worse overall because it will break many test suites due to the API change in the tmp_path fixture.

We have updated the docs for some time now explaining pathlib2 objects are returned in Python 3.5.

Thanks again. 👍

@khaeru
Copy link
Author

khaeru commented Jun 19, 2019

Hi @nicoddemus —thanks for the valiant attempt to overcome that upstream brokenness!

Hopefully anyone who encounters the same problem can stumble upon this issue and learn that they might need to explicitly cast to pathlib.Path or pathlib2.Path on 3.5.

billbrod added a commit to LabForComputationalVision/plenoptic that referenced this issue Jul 11, 2019
need to wrap pytest's tmp_dir in pathlib.Path for python 3.5:
pytest-dev/pytest#5017
vaneseltine added a commit to vaneseltine/aiohttp that referenced this issue 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 issue 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 to aio-libs/aiohttp that referenced this issue 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 pushed a commit to webknjaz/aiohttp that referenced this issue 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 issue 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 issue 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 to aio-libs/aiohttp that referenced this issue 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 to aio-libs/aiohttp that referenced this issue 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
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants