Skip to content
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

Closed
soxofaan opened this issue Jul 26, 2024 · 10 comments
Closed

Comments

@soxofaan
Copy link
Contributor

soxofaan commented Jul 26, 2024

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 stats

minimal reproduction example as follows.
conftest.py, which defines a --fancy-k option to filter based on nodeid, much like the existing -k

def pytest_addoption(parser):
    parser.addoption("--fancy-k")

def pytest_collection_modifyitems(config, items):
    k = config.getoption("--fancy-k")
    if k:
        items[:] = [n for n in items if k in n.nodeid]

the tests

import pytest

@pytest.mark.parametrize("fruit", ["apple", "banana", "coconut", "dragonfruit", "elderberry"])
def test_fruit(fruit):
    assert fruit

To set a baseline, a default run (I'll omit the non-relevant output lines):

❯ pytest
collecting ... collected 5 items
test_fruit.py::test_fruit[apple] PASSED                                  [ 20%]
test_fruit.py::test_fruit[banana] PASSED                                 [ 40%]
test_fruit.py::test_fruit[coconut] PASSED                                [ 60%]
test_fruit.py::test_fruit[dragonfruit] PASSED                            [ 80%]
test_fruit.py::test_fruit[elderberry] PASSED                             [100%]
============================== 5 passed in 0.00s ===============================

A run with classic -k, selecting tests with "n" in their name:

❯ pytest -v -k=n
collecting ... collected 5 items / 2 deselected / 3 selected
test_fruit.py::test_fruit[banana] PASSED                                 [ 33%]
test_fruit.py::test_fruit[coconut] PASSED                                [ 66%]
test_fruit.py::test_fruit[dragonfruit] PASSED                            [100%]
======================= 3 passed, 2 deselected in 0.00s ========================

Nothing weird so far of course

Using my --fancy-k option now for same query:

❯ pytest -v --fancy-k=n
collecting ... collected 5 items
test_fruit.py::test_fruit[banana] PASSED                                 [ 33%]
test_fruit.py::test_fruit[coconut] PASSED                                [ 66%]
test_fruit.py::test_fruit[dragonfruit] PASSED                            [100%]
============================== 3 passed in 0.00s ===============================

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):

❯ pytest -v --fancy-k=n -k=o
collecting ... collected 5 items / 1 deselected / 4 selected
test_fruit.py::test_fruit[coconut] PASSED                                [ 50%]
test_fruit.py::test_fruit[dragonfruit] PASSED                            [100%]
======================= 2 passed, 1 deselected in 0.00s ========================

Now the collection and summary stats lost coherence:

  • "collected 5 items / 1 deselected / 4 selected"
  • "2 passed, 1 deselected"
@dongfangtianyu
Copy link
Contributor

Hi, @soxofaan

If the number of items is modified, the hook pytest_deselected should be used to inform pytest which test cases were deselected.

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 ===============

@soxofaan
Copy link
Contributor Author

thanks @dongfangtianyu for the response

But that's pretty confusing. pytest_deselected is a pytest hook, right? So why should I call it from my own code? Aren't hooks to be implemented by the plugin developer and to be called by pytest?

Where is this documented? https://docs.pytest.org/en/stable/reference/reference.html#pytest.hookspec.pytest_deselected currently says

Called for deselected test items, e.g. by keyword.

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

@nicoddemus
Copy link
Member

@soxofaan indeed this is a bit confusing per the current documentation, but plugins which implement pytest_collection_modifyitems should call pytest_deselected with the items they have deselected, for example:

if self.config.getoption("lf"):
items[:] = previously_failed
config.hook.pytest_deselected(items=previously_passed)

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.

@webknjaz
Copy link
Member

FWIW I also found this weird while implementing https://github.com/aio-libs/multidict/blob/bc86d23/tests/conftest.py#L189-L210

@soxofaan
Copy link
Contributor Author

ok thanks for confirming that the current docs don't align with the currently recommended usage.

seems like the terminal could just figure out whatever was deselected itself via a hookwrapper.

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

@soxofaan
Copy link
Contributor Author

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.

@nicoddemus is it worthwhile to create a feature request for that?

@nicoddemus
Copy link
Member

@nicoddemus is it worthwhile to create a feature request for that?

Sure, unless @RonnyPfannschmidt can recall a reason not to.

@RonnyPfannschmidt
Copy link
Member

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

@nicoddemus
Copy link
Member

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 pytest_collect_modifyitems to figure out how many items were deselected, instead of relying on plugins calling pytest_deselected.

soxofaan added a commit to soxofaan/pytest that referenced this issue Aug 22, 2024
@RonnyPfannschmidt
Copy link
Member

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)

patchback bot pushed a commit that referenced this issue Aug 22, 2024
nicoddemus pushed a commit that referenced this issue Aug 22, 2024
…ge (#12729) (#12733)

Closes #12663

(cherry picked from commit 51845fc)

Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
soxofaan added a commit to ESA-APEx/apex_algorithms that referenced this issue Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants