Skip to content
Merged
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 AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,4 @@ Xuecong Liao
Yoav Caspi
Zac Hatfield-Dodds
Zoltán Máté
Zsolt Cserna
1 change: 1 addition & 0 deletions changelog/7951.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed handling of recursive symlinks when collecting tests.
39 changes: 38 additions & 1 deletion src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
import uuid
import warnings
from enum import Enum
from errno import EBADF
from errno import ELOOP
from errno import ENOENT
from errno import ENOTDIR
from functools import partial
from os.path import expanduser
from os.path import expandvars
Expand Down Expand Up @@ -37,6 +41,24 @@

_AnyPurePath = TypeVar("_AnyPurePath", bound=PurePath)

# The following function, variables and comments were
# copied from cpython 3.9 Lib/pathlib.py file.

# EBADF - guard against macOS `stat` throwing EBADF
_IGNORED_ERRORS = (ENOENT, ENOTDIR, EBADF, ELOOP)

_IGNORED_WINERRORS = (
21, # ERROR_NOT_READY - drive exists but is not accessible
1921, # ERROR_CANT_RESOLVE_FILENAME - fix for broken symlink pointing to itself
)


def _ignore_error(exception):
return (
getattr(exception, "errno", None) in _IGNORED_ERRORS
or getattr(exception, "winerror", None) in _IGNORED_WINERRORS
)


def get_lock_path(path: _AnyPurePath) -> _AnyPurePath:
return path.joinpath(".lock")
Expand Down Expand Up @@ -555,8 +577,23 @@ def visit(

Entries at each directory level are sorted.
"""
entries = sorted(os.scandir(path), key=lambda entry: entry.name)

# Skip entries with symlink loops and other brokenness, so the caller doesn't
# have to deal with it.
entries = []
for entry in os.scandir(path):
try:
Copy link
Member

Choose a reason for hiding this comment

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

I would add a comment here, something like this:

Skip entries with symlink loops and other brokenness, so the caller doesn't
have to deal with it.

entry.is_file()
except OSError as err:
if _ignore_error(err):
continue
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
continue
continue
raise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In such case, when the directory have n entries where 1 entry is broken, the caller will not able to see any entries but the error itself. In such case, the caller won't be able to ignore or somehow workaround the error.
On the other hand, raising the error makes sense as if is_file() raises some error, probably the following open() or other file-related call will fail and it is better to report the error as soon as possible.
So made the suggested change, just wanted to comment on it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with both the first and second part.

raise
entries.append(entry)

entries.sort(key=lambda entry: entry.name)

yield from entries

for entry in entries:
if entry.is_dir(follow_symlinks=False) and recurse(entry):
yield from visit(entry.path, recurse)
Expand Down
14 changes: 14 additions & 0 deletions testing/test_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1404,3 +1404,17 @@ def a(): return 4
result = testdir.runpytest()
# Not INTERNAL_ERROR
assert result.ret == ExitCode.INTERRUPTED


def test_does_not_crash_on_recursive_symlink(testdir: Testdir) -> None:
"""Regression test for an issue around recursive symlinks (#7951)."""
symlink_or_skip("recursive", testdir.tmpdir.join("recursive"))
testdir.makepyfile(
"""
def test_foo(): assert True
"""
)
result = testdir.runpytest()

assert result.ret == ExitCode.OK
assert result.parseoutcomes() == {"passed": 1}
13 changes: 13 additions & 0 deletions testing/test_pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
from _pytest.pathlib import ImportPathMismatchError
from _pytest.pathlib import maybe_delete_a_numbered_dir
from _pytest.pathlib import resolve_package_path
from _pytest.pathlib import symlink_or_skip
from _pytest.pathlib import visit


class TestFNMatcherPort:
Expand Down Expand Up @@ -401,3 +403,14 @@ def test_commonpath() -> None:
assert commonpath(subpath, path) == path
assert commonpath(Path(str(path) + "suffix"), path) == path.parent
assert commonpath(path, path.parent.parent) == path.parent.parent


def test_visit_ignores_errors(tmpdir) -> None:
symlink_or_skip("recursive", tmpdir.join("recursive"))
tmpdir.join("foo").write_binary(b"")
tmpdir.join("bar").write_binary(b"")

assert [entry.name for entry in visit(tmpdir, recurse=lambda entry: False)] == [
"bar",
"foo",
]