[PR #3957/79fe2045 backport][3.9] Improve test suite handling of path…

…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>

CHANGES/3957.misc

@@ -0,0 +1 @@
+Improve test suite handling of paths and temp files to consistently use pathlib and pytest fixtures.

tests/test_client_request.py

@@ -1,7 +1,7 @@
 import asyncio
 import hashlib
 import io
-import os.path
+import pathlib
 import urllib.parse
 import zlib
 from http.cookies import BaseCookie, Morsel, SimpleCookie
@@ -921,12 +921,11 @@ async def test_chunked_transfer_encoding(loop, conn) -> None:
 
 
 async def test_file_upload_not_chunked(loop) -> None:
-    here = os.path.dirname(__file__)
-    fname = os.path.join(here, "aiohttp.png")
-    with open(fname, "rb") as f:
+    file_path = pathlib.Path(__file__).parent / "aiohttp.png"
+    with file_path.open("rb") as f:
         req = ClientRequest("post", URL("http://python.org/"), data=f, loop=loop)
         assert not req.chunked
-        assert req.headers["CONTENT-LENGTH"] == str(os.path.getsize(fname))
+        assert req.headers["CONTENT-LENGTH"] == str(file_path.stat().st_size)
         await req.close()
 
 
@@ -947,19 +946,17 @@ async def test_precompressed_data_stays_intact(loop) -> None:
 
 
 async def test_file_upload_not_chunked_seek(loop) -> None:
-    here = os.path.dirname(__file__)
-    fname = os.path.join(here, "aiohttp.png")
-    with open(fname, "rb") as f:
+    file_path = pathlib.Path(__file__).parent / "aiohttp.png"
+    with file_path.open("rb") as f:
         f.seek(100)
         req = ClientRequest("post", URL("http://python.org/"), data=f, loop=loop)
-        assert req.headers["CONTENT-LENGTH"] == str(os.path.getsize(fname) - 100)
+        assert req.headers["CONTENT-LENGTH"] == str(file_path.stat().st_size - 100)
         await req.close()
 
 
 async def test_file_upload_force_chunked(loop) -> None:
-    here = os.path.dirname(__file__)
-    fname = os.path.join(here, "aiohttp.png")
-    with open(fname, "rb") as f:
+    file_path = pathlib.Path(__file__).parent / "aiohttp.png"
+    with file_path.open("rb") as f:
         req = ClientRequest(
             "post", URL("http://python.org/"), data=f, chunked=True, loop=loop
         )

tests/test_cookiejar.py

@@ -1,9 +1,8 @@
 import asyncio
 import datetime
 import itertools
-import os
+import pathlib
 import pickle
-import tempfile
 import unittest
 from http.cookies import BaseCookie, Morsel, SimpleCookie
 from unittest import mock
@@ -200,8 +199,8 @@ async def test_constructor_with_expired(
     assert jar._loop is loop
 
 
-async def test_save_load(loop, cookies_to_send, cookies_to_receive) -> None:
-    file_path = tempfile.mkdtemp() + "/aiohttp.test.cookie"
+async def test_save_load(tmp_path, loop, cookies_to_send, cookies_to_receive) -> None:
+    file_path = pathlib.Path(str(tmp_path)) / "aiohttp.test.cookie"
 
     # export cookie jar
     jar_save = CookieJar(loop=loop)
@@ -215,7 +214,6 @@ async def test_save_load(loop, cookies_to_send, cookies_to_receive) -> None:
     for cookie in jar_load:
         jar_test[cookie.key] = cookie
 
-    os.unlink(file_path)
     assert jar_test == cookies_to_receive
 
 

tests/test_helpers.py

@@ -4,7 +4,6 @@
 import gc
 import platform
 import sys
-import tempfile
 import weakref
 from math import ceil, modf
 from pathlib import Path
@@ -73,11 +72,21 @@ def test_parse_mimetype(mimetype, expected) -> None:
 # ------------------- guess_filename ----------------------------------
 
 
-def test_guess_filename_with_tempfile() -> None:
-    with tempfile.TemporaryFile() as fp:
+def test_guess_filename_with_file_object(tmp_path) -> None:
+    file_path = tmp_path / "test_guess_filename"
+    with file_path.open("w+b") as fp:
         assert helpers.guess_filename(fp, "no-throw") is not None
 
 
+def test_guess_filename_with_path(tmp_path) -> None:
+    file_path = tmp_path / "test_guess_filename"
+    assert helpers.guess_filename(file_path, "no-throw") is not None
+
+
+def test_guess_filename_with_default() -> None:
+    assert helpers.guess_filename(None, "no-throw") == "no-throw"
+
+
 # ------------------- BasicAuth -----------------------------------
 
 

tests/test_multipart.py

@@ -1,6 +1,7 @@
 import asyncio
 import io
 import json
+import pathlib
 import zlib
 from unittest import mock
 
@@ -1270,7 +1271,7 @@ async def test_write_preserves_content_disposition(self, buf, stream) -> None:
 
     async def test_preserve_content_disposition_header(self, buf, stream):
         # https://github.com/aio-libs/aiohttp/pull/3475#issuecomment-451072381
-        with open(__file__, "rb") as fobj:
+        with pathlib.Path(__file__).open("rb") as fobj:
             with aiohttp.MultipartWriter("form-data", boundary=":") as writer:
                 part = writer.append(
                     fobj,
@@ -1297,7 +1298,7 @@ async def test_preserve_content_disposition_header(self, buf, stream):
 
     async def test_set_content_disposition_override(self, buf, stream):
         # https://github.com/aio-libs/aiohttp/pull/3475#issuecomment-451072381
-        with open(__file__, "rb") as fobj:
+        with pathlib.Path(__file__).open("rb") as fobj:
             with aiohttp.MultipartWriter("form-data", boundary=":") as writer:
                 part = writer.append(
                     fobj,
@@ -1324,7 +1325,7 @@ async def test_set_content_disposition_override(self, buf, stream):
 
     async def test_reset_content_disposition_header(self, buf, stream):
         # https://github.com/aio-libs/aiohttp/pull/3475#issuecomment-451072381
-        with open(__file__, "rb") as fobj:
+        with pathlib.Path(__file__).open("rb") as fobj:
             with aiohttp.MultipartWriter("form-data", boundary=":") as writer:
                 part = writer.append(
                     fobj,

tests/test_proxy_functional.py

@@ -731,7 +731,7 @@ async def test_proxy_from_env_http_with_auth_from_netrc(
         auth.login,
         auth.password,
     )
-    with open(str(netrc_file), "w") as f:
+    with netrc_file.open("w") as f:
         f.write(netrc_file_data)
     mocker.patch.dict(
         os.environ, {"http_proxy": str(proxy.url), "NETRC": str(netrc_file)}
@@ -757,7 +757,7 @@ async def test_proxy_from_env_http_without_auth_from_netrc(
         auth.login,
         auth.password,
     )
-    with open(str(netrc_file), "w") as f:
+    with netrc_file.open("w") as f:
         f.write(netrc_file_data)
     mocker.patch.dict(
         os.environ, {"http_proxy": str(proxy.url), "NETRC": str(netrc_file)}
@@ -780,7 +780,7 @@ async def test_proxy_from_env_http_without_auth_from_wrong_netrc(
     auth = aiohttp.BasicAuth("user", "pass")
     netrc_file = tmp_path / "test_netrc"
     invalid_data = f"machine 127.0.0.1 {auth.login} pass {auth.password}"
-    with open(str(netrc_file), "w") as f:
+    with netrc_file.open("w") as f:
         f.write(invalid_data)
 
     mocker.patch.dict(

tests/test_urldispatch.py

@@ -1,4 +1,3 @@
-import os
 import pathlib
 import re
 from collections.abc import Container, Iterable, Mapping, MutableMapping, Sized
@@ -49,7 +48,7 @@ def fill_routes(router):
     def go():
         route1 = router.add_route("GET", "/plain", make_handler())
         route2 = router.add_route("GET", "/variable/{name}", make_handler())
-        resource = router.add_static("/static", os.path.dirname(aiohttp.__file__))
+        resource = router.add_static("/static", pathlib.Path(aiohttp.__file__).parent)
         return [route1, route2] + list(resource)
 
     return go
@@ -342,7 +341,7 @@ def test_route_dynamic(router) -> None:
 
 def test_add_static(router) -> None:
     resource = router.add_static(
-        "/st", os.path.dirname(aiohttp.__file__), name="static"
+        "/st", pathlib.Path(aiohttp.__file__).parent, name="static"
     )
     assert router["static"] is resource
     url = resource.url_for(filename="/dir/a.txt")
@@ -351,7 +350,7 @@ def test_add_static(router) -> None:
 
 
 def test_add_static_append_version(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(__file__), name="static")
+    resource = router.add_static("/st", pathlib.Path(__file__).parent, name="static")
     url = resource.url_for(filename="/data.unknown_mime_type", append_version=True)
     expect_url = (
         "/st/data.unknown_mime_type?" "v=aUsn8CHEhhszc81d28QmlcBW0KQpfS2F4trgQKhOYd8%3D"
@@ -361,7 +360,7 @@ def test_add_static_append_version(router) -> None:
 
 def test_add_static_append_version_set_from_constructor(router) -> None:
     resource = router.add_static(
-        "/st", os.path.dirname(__file__), append_version=True, name="static"
+        "/st", pathlib.Path(__file__).parent, append_version=True, name="static"
     )
     url = resource.url_for(filename="/data.unknown_mime_type")
     expect_url = (
@@ -372,15 +371,15 @@ def test_add_static_append_version_set_from_constructor(router) -> None:
 
 def test_add_static_append_version_override_constructor(router) -> None:
     resource = router.add_static(
-        "/st", os.path.dirname(__file__), append_version=True, name="static"
+        "/st", pathlib.Path(__file__).parent, append_version=True, name="static"
     )
     url = resource.url_for(filename="/data.unknown_mime_type", append_version=False)
     expect_url = "/st/data.unknown_mime_type"
     assert expect_url == str(url)
 
 
 def test_add_static_append_version_filename_without_slash(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(__file__), name="static")
+    resource = router.add_static("/st", pathlib.Path(__file__).parent, name="static")
     url = resource.url_for(filename="data.unknown_mime_type", append_version=True)
     expect_url = (
         "/st/data.unknown_mime_type?" "v=aUsn8CHEhhszc81d28QmlcBW0KQpfS2F4trgQKhOYd8%3D"
@@ -389,27 +388,26 @@ def test_add_static_append_version_filename_without_slash(router) -> None:
 
 
 def test_add_static_append_version_non_exists_file(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(__file__), name="static")
+    resource = router.add_static("/st", pathlib.Path(__file__).parent, name="static")
     url = resource.url_for(filename="/non_exists_file", append_version=True)
     assert "/st/non_exists_file" == str(url)
 
 
 def test_add_static_append_version_non_exists_file_without_slash(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(__file__), name="static")
+    resource = router.add_static("/st", pathlib.Path(__file__).parent, name="static")
     url = resource.url_for(filename="non_exists_file", append_version=True)
     assert "/st/non_exists_file" == str(url)
 
 
-def test_add_static_append_version_follow_symlink(router, tmpdir) -> None:
+def test_add_static_append_version_follow_symlink(router, tmp_path) -> None:
     # Tests the access to a symlink, in static folder with apeend_version
-    tmp_dir_path = str(tmpdir)
-    symlink_path = os.path.join(tmp_dir_path, "append_version_symlink")
-    symlink_target_path = os.path.dirname(__file__)
-    os.symlink(symlink_target_path, symlink_path, True)
+    symlink_path = tmp_path / "append_version_symlink"
+    symlink_target_path = pathlib.Path(__file__).parent
+    pathlib.Path(str(symlink_path)).symlink_to(str(symlink_target_path), True)
 
     # Register global static route:
     resource = router.add_static(
-        "/st", tmp_dir_path, follow_symlinks=True, append_version=True
+        "/st", str(tmp_path), follow_symlinks=True, append_version=True
     )
 
     url = resource.url_for(filename="/append_version_symlink/data.unknown_mime_type")
@@ -421,16 +419,16 @@ def test_add_static_append_version_follow_symlink(router, tmpdir) -> None:
     assert expect_url == str(url)
 
 
-def test_add_static_append_version_not_follow_symlink(router, tmpdir) -> None:
+def test_add_static_append_version_not_follow_symlink(router, tmp_path) -> None:
     # Tests the access to a symlink, in static folder with apeend_version
-    tmp_dir_path = str(tmpdir)
-    symlink_path = os.path.join(tmp_dir_path, "append_version_symlink")
-    symlink_target_path = os.path.dirname(__file__)
-    os.symlink(symlink_target_path, symlink_path, True)
+    symlink_path = tmp_path / "append_version_symlink"
+    symlink_target_path = pathlib.Path(__file__).parent
+
+    pathlib.Path(str(symlink_path)).symlink_to(str(symlink_target_path), True)
 
     # Register global static route:
     resource = router.add_static(
-        "/st", tmp_dir_path, follow_symlinks=False, append_version=True
+        "/st", str(tmp_path), follow_symlinks=False, append_version=True
     )
 
     filename = "/append_version_symlink/data.unknown_mime_type"
@@ -467,7 +465,7 @@ def test_dynamic_not_match(router) -> None:
 
 
 async def test_static_not_match(router) -> None:
-    router.add_static("/pre", os.path.dirname(aiohttp.__file__), name="name")
+    router.add_static("/pre", pathlib.Path(aiohttp.__file__).parent, name="name")
     resource = router["name"]
     ret = await resource.resolve(make_mocked_request("GET", "/another/path"))
     assert (None, set()) == ret
@@ -503,17 +501,17 @@ def test_contains(router) -> None:
 
 
 def test_static_repr(router) -> None:
-    router.add_static("/get", os.path.dirname(aiohttp.__file__), name="name")
+    router.add_static("/get", pathlib.Path(aiohttp.__file__).parent, name="name")
     assert Matches(r"<StaticResource 'name' /get") == repr(router["name"])
 
 
 def test_static_adds_slash(router) -> None:
-    route = router.add_static("/prefix", os.path.dirname(aiohttp.__file__))
+    route = router.add_static("/prefix", pathlib.Path(aiohttp.__file__).parent)
     assert "/prefix" == route._prefix
 
 
 def test_static_remove_trailing_slash(router) -> None:
-    route = router.add_static("/prefix/", os.path.dirname(aiohttp.__file__))
+    route = router.add_static("/prefix/", pathlib.Path(aiohttp.__file__).parent)
     assert "/prefix" == route._prefix
 
 
@@ -778,7 +776,7 @@ def test_named_resources(router) -> None:
     route1 = router.add_route("GET", "/plain", make_handler(), name="route1")
     route2 = router.add_route("GET", "/variable/{name}", make_handler(), name="route2")
     route3 = router.add_static(
-        "/static", os.path.dirname(aiohttp.__file__), name="route3"
+        "/static", pathlib.Path(aiohttp.__file__).parent, name="route3"
     )
     names = {route1.name, route2.name, route3.name}
 
@@ -943,11 +941,11 @@ def test_resources_abc(router) -> None:
 
 def test_static_route_user_home(router) -> None:
     here = pathlib.Path(aiohttp.__file__).parent
-    home = pathlib.Path(os.path.expanduser("~"))
-    if not str(here).startswith(str(home)):  # pragma: no cover
+    try:
+        static_dir = pathlib.Path("~") / here.relative_to(pathlib.Path.home())
+    except ValueError:  # pragma: no cover
         pytest.skip("aiohttp folder is not placed in user's HOME")
-    static_dir = "~/" + str(here.relative_to(home))
-    route = router.add_static("/st", static_dir)
+    route = router.add_static("/st", str(static_dir))
     assert here == route.get_info()["directory"]
 
 
@@ -958,13 +956,13 @@ def test_static_route_points_to_file(router) -> None:
 
 
 async def test_404_for_static_resource(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(aiohttp.__file__))
+    resource = router.add_static("/st", pathlib.Path(aiohttp.__file__).parent)
     ret = await resource.resolve(make_mocked_request("GET", "/unknown/path"))
     assert (None, set()) == ret
 
 
 async def test_405_for_resource_adapter(router) -> None:
-    resource = router.add_static("/st", os.path.dirname(aiohttp.__file__))
+    resource = router.add_static("/st", pathlib.Path(aiohttp.__file__).parent)
     ret = await resource.resolve(make_mocked_request("POST", "/st/abc.py"))
     assert (None, {"HEAD", "GET"}) == ret
 
@@ -979,12 +977,12 @@ async def test_check_allowed_method_for_found_resource(router) -> None:
 
 
 def test_url_for_in_static_resource(router) -> None:
-    resource = router.add_static("/static", os.path.dirname(aiohttp.__file__))
+    resource = router.add_static("/static", pathlib.Path(aiohttp.__file__).parent)
     assert URL("/static/file.txt") == resource.url_for(filename="file.txt")
 
 
 def test_url_for_in_static_resource_pathlib(router) -> None:
-    resource = router.add_static("/static", os.path.dirname(aiohttp.__file__))
+    resource = router.add_static("/static", pathlib.Path(aiohttp.__file__).parent)
     assert URL("/static/file.txt") == resource.url_for(
         filename=pathlib.Path("file.txt")
     )
@@ -1163,7 +1161,7 @@ def test_frozen_app_on_subapp(app) -> None:
 
 
 def test_set_options_route(router) -> None:
-    resource = router.add_static("/static", os.path.dirname(aiohttp.__file__))
+    resource = router.add_static("/static", pathlib.Path(aiohttp.__file__).parent)
     options = None
     for route in resource:
         if route.method == "OPTIONS":
@@ -1233,7 +1231,7 @@ def test_dynamic_resource_canonical() -> None:
 
 def test_static_resource_canonical() -> None:
     prefix = "/prefix"
-    directory = str(os.path.dirname(aiohttp.__file__))
+    directory = str(pathlib.Path(aiohttp.__file__).parent)
     canonical = prefix
     res = StaticResource(prefix=prefix, directory=directory)
     assert res.canonical == canonical