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

improve(fixtures-per-test): exclude pseudo fixtures from output #11295 #12129

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

WarrenTheRabbit
Copy link
Contributor

@WarrenTheRabbit WarrenTheRabbit commented Mar 17, 2024

Closes #11295 by excluding 'pseudo fixtures' from the --fixtures-per-test output.

pseudo fixtures are neither builtin fixtures nor ones created by the user with the pytest.fixture decorator. They are created internally by pytest when @pytest.mark.parametrize directly parametrizes a test. It provides a succinct syntax for batch executing a test over multiple argument sets.

For example, when pytest --fixtures-per-test is run, this test

# test_file.py
Import pytest

@pytest.mark.parametrize("x", [1, 2])
def test_one(monkeypatch, x):
    pass

will no longer produce output such as this:

------------------------- fixtures used by test_one[1] -------------------------
----------------------------------- (test_file.py:5) ------------------------------
monkeypatch -- src/_pytest/monkeypatch.py:33
    A convenient fixture for monkey-patching.
x -- src/_pytest/python.py:1113
    no docstring available
    
------------------------- fixtures used by test_one[2] -------------------------
----------------------------------- (test_file.py:5) ------------------------------
monkeypatch -- src/_pytest/monkeypatch.py:33
    A convenient fixture for monkey-patching.
x -- src/_pytest/python.py:1113
    no docstring available

Instead, it will be:

------------------------- fixtures used by test_one[1] -------------------------
----------------------------------- (test_file.py:5) ------------------------------
monkeypatch -- src/_pytest/monkeypatch.py:33
    A convenient fixture for monkey-patching.
    
------------------------- fixtures used by test_one[2] -------------------------
----------------------------------- (test_file.py:5) ------------------------------
monkeypatch -- src/_pytest/monkeypatch.py:33
    A convenient fixture for monkey-patching.

Justification for changing the output

The original output did not match with new users' intuitions and expectations

As a new user, I found it unintuitive to see the @pytest.mark.parametrize variables appear in my --fixtures-per-test report. I am of the opinion that the inclusion of pseudo fixtures in the output confuses new users because they do not conform to the expectations established in the documentation. Namely, that fixtures are

  • richly reusable
  • provide setup/teardown features
  • created via the @pytest.fixture decorator

The original output puts attention on internal implementation details

The purpose of --fixtures-per-test is to create a summary of the user's fixture decisions and dependencies. Yet creating a fixture for the user is not the goal of the direct parametrization mark. Instead, _pytest'_s internals just leverage the fixture system to achieve the actual goal: a succinct batch execution syntax. I believe that including the pseudo fixtures in the output exposes internal implementation details unnecessarily and distracts from the user-side summary.

Checklist

  • Include new tests or update existing tests when applicable.
  • I have not found any documentation to update yet.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Add text like closes #XYZW to the PR . . .
  • Create a new changelog file . . .
  • Add yourself to AUTHORS in alphabetical order.

@WarrenTheRabbit WarrenTheRabbit changed the title Exclude psuedo fixtures from --fixtures-per-test output #11295 exclude pseudo fixtures from --fixtures-per-test output #11295 Mar 17, 2024
@WarrenTheRabbit WarrenTheRabbit changed the title exclude pseudo fixtures from --fixtures-per-test output #11295 improve(fixtures-per-test): exclude pseudo fixtures from output #11295 Mar 17, 2024
WarrenTheRabbit and others added 11 commits March 18, 2024 08:25
Adds a test suite to validate that --fixtures-per-test behaves
as intuitively as possible for new users by excluding
counter-intuitive output: the pseudo fixtures that result
from directly parametrizing a test with ``@pytest.mark.parametrize``.

The test suite further validates that fixtures which have been
parametrized, either by direct or indirect means, are retained in
the --fixtures-per-test output.
Addresses issue pytest-dev#11295 by excluding from the --fixtures-per-test
output any 'pseudo fixture' that results from directly parametrizating
a test with ``@pytest.mark.parametrize``.

The justification for removing these fixtures from the report is that

a) They are unintuitive. Their appearance in the fixtures-per-test
report confuses new users because the fixtures created via
``@pytest.mark.parametrize`` do not confrom to the expectations
established in the documentation; namely, that fixtures are
	- richly reusable
	- provide setup/teardown features
	- created via the ``@pytest.fixture` decorator

b) They are an internal implementation detail. It is not the explicit
goal of the direct parametrization mark to create a fixture; instead,
pytest's internals leverages the fixture system to achieve the explicit
goal: a succinct batch execution syntax. Consequently, exposing the
fixtures that implement the batch execution behaviour reveal more
about pytest's internals than they do about the user's own design
choices and test dependencies.
@WarrenTheRabbit WarrenTheRabbit force-pushed the exclude-psuedo-fixtures-from-fixtures-per-test-output branch from fb25685 to c0a23ac Compare March 17, 2024 21:25
@WarrenTheRabbit
Copy link
Contributor Author

WarrenTheRabbit commented Mar 18, 2024

Issue #12086 suggests another possible benefit of removing pseudo fixtures from the --fixtures-per-test output. If a user is expecting their fixture, user_defined, to be parametrised in the following manner:

@pytest.fixture
def user_defined(request):
   return request.param * 3
 
@pytest.mark.parametrize('user_defined', [1])
def test_function(user_defined):
    assert user_defined == 3

This test will fail:

user_defined = 1

    @pytest.mark.parametrize('user_defined', [1])
    def test_scenario_1(user_defined):
>       assert user_defined == 3
E       assert 1 == 3

file.py:9: AssertionError

Yet the current output of --fixtures-per-test does not provide a clue about what is happening. Even though the user-defined fixture has been shadowed by parametrization and is no longer in the test function's scope, it doesn't look like that has happened:

--------------------------- fixtures used by test_scenario_1[1] ----------------------------
--------------------------------------- (file.py:8) ----------------------------------------
user_defined -- src/_pytest/python.py:1113
    no docstring available

But if pseudo fixtures are excluded from the output, there is a stronger message about the user-defined fixtures's non-use because no fixture use will be reported.

Documentation needs to clarify

From one point of view, one mystery has been replaced with another, so I definitely think documentation needs to be available to help clarify the matter for new users.

Devil's advocate

Should --fixtures-per-test still include pseudo fixtures but add a docstring that explains what they are? In my opinion, this is very explicit, which is good, and it will provide some comfort to users when they use --fixtures-per-test and have their intuitions about fixtures in pytest violated, but I suppose it still has the problem of exposing internal details.

Comment on lines 1614 to 1615
if verbose <= 0 and argname.startswith("_"):
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this condition needs to be moved into write_item before the occurrence of the terminal writes

     tw.sep("-", f"fixtures used by {item.name}")
     tw.sep("-", f"({get_best_relpath(item.function)})") 

and adapted to filter fixturedefs of such private fixtures (assuming verbosity less than or equal to zero).

Otherwise there is a circumstance where all an item's fixtures have been excluded from the output yet the terminal still starts being written to as if fixture output is about to follow.

For example, if all of test_private_fixtures's fixtures start with _ and verbosity is zero or less, then no fixtures will be shown but fixtures used by test_private_fixtures. . . etc will be shown.

TODO:

  • add tests for fixtures starting with _
  • adapt condition to conditionally filter the fixturedefs list

I could be wrong about all of this. I will write up the test tonight or tomorrow and see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have confirmed that in this PR private fixtures are not shown when verbosity is zero or less but the fixtures used by . . . is still printed. Is this desired behaviour?
I don't know. It is how pytest currently handles reporting on private fixtures.

import pytest

# DEFINE FIXTURES
#################

@pytest.fixture
def _private():
    """Private fixture"""
    pass

@pytest.fixture
def public():
    """Public fixture."""
    pass

# TESTS
#################

def test_private_fixture(_private):
    pass

def test_public_fixture(public):
    pass

@pytest.mark.parametrize("pseudo", [1])
def test_pseudo_fixture(pseudo):
    pass

def test_no_fixture():
    pass

Running pytest --fixtures-per-test on the current version of pytest shows:

----------- fixtures used by test_private_fixture ------------
-------------------- (test_private.py:14) --------------------

------------ fixtures used by test_public_fixture ------------
-------------------- (test_private.py:17) --------------------
public -- test_private.py:9
    Public fixture.

---------- fixtures used by test_pseudo_fixture[1] -----------
-------------------- (test_private.py:20) --------------------
pseudo -- .../_pytest/python.py:1112
    no docstring available

I'm proposing it shows:

--------------------- fixtures used by test_public_fixture ----------------------
-------------------------- (test_fixture_marker.py:23) --------------------------
public -- test_fixture_marker.py:12
    Public fixture.

Notable differences:

  • pseudo fixtures in tests are ignored
  • a test with only private fixtures is treated like a test with no fixtures at verbosity zero or less (i.e. the test is not reported on at all)

The output this PR currently produces is:

--------------------- fixtures used by test_private_fixture ---------------------
-------------------------- (test_fixture_marker.py:20) --------------------------

--------------------- fixtures used by test_public_fixture ----------------------
-------------------------- (test_fixture_marker.py:23) --------------------------
public -- test_fixture_marker.py:12
    Public fixture.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @WarrenTheRabbit. IMO it is indeed better not to show params as fixtures in --fixtures-per-test output, for the reasons you stated, and also because they are somewhat already reflected in the test id (the part in [...]).

For the implementation you've used _get_direct_parametrize_args to exclude the params. But at this point we've already gathered this info for the item (test), which can be found in the callspec field. So it's better to use that.

However, I'd even go a step further and just exclude "pseudo" fixtures directly. This would involve adding an indication on the FixtureDef whether it's a "pseudo" fixture (we really ought to find a better name for this...). As long as it's only used for "information" purposes such as --fixtures-per-test and not for the actual business logic of fixtures, I have no objection to adding such an indication. Then, the change could be to just skip over such FixtureDefs.

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

Successfully merging this pull request may close these issues.

--fixtures-per-test: Exclude parametrizing pseudo fixtures?
2 participants