-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Weird collection stats when filtering from pytest_collection_modifyitems
hook
#12663
Comments
Hi, @soxofaan If the number of items is modified, the hook for example: def pytest_collection_modifyitems(config, items):
k = config.getoption("--fancy-k")
if k:
items_selected = [n for n in items if k in n.nodeid]
items_deselected = [n for n in items if k not in n.nodeid]
items[:] = selected
config.hook.pytest_deselected(items=items_deselected ) result: > pytest -v --fancy-k=n
=============== test session starts ===============
platform win32 -- Python 3.12.0, pytest-8.0.2, pluggy-1.5.0 -- E:\demo\.venv\Scripts\python.exe
cachedir: .pytest_cache
rootdir: E:\demo
plugins: allure-pytest-2.13.5
collected 6 items / 3 deselected / 3 selected
test_.py::test_fruit[banana] PASSED [ 33%]
test_.py::test_fruit[coconut] PASSED [ 66%]
test_.py::test_fruit[dragonfruit] PASSED [100%]
=============== 3 passed, 3 deselected in 0.02s =============== |
thanks @dongfangtianyu for the response But that's pretty confusing. Where is this documented? https://docs.pytest.org/en/stable/reference/reference.html#pytest.hookspec.pytest_deselected currently says
The fact that it says "Called for .." instead of "Call this for" suggests to me that pytest will call this hook for me, and I should not call it myself |
@soxofaan indeed this is a bit confusing per the current documentation, but plugins which implement pytest/src/_pytest/cacheprovider.py Lines 395 to 397 in 33db65c
But we definitely should improve the documentation. Also I'm not sure why we need this hook at all, seems like the terminal could just figure out whatever was deselected itself via a hookwrapper... but that's another discussion. |
FWIW I also found this weird while implementing https://github.com/aio-libs/multidict/blob/bc86d23/tests/conftest.py#L189-L210 |
ok thanks for confirming that the current docs don't align with the currently recommended usage.
That's indeed what I would expect from the standpoint of plugin developer I'll try to whip up a PR to clarify this a bit in the docs |
@nicoddemus is it worthwhile to create a feature request for that? |
Sure, unless @RonnyPfannschmidt can recall a reason not to. |
I believe it's a good ideal to call the deselected hooks with all the missed items but ensuring the message mentioned the hook and the lack of a explanation |
Not sure I follow, can you elaborate? Just to make sure we are on the same page, my idea was that the terminal plugin could implement a hook wrapper around |
I was expanding on that - the items deselected should denote reasons Reason passover is currently not possible and I wanted to implement it after adding callsite compatibility to pluggy (pluggy needs call site compatibility for old code missing a parameter to use the same hook name, I want to avoid similar hacks to the path parameters) |
setup:
platform linux -- Python 3.11.0, pytest-8.3.2
I'm trying to work with the
pytest_collection_modifyitems
hook, but got in a rabbit hole trying to understand the collected/selected/deselected statsminimal reproduction example as follows.
conftest.py
, which defines a--fancy-k
option to filter based on nodeid, much like the existing-k
the tests
To set a baseline, a default run (I'll omit the non-relevant output lines):
A run with classic
-k
, selecting tests with "n" in their name:Nothing weird so far of course
Using my
--fancy-k
option now for same query:First somewhat off-putting observation: at top it says just "collected 5 items" and footer says "3 passed". Nothing really wrong, but the previous stats with standard
-k
were more coherent.Next step: mix
--fancy-k
with-k
(get tests with "n" and "o" in their nodeid):Now the collection and summary stats lost coherence:
The text was updated successfully, but these errors were encountered: