-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add discover_imports
in conf, don't collect imported classes named Test* closes #12749`
#12810
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
Changes from 4 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:`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. | ||
|
||
-- by :user:`FreerGit` |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -78,6 +78,12 @@ | |||||
type="args", | ||||||
default=[], | ||||||
) | ||||||
parser.addini( | ||||||
"collect_imported_tests", | ||||||
"Whether to collect tests in imported modules outside `testpaths`", | ||||||
type="bool", | ||||||
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", | ||||||
|
@@ -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 self.config.getini("collect_imported_tests"): | ||||||
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. The logic for the variable is inverted: if So the default for
Suggested change
|
||||||
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__ | ||||||
if fn_defined_at != in_module: | ||||||
return | ||||||
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 self.config.getini("collect_imported_tests"): | ||||||
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. My other comment applies here as well. |
||||||
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__ | ||||||
if class_defined_at != in_module: | ||||||
rep.result.remove(subnode) | ||||||
|
||||||
if duplicate and not keepduplicates: | ||||||
return | ||||||
if rep.passed: | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
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, errors=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, passed=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=1, errors=0, failed=0) | ||
|
||
|
||
def test_collect_function_imports_disabled(pytester: Pytester) -> None: | ||
pytester.makeini(""" | ||
[pytest] | ||
testpaths = "tests" | ||
collect_imported_tests = false | ||
""") | ||
|
||
run_import_functions_test(pytester, passed=2, errors=0, failed=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.
As per my comment, this description needs an update:
Also, we should describe this option in https://github.com/pytest-dev/pytest/blob/main/doc/en/reference/reference.rst in a new
.. conf-val::
block.