-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Changes from 2 commits
222457d
fa3b631
68ac4a1
a6ee0bc
eb8592c
935c06d
191456e
f1821ea
022d316
6ccb5e7
e2ec64e
43865ed
d2327d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
|
||
-- by :user:`FreerGit` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,11 @@ def pytest_addoption(parser: Parser) -> None: | |
type="args", | ||
default=[], | ||
) | ||
parser.addini( | ||
"discover_imports", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the idea that |
||
"Whether to discover tests in imported modules outside `testpaths`", | ||
default=False, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -741,6 +741,12 @@ | |
return self.obj() | ||
|
||
def collect(self) -> Iterable[nodes.Item | nodes.Collector]: | ||
if self.config.getini("discover_imports") == ("false" or False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think changing the default value for the option to |
||
paths = self.config.getini("testpaths") | ||
class_file = inspect.getfile(self.obj) | ||
if not any(string in class_file for string in paths): | ||
return [] | ||
|
||
if not safe_getattr(self.obj, "__test__", True): | ||
return [] | ||
if hasinit(self.obj): | ||
|
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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!