Skip to content

cacheprovider: pytest_collection_modifyitems: copy items #6411

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.

Sign up for GitHub

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

Merged
blueyed merged 1 commit into from
Jan 9, 2020
Merged

cacheprovider: pytest_collection_modifyitems: copy items #6411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/_pytest/cacheprovider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ def pytest_collection_modifyitems(self, session, config, items):
self._report_status = "no previously failed tests, "
if self.config.getoption("last_failed_no_failures") == "none":
self._report_status += "deselecting all items."
config.hook.pytest_deselected(items=items)
config.hook.pytest_deselected(items=items[:])
Copy link
Member

@nicoddemus nicoddemus Jan 6, 2020

Choose a reason for hiding this comment

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

Good catch!

I wonder if it is a cache provider bug though... I mean, should every caller of pytest_deselected(items) always send a copy over? 🤔

Sure, pytest_deselected is kind of a weird hook, but I thought I would raise this.

Copy link
Contributor Author

@blueyed blueyed Jan 7, 2020

Choose a reason for hiding this comment

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

Not sure myself, but here it is clear that the list is cleared just below at least.

FWIW, I've found an issue with auto-deselecting of tests when I was passing a set there (and trying to items[:] it).
As for the terminal plugin only the length is relevant anyway I think (#6409), but thought to keep stats there like before.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt Jan 9, 2020

Choose a reason for hiding this comment

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

for sanity we should actually fail if session.items is passed by accident

this is a great and horrifying catch

the typical/presumed usage is collecting keep and drop in new lists, then passing drop after changing items, so the error is a honest misstake

Copy link
Contributor Author

@blueyed blueyed Jan 9, 2020

Choose a reason for hiding this comment

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

@RonnyPfannschmidt

for sanity we should actually fail if session.items is passed by accident

Makes sense, please consider creating a new issue for it.

the typical/presumed usage is collecting keep and drop in new lists, then passing drop after changing items, so the error is a honest misstake

I could not really parse this.

items[:] = []
else:
self._report_status += "not deselecting items."
Expand Down
17 changes: 17 additions & 0 deletions testing/test_cacheprovider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,11 +681,28 @@ def test_2(): pass
result.stdout.fnmatch_lines(["*2 passed*"])
result = testdir.runpytest("--lf", "--lfnf", "all")
result.stdout.fnmatch_lines(["*2 passed*"])

# Ensure the list passed to pytest_deselected is a copy,
# and not a reference which is cleared right after.
testdir.makeconftest(
"""
deselected = []

def pytest_deselected(items):
global deselected
deselected = items

def pytest_sessionfinish():
print("\\ndeselected={}".format(len(deselected)))
"""
)

result = testdir.runpytest("--lf", "--lfnf", "none")
result.stdout.fnmatch_lines(
[
"collected 2 items / 2 deselected",
"run-last-failure: no previously failed tests, deselecting all items.",
"deselected=2",
"* 2 deselected in *",
]
)
Expand Down