Migrate tests to use pathlib.Path

[uranusjr]

I was annoyed enough to spend a day on this.

The pip-specific Path implementation has been removed, and all its usages replaced by pathlib.Path. The tmpdir and tmpdir_factory fixtures are also removed, and all usages replaced by tmp_path and tmp_path_factory, which use pathlib.Path.

As anyone might expect, the bulk of the work is simply adding str() everywhere.

[pfmoore]

Generally I'm +1 on this. I'm going to assume that if the tests pass it's probably OK.

I did start reading through, and made a couple of comments, but I lost the will to live with all those str calls 🙂 Also, black reformattings make the diffs a bit harder to skim. I wouldn't bother doing it now¹, but splitting the commit into (1) mechanical str changes, (2) black reformattings and (3) substantial changes, might hae made it easier to review.

I added a couple of suggestions, but they aren't critical - feel free to treat them as matters of opinion and stick with what you have.

Footnotes

1. And to be 100% clear, if I'd been doing this PR I would never have thought of doing this either... ↩

[pfmoore]

Unrelated to the Path change, but shouldn't we specify an encoding here? (And below).

[uranusjr]

cc @sbidoul I’m not sure if platform-dependant encoding is intended here.

[sbidoul]

It is was not intended, no. I'll do a PR to fix this (it should not matter much for the tests in practice but I spotted one place in the code where it could be an issue).

[pradyunsg]

Hurray! Thanks for picking this up @uranusjr! ^.^

[uranusjr]

Hmm, a couple of failing tests that I can’t figure out. They pass on my machine, and this PR doesn’t even touch those.

FAILED tests/functional/test_freeze.py::test_freeze_with_pip
FAILED tests/functional/test_freeze.py::test_freeze_multiple_exclude_with_all

[sbidoul]

@uranusjr I can reproduce on Linux with nox -s test-3.8 -- -k test_freeze_multiple_exclude_with_all. It passes with test-3.7, however.

With 3.7, the pip freeze --all output is

# Editable install with no version control (coverage==6.4.1)
-e /tmp/pytest-of-me/pytest-0/coverage0/install
# Editable install with no version control (pip==22.2.dev0)
-e /tmp/pytest-of-me/pytest-0/pip0/pip/src
# Editable install with no version control (setuptools==62.3.3)
-e /tmp/pytest-of-me/pytest-0/setuptools0/install
# Editable install with no version control (wheel==0.37.1)
-e /tmp/pytest-of-me/pytest-0/wheel0/install

so there is pip== in it, albeit in the comment that says there is no vcs directory.

With 3.8 the pip freeze output is similar to what we have in the CI log

# Editable install with no version control (coverage==6.4.1)
-e /tmp/pytest-of-me/pytest-0/coverage0/install
-e git+ssh://git@github.com/sbidoul/pip@42359a9605d56a37ac9a1ca76ea08a3b01d48468#egg=pip
# Editable install with no version control (setuptools==62.3.3)
-e /tmp/pytest-of-me/pytest-0/setuptools0/install
# Editable install with no version control (wheel==0.37.1)
-e /tmp/pytest-of-me/pytest-0/wheel0/install

so here we have no pip== in the output because the pip install was detected as an editable with VCS.

That is super weird. Would the shutil.copytree in virtualenv_template behave differently across version and due to a change in this PR ?

We could fix the test with assert "pip==" in result.stdout or "#egg=pip" in result.stdout, which would make it more robust. Although understanding the root cause would be more satisfying.

[uranusjr]

Ah interesting. So in a sense the test is wrong—the behaviour they intend to test was broken by #5031 (before it pip freeze would have outputted pip==22.2.dev0 instead of the editable location), but since the PR (accidentally?) put a pip== in the message, the test continued to pass.

I suspect the behaviour change is due to Path.resolve(). The pathlib implementation on this is known to vary ever so slightly from version to version (under the “bugfix” category), and our old str-based implementation used the naive abspath. I’ll investigate more deeply.

[uranusjr]

Found the exact bugfix that changed behaviour: python/cpython#83571

This can be fixed by using samefile instead of ==. Don’t use == for “does this path point to this thing” logic, folks.

[sbidoul]

Thanks for doing this!

[pradyunsg]

🎉

[q0w]

error: Argument 1 to "move" has incompatible type "Path"; expected "str"
see: https://github.com/python/typeshed/blob/91d6383d9d1ca38157ce46bb498a11347658db1d/stdlib/shutil.pyi#L105

[uranusjr]

So it seems PathLike support here was only added in 3.9, and pre-commit missed this since we only run linters against the newest Python version.