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

Issues a warning when a function is not collected as a test case just because it uses @pytest.fixture (#12989) #13051

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

Conversation

dongfangtianyu
Copy link
Contributor

close #12989 .

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits.
  • Create a new changelog file in the changelog folder

A function is declared as a test case using the convention of the test prefix in its name , or as a fixture using the pytest.fixture decorator.

When both are used simultaneously, it is not possible to have both a test case and a fixture.

So a warning is issued to remind the user: no test case, only a fixture.

Example

import pytest

@pytest.fixture
def test_function():
    pass
(venv) C:\Users\administrator\pytest\demo>pytest
============================= test session starts ==============================
platform win32 -- Python 3.12.0, pytest-8.4.0.dev262+gf160963c4.d20241211, pluggy-1.5.0
rootdir: C:\Users\administrator\pytest\demo
configfile: pytest.ini
plugins: hypothesis-6.111.0, xdist-3.6.1
collected 0 items                                                               

=============================== warnings summary =============================== 
test_aa.py:1
  C:\Users\administrator\pytest\demo\test_aa.py:1: PytestCollectionWarning: ca
nnot collect test function 'test_function',because it used the '@pytest.fixture' than becomes a fixture (from: test_aa.py)
    import warnings

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================== 1 warning in 0.26s ==============================

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Dec 11, 2024
@dongfangtianyu
Copy link
Contributor Author

Probably should add an ini config that allows old projects to ignore this warning?

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

I'm probably in favor of introducing a warning, I'm not sure what fixture requires being named test_; and if so they can suppress the warning. But it might be a better fit for https://pypi.org/project/flake8-pytest-style/

@@ -403,6 +403,7 @@ filterwarnings = [
"ignore:VendorImporter\\.find_spec\\(\\) not found; falling back to find_module\\(\\):ImportWarning",
# https://github.com/pytest-dev/execnet/pull/127
"ignore:isSet\\(\\) is deprecated, use is_set\\(\\) instead:DeprecationWarning",
"ignore:.*becomes a fixture.*:pytest.PytestCollectionWarning",
Copy link
Member

Choose a reason for hiding this comment

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

having to add an entry to filterwarnings is not great. What's the reason we need to add it?

@@ -88,6 +88,65 @@ prefer. You can also start out from existing :ref:`unittest.TestCase
style <unittest.TestCase>`.


Distinguishing fixtures and tests
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding this section is net positive, if you think https://docs.pytest.org/en/stable/how-to/fixtures.html#how-to-fixtures doesn't explain well what a fixture is then the intro section should be improved.

Comment on lines +360 to +362
f"cannot collect test function {name!r},"
f"because it used the '@pytest.fixture' than becomes a fixture "
f"(from: {self.nodeid})"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"cannot collect test function {name!r},"
f"because it used the '@pytest.fixture' than becomes a fixture "
f"(from: {self.nodeid})"
f"failed to collect test function {name!r},"
f"because it uses '@pytest.fixture'. A function cannot simultaneously "
f"be both a test and a fixture.\n"
f"If you want to use this as a fixture, rename it to not start with "
f'"test_" to suppress this warning\n'
f"If you want to test the fixture, write a separate test that "
f"uses it.\n"
f"(from: {self.nodeid})"

Maybe something like that? Also I'm not sure we need both the name and the nodeid?

[
"collected 0 items",
"*== warnings summary ==*",
"*because it used the '@pytest.fixture' than becomes a fixture*",
Copy link
Member

Choose a reason for hiding this comment

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

when we're warning with an f-string I think it's good if this checks the entire warning string.
I also think it's good form to use result.assert_outcomes(warnings=1) here to make sure we're not outputting multiple warnings or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
2 participants