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

Add discover_imports in conf, don't collect imported classes named Test* closes #12749` #12810

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ Stefanie Molin
Stefano Taschini
Steffen Allner
Stephan Obermann
Sven
Sven-Hendrik Haase
Sviatoslav Sydorenko
Sylvain Marié
Expand Down
5 changes: 5 additions & 0 deletions changelog/12749.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
New :confval:`collect_imported_tests`: when enabled (the default) pytest will collect classes/functions in test modules even if they are imported from another file.

Setting this to False will make pytest collect classes/functions from test files only if they are defined in that file (as opposed to imported there).

-- by :user:`FreerGit`
10 changes: 9 additions & 1 deletion doc/en/reference/reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1839,9 +1839,17 @@ passed multiple times. The expected format is ``name=value``. For example::
pytest testing doc


.. confval:: tmp_path_retention_count
.. confval:: collect_imported_tests
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this closer to the other options alphabetically?


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
.. versionadded:: 8.4

Setting this to `false` will make pytest collect classes/functions from test
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
Setting this to `false` will make pytest collect classes/functions from test
Setting this to ``false`` will make pytest collect classes/functions from test

files only if they are defined in that file (as opposed to imported there).

.. code-block:: ini
Copy link
Member

@nicoddemus nicoddemus Oct 1, 2024

Choose a reason for hiding this comment

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

RST is fidgety with spaces, this needs to be indented according to the block above otherwise it will not render correctly:

image

(Compare with tmp_path_retention_count).

Suggested change
.. code-block:: ini
.. code-block:: ini


[pytest]
collect_imported_tests = false
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
collect_imported_tests = false
collect_imported_tests = false
Default: ``true``


.. confval:: tmp_path_retention_count

How many sessions should we keep the `tmp_path` directories,
according to `tmp_path_retention_policy`.
Expand Down
31 changes: 31 additions & 0 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@
type="args",
default=[],
)
parser.addini(
"collect_imported_tests",
"Whether to collect tests in imported modules outside `testpaths`",
type="bool",
default=True,
)
group = parser.getgroup("general", "Running and selection options")
group._addoption(
"-x",
Expand Down Expand Up @@ -958,16 +964,41 @@
self.trace.root.indent -= 1

def genitems(self, node: nodes.Item | nodes.Collector) -> Iterator[nodes.Item]:
import inspect

from _pytest.python import Class
from _pytest.python import Function
from _pytest.python import Module

self.trace("genitems", node)
if isinstance(node, nodes.Item):
node.ihook.pytest_itemcollected(item=node)
if not self.config.getini("collect_imported_tests"):
Copy link
Member

Choose a reason for hiding this comment

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

Previously this code was placed in _pytest.python, which I think is the correct place... as it is implemented here, we are calling pytest_itemcollected but then discarding the item if the option is set and the item is not defined in its module, which I think is incorrect: pytest_itemcollected should only really be called if we did collect the item.

In fact, it is important for this to be tested too: the following collection hooks should not receive any item that was discarded due to collect_imported_tests:

  • pytest_itemcollected
  • pytest_collectreport
  • pytest_collection_modifyitems

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Thought I would pick up this issue again after some wait. Is there any examples on how I could test these hooks? Im unsure on how to extract that information from the hooks. Im assuming this is problematic because extensions to pytest could be listening to these hooks?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any examples on how I could test these hooks? Im unsure on how to extract that information from the hooks.

Usually we write a conftest.py file in the tests that implement these hooks, and then set/update some global variable when they are called.

Im assuming this is problematic because extensions to pytest could be listening to these hooks?

Exactly.

if isinstance(node.parent, Module) and isinstance(node, Function):
if inspect.isfunction(node._getobj()):
fn_defined_at = node._getobj().__module__
in_module = node.parent._getobj().__name__

Check warning on line 980 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L979-L980

Added lines #L979 - L980 were not covered by tests
if fn_defined_at != in_module:
return

Check warning on line 982 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L982

Added line #L982 was not covered by tests
yield node
else:
assert isinstance(node, nodes.Collector)
keepduplicates = self.config.getoption("keepduplicates")
# For backward compat, dedup only applies to files.
handle_dupes = not (keepduplicates and isinstance(node, nodes.File))
rep, duplicate = self._collect_one_node(node, handle_dupes)

if not self.config.getini("collect_imported_tests"):
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here, this logic should be moved to _pytest.python instead of main.

for subnode in rep.result:
if isinstance(subnode, Class) and isinstance(
subnode.parent, Module
):
if inspect.isclass(subnode._getobj()):
class_defined_at = subnode._getobj().__module__
in_module = subnode.parent._getobj().__name__

Check warning on line 998 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L997-L998

Added lines #L997 - L998 were not covered by tests
if class_defined_at != in_module:
rep.result.remove(subnode)

Check warning on line 1000 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L1000

Added line #L1000 was not covered by tests

if duplicate and not keepduplicates:
return
if rep.passed:
Expand Down
168 changes: 168 additions & 0 deletions testing/test_collect_imports.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
from __future__ import annotations

import textwrap

from _pytest.pytester import Pytester


def run_import_class_test(pytester: Pytester, passed: int = 0, errors: int = 0) -> None:
src_dir = pytester.mkdir("src")
tests_dir = pytester.mkdir("tests")
src_file = src_dir / "foo.py"

src_file.write_text(
textwrap.dedent("""\
class Testament(object):
def __init__(self):
super().__init__()
self.collections = ["stamp", "coin"]

def personal_property(self):
return [f"my {x} collection" for x in self.collections]
"""),
encoding="utf-8",
)

test_file = tests_dir / "foo_test.py"
test_file.write_text(
textwrap.dedent("""\
import sys
import os

current_file = os.path.abspath(__file__)
current_dir = os.path.dirname(current_file)
parent_dir = os.path.abspath(os.path.join(current_dir, '..'))
sys.path.append(parent_dir)

from src.foo import Testament

class TestDomain:
def test_testament(self):
testament = Testament()
assert testament.personal_property()
"""),
encoding="utf-8",
)

result = pytester.runpytest()
result.assert_outcomes(passed=passed, errors=errors)


def test_collect_imports_disabled(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
testpaths = "tests"
collect_imported_tests = false
""")

run_import_class_test(pytester, passed=1)


def test_collect_imports_default(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
testpaths = "tests"
""")

run_import_class_test(pytester, errors=1)


def test_collect_imports_enabled(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
testpaths = "tests"
collect_imported_tests = true
""")

run_import_class_test(pytester, errors=1)


def run_import_functions_test(
pytester: Pytester, passed: int, errors: int, failed: int
) -> None:
src_dir = pytester.mkdir("src")
tests_dir = pytester.mkdir("tests")

src_file = src_dir / "foo.py"

# Note that these "tests" are should _not_ be treated as tests.
# They are normal functions that happens to have test_* or *_test in the name.
# Thus should _not_ be collected!
src_file.write_text(
textwrap.dedent("""\
def test_function():
some_random_computation = 5
return some_random_computation

def test_bar():
pass
"""),
encoding="utf-8",
)

test_file = tests_dir / "foo_test.py"

# Inferred from the comment above, this means that there is _only_ one actual test
# which should result in only 1 passing test being ran.
test_file.write_text(
textwrap.dedent("""\
import sys
import os

current_file = os.path.abspath(__file__)
current_dir = os.path.dirname(current_file)
parent_dir = os.path.abspath(os.path.join(current_dir, '..'))
sys.path.append(parent_dir)

from src.foo import *

class TestDomain:
def test_important(self):
res = test_function()
if res == 5:
pass

"""),
encoding="utf-8",
)

result = pytester.runpytest()
result.assert_outcomes(passed=passed, errors=errors, failed=failed)


def test_collect_function_imports_enabled(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
testpaths = "tests"
collect_imported_tests = true
""")

run_import_functions_test(pytester, passed=2, errors=0, failed=1)


def test_collect_function_imports_disabled(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
# testpaths = "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 think this was meant to be:

Suggested change
# testpaths = "tests"
testpaths = "tests"

But I don't think testpaths is related to the issue anymore?

Copy link
Author

Choose a reason for hiding this comment

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

I ment delete that commented line but forgot. You're right testpaths is not related and I wanted to make a test to ensure that.

Copy link
Member

Choose a reason for hiding this comment

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

Oh in that case make sure to add a comment/docstring mentioning that if you want to keep the test.

collect_imported_tests = false
""")

run_import_functions_test(pytester, passed=1, errors=0, failed=0)


def test_behaviour_without_testpaths_set_and_false(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
collect_imported_tests = false
""")

run_import_functions_test(pytester, passed=1, errors=0, failed=0)


def test_behaviour_without_testpaths_set_and_true(pytester: Pytester) -> None:
pytester.makeini("""
[pytest]
collect_imported_tests = true
""")

run_import_functions_test(pytester, passed=2, errors=0, failed=1)
Loading