Skip to content
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
4 changes: 3 additions & 1 deletion python/tach/pytest_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,9 @@ def pytest_collection_modifyitems(config: Config, items: list[Item]):
if state is None:
return
items_to_remove = [
item for item in items if state.handler.should_remove_items(file_path=item.path)
item
for item in items
if state.handler.should_remove_items(file_path=item.path.resolve())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

pytest_collection_modifyitems passes item.path directly to the should_remove_items, but item.path can be relative (e.g. when tests live in subdirectories like tests/).

would you mind providing a minimal repro of this issue? i can't seem to reproduce it myself. the tach repo itself has its tests located in a subdirectory (python/tests) and item.path seems to always be absolute no matter how i run it.

]
state.handler.num_removed_items = len(items_to_remove)

Expand Down
45 changes: 45 additions & 0 deletions python/tests/test_pytest_plugin.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
from __future__ import annotations

from pathlib import Path
from unittest.mock import MagicMock, patch

import pytest

from tach.pytest_plugin import pytest_collection_modifyitems

pytest_plugins = ["pytester"]


Expand Down Expand Up @@ -207,6 +212,46 @@ def test_three(self):
result.stdout.fnmatch_lines(["*Skipped 8 test* (3 file*"])


class TestPytestPluginResolveItemPath:
"""Unit test that item.path is resolved before passing to should_remove_items.

pytester always provides absolute item.path, so it can't catch this bug.
We mock the handler to verify resolve() is called.
"""

def test_should_remove_items_receives_resolved_path(self, tmp_path: Path):
# Create a real file so resolve() produces an absolute path
test_file = tmp_path / "tests" / "test_foo.py"
test_file.parent.mkdir()
test_file.touch()
relative_path = Path("tests/test_foo.py")

handler = MagicMock()
handler.should_remove_items.return_value = True # pyright: ignore[reportAny]
handler.num_removed_items = 0

state = MagicMock()
state.handler = handler
state.skip_enabled = False

config = MagicMock()
config.stash.get.return_value = state # pyright: ignore[reportAny]

item = MagicMock()
item.path = relative_path

with patch.object(Path, "resolve", return_value=test_file) as mock_resolve:
pytest_collection_modifyitems(config, [item])
mock_resolve.assert_called_once()

# The key assertion: should_remove_items must receive the resolved
# absolute path, not the relative item.path
called_path: Path = handler.should_remove_items.call_args.kwargs["file_path"] # pyright: ignore[reportAny]
assert called_path == test_file, (
f"should_remove_items received {called_path}, expected resolved path {test_file}"
)


class TestPytestPluginDurations:
def test_shows_estimated_duration_after_caching(
self, tach_project: pytest.Pytester
Expand Down
Loading