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 8 commits into
base: main
Choose a base branch
from
4 changes: 3 additions & 1 deletion changelog/12749.feature.rst
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Add :confval:`collect_imported_tests`, when enabled (default is disabled) will make sure to not consider classes/functions which are imported by a test file and contains Test/test_*/*_test.
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
6 changes: 3 additions & 3 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"collect_imported_tests",
"Whether to collect tests in imported modules outside `testpaths`",
type="bool",
default=False,
default=True,
)
group = parser.getgroup("general", "Running and selection options")
group._addoption(
Expand Down Expand Up @@ -973,13 +973,13 @@
self.trace("genitems", node)
if isinstance(node, nodes.Item):
node.ihook.pytest_itemcollected(item=node)
if self.config.getini("collect_imported_tests"):
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)
Expand All @@ -988,16 +988,16 @@
handle_dupes = not (keepduplicates and isinstance(node, nodes.File))
rep, duplicate = self._collect_one_node(node, handle_dupes)

if self.config.getini("collect_imported_tests"):
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
Expand Down
26 changes: 22 additions & 4 deletions testing/test_collect_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def test_collect_imports_disabled(pytester: Pytester) -> None:
collect_imported_tests = false
""")

run_import_class_test(pytester, errors=1)
run_import_class_test(pytester, passed=1)


def test_collect_imports_default(pytester: Pytester) -> None:
Expand All @@ -74,7 +74,7 @@ def test_collect_imports_enabled(pytester: Pytester) -> None:
collect_imported_tests = true
""")

run_import_class_test(pytester, passed=1)
run_import_class_test(pytester, errors=1)


def run_import_functions_test(
Expand Down Expand Up @@ -137,14 +137,32 @@ def test_collect_function_imports_enabled(pytester: Pytester) -> None:
collect_imported_tests = true
""")

run_import_functions_test(pytester, passed=1, errors=0, failed=0)
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"
# 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