Skip to content

main: use a better windows short-path comparison method #13598

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/13598.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed possible collection confusion on Windows when short paths and symlinks are involved.
13 changes: 5 additions & 8 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from _pytest.pathlib import bestrelpath
from _pytest.pathlib import fnmatch_ex
from _pytest.pathlib import safe_exists
from _pytest.pathlib import samefile_nofollow
from _pytest.pathlib import scandir
from _pytest.reports import CollectReport
from _pytest.reports import TestReport
Expand Down Expand Up @@ -935,14 +936,10 @@ def collect(self) -> Iterator[nodes.Item | nodes.Collector]:
is_match = node.path == matchparts[0]
if sys.platform == "win32" and not is_match:
# In case the file paths do not match, fallback to samefile() to
# account for short-paths on Windows (#11895).
same_file = os.path.samefile(node.path, matchparts[0])
# We don't want to match links to the current node,
# otherwise we would match the same file more than once (#12039).
is_match = same_file and (
os.path.islink(node.path)
== os.path.islink(matchparts[0])
)
# account for short-paths on Windows (#11895). But use a version
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the logic into a function in compat and add a unittests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Since I don't run windows, going to rely on CI to see if it actually passes there.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about that, I run Windows but didn't have time to check this.

# which doesn't resolve symlinks, otherwise we might match the
# same file more than once (#12039).
is_match = samefile_nofollow(node.path, matchparts[0])

# Name part e.g. `TestIt` in `/a/b/test_file.py::TestIt::test_it`.
else:
Expand Down
8 changes: 8 additions & 0 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -1053,3 +1053,11 @@ def safe_exists(p: Path) -> bool:
# ValueError: stat: path too long for Windows
# OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect
return False


def samefile_nofollow(p1: Path, p2: Path) -> bool:
"""Test whether two paths reference the same actual file or directory.

Unlike Path.samefile(), does not resolve symlinks.
"""
return os.path.samestat(p1.lstat(), p2.lstat())
35 changes: 35 additions & 0 deletions testing/test_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1780,6 +1780,41 @@ def test_collect_short_file_windows(pytester: Pytester) -> None:
assert result.parseoutcomes() == {"passed": 1}


def test_collect_short_file_windows_multi_level_symlink(
pytester: Pytester,
request: FixtureRequest,
) -> None:
"""Regression test for multi-level Windows short-path comparison with
symlinks.

Previously, when matching collection arguments against collected nodes on
Windows, the short path fallback resolved symlinks. With a chain a -> b ->
target, comparing 'a' against 'b' would incorrectly succeed because both
resolved to 'target', which could cause incorrect matching or duplicate
collection.
"""
# Prepare target directory with a test file.
short_path = Path(tempfile.mkdtemp())
request.addfinalizer(lambda: shutil.rmtree(short_path, ignore_errors=True))
target = short_path / "target"
target.mkdir()
(target / "test_chain.py").write_text("def test_chain(): pass", encoding="UTF-8")

# Create multi-level symlink chain: a -> b -> target.
b = short_path / "b"
a = short_path / "a"
symlink_or_skip(target, b, target_is_directory=True)
symlink_or_skip(b, a, target_is_directory=True)

# Collect via the first symlink; should find exactly one test.
result = pytester.runpytest(a)
result.assert_outcomes(passed=1)

# Collect via the intermediate symlink; also exactly one test.
result = pytester.runpytest(b)
result.assert_outcomes(passed=1)


def test_pyargs_collection_tree(pytester: Pytester, monkeypatch: MonkeyPatch) -> None:
"""When using `--pyargs`, the collection tree of a pyargs collection
argument should only include parents in the import path, not up to confcutdir.
Expand Down
Loading