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 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
3 changes: 3 additions & 0 deletions changelog/12749.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Add :confval:`discover_imports`, when disabled (default) will make sure to not consider classes which are imported by a test file and starts with Test.
Copy link
Member

Choose a reason for hiding this comment

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

Nice, changelog entry.

We also need to add documentation for that confval in reference.rst.

I'm not sure about the name too, but at the moment I don't have another suggestion (however we really should strive to find a more appropriate name).

Copy link
Author

Choose a reason for hiding this comment

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

treat_imports_as_tests?
imports_are_tests?
crawl_imports?

Copy link
Member

@nicoddemus nicoddemus Sep 13, 2024

Choose a reason for hiding this comment

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

As per my comment, perhaps we should focus on being more "strict" about what we collect from modules.

collect_?something? = "all"/"local-only"

But we should wait to see what other maintainers think.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, now after a some hours (more than id like to admit) I now have something that works. Both for functions and classes. Let me know how we wanna handle the toggle of the feature. Have a good weekend!


-- by :user:`FreerGit`
5 changes: 5 additions & 0 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ def pytest_addoption(parser: Parser) -> None:
type="args",
default=[],
)
parser.addini(
"discover_imports",
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea that discover_imports is really only active if 'testpaths' is also defined in pytest.ini?

"Whether to discover tests in imported modules outside `testpaths`",
default=False,
Copy link
Member

Choose a reason for hiding this comment

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

We need to start with a default of true for backwards compatibility

Alternatively none with a informative warning

Copy link

Choose a reason for hiding this comment

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

If it helps, I'd be happy to argue that the current behavior was a substantial surprise to me, so that defaulting to False would count as removing a bug.

Copy link
Member

Choose a reason for hiding this comment

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

If it helps, I'd be happy to argue that the current behavior was a substantial surprise to me, so that defaulting to False would count as removing a bug.

I understand where you are coming from and I agree this would be the reasonable default if pytest was being born today... but unfortunately in this case I suspect there are test suites which rely on this behavior, so we really should be conservative here.

)
group = parser.getgroup("general", "Running and selection options")
group._addoption(
"-x",
Expand Down
6 changes: 6 additions & 0 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,12 @@
return self.obj()

def collect(self) -> Iterable[nodes.Item | nodes.Collector]:
if self.config.getini("discover_imports") == ("false" or False):
Copy link
Member

Choose a reason for hiding this comment

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

We should ensure this is always a boolean or parsed somewhere else to ensure Separation

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 changing the default value for the option to False and changing this to if self.config.getinit("discover_imports"): should be enough.

paths = self.config.getini("testpaths")
class_file = inspect.getfile(self.obj)

Check warning on line 746 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L745-L746

Added lines #L745 - L746 were not covered by tests
if not any(string in class_file for string in paths):
return []

Check warning on line 748 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L748

Added line #L748 was not covered by tests

if not safe_getattr(self.obj, "__test__", True):
return []
if hasinit(self.obj):
Expand Down
100 changes: 100 additions & 0 deletions testing/test_discover_imports.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
from __future__ import annotations

import textwrap


def test_discover_imports_enabled(pytester):
src_dir = pytester.mkdir("src")
tests_dir = pytester.mkdir("tests")
pytester.makeini("""
[pytest]
testpaths = "tests"
discover_imports = true
""")

src_file = src_dir / "foo.py"

src_file.write_text(
textwrap.dedent("""\
class TestClass(object):
def __init__(self):
super().__init__()

def test_foobar(self):
return true
"""),
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 TestClass

class TestDomain:
def test_testament(self):
testament = TestClass()
pass
"""),
encoding="utf-8",
)

result = pytester.runpytest()
result.assert_outcomes(errors=1)


def test_discover_imports_disabled(pytester):
src_dir = pytester.mkdir("src")
tests_dir = pytester.mkdir("tests")
pytester.makeini("""
[pytest]
testpaths = "tests"
discover_imports = false
""")

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=1)
Loading