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.

Sign up for GitHub

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

Jump to bottom

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
+362 −3
Open

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

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
222457d
Add `discover_imports` in conf
FreerGit Sep 12, 2024
fa3b631
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 12, 2024
68ac4a1
`collect_imported_tests` option
FreerGit Sep 24, 2024
a6ee0bc
merge: rename from discover to collect
FreerGit Sep 24, 2024
eb8592c
update default and add docs
FreerGit Oct 1, 2024
935c06d
WIP: don't collect instead of filtering out
FreerGit Nov 4, 2024
191456e
WIP: modified items, reports and items collected
FreerGit Nov 12, 2024
f1821ea
WIP - report collection needs triage
FreerGit Nov 17, 2024
022d316
Ensure correct collection of reports
FreerGit Nov 24, 2024
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
278 changes: 148 additions & 130 deletions testing/test_collect_imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,78 +13,82 @@

# Start of tests for classes

# 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)

# # Verify that the state of hooks
# reprec = pytester.inline_run()
# items_collected = reprec.getcalls("pytest_itemcollected")
# assert len(items_collected) == 1
# for x in items_collected:
# assert x.item._getobj().__name__ == "test_testament"

# def test_collect_imports_default(pytester: Pytester) -> None:
# run_import_class_test(pytester, errors=1)

# # TODO, hooks


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

# run_import_class_test(pytester, errors=1)
# # TODO, hooks

def run_import_class_test(
pytester: Pytester, passed: int = 0, errors: int = 0
) -> RunResult:
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)
return result


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

run_import_class_test(pytester, passed=1)

# Verify that the state of hooks
reprec = pytester.inline_run()
items_collected = reprec.getcalls("pytest_itemcollected")
assert len(items_collected) == 1
for x in items_collected:
assert x.item._getobj().__name__ == "test_testament"


def test_collect_imports_default(pytester: Pytester) -> None:
run_import_class_test(pytester, errors=1)
# TODO


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

run_import_class_test(pytester, errors=1)


# End of tests for classes
Expand Down Expand Up @@ -144,54 +148,54 @@ def test_important(self):
return result


# 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)
# reprec = pytester.inline_run()
# items_collected = reprec.getcalls("pytest_itemcollected")
# # Recall that the default is `collect_imported_tests = true`.
# # Which means that the normal functions are now interpreted as
# # valid tests and `test_function()` will fail.
# assert len(items_collected) == 3
# for x in items_collected:
# assert x.item._getobj().__name__ in [
# "test_important",
# "test_bar",
# "test_function",
# ]


# def test_behaviour_without_testpaths_set_and_false(pytester: Pytester) -> None:
# # Make sure `collect_imported_tests` has no dependence on `testpaths`
# pytester.makeini("""
# [pytest]
# collect_imported_tests = false
# """)

# run_import_functions_test(pytester, passed=1, errors=0, failed=0)
# reprec = pytester.inline_run()
# items_collected = reprec.getcalls("pytest_itemcollected")
# assert len(items_collected) == 1
# for x in items_collected:
# assert x.item._getobj().__name__ == "test_important"


# def test_behaviour_without_testpaths_set_and_true(pytester: Pytester) -> None:
# # Make sure `collect_imported_tests` has no dependence on `testpaths`
# pytester.makeini("""
# [pytest]
# collect_imported_tests = true
# """)

# run_import_functions_test(pytester, passed=2, errors=0, failed=1)
# reprec = pytester.inline_run()
# items_collected = reprec.getcalls("pytest_itemcollected")
# assert len(items_collected) == 3
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)
reprec = pytester.inline_run()
items_collected = reprec.getcalls("pytest_itemcollected")
# Recall that the default is `collect_imported_tests = true`.
# Which means that the normal functions are now interpreted as
# valid tests and `test_function()` will fail.
assert len(items_collected) == 3
for x in items_collected:
assert x.item._getobj().__name__ in [
"test_important",
"test_bar",
"test_function",
]


def test_behaviour_without_testpaths_set_and_false(pytester: Pytester) -> None:
# Make sure `collect_imported_tests` has no dependence on `testpaths`
pytester.makeini("""
[pytest]
collect_imported_tests = false
""")

run_import_functions_test(pytester, passed=1, errors=0, failed=0)
reprec = pytester.inline_run()
items_collected = reprec.getcalls("pytest_itemcollected")
assert len(items_collected) == 1
for x in items_collected:
assert x.item._getobj().__name__ == "test_important"


def test_behaviour_without_testpaths_set_and_true(pytester: Pytester) -> None:
# Make sure `collect_imported_tests` has no dependence on `testpaths`
pytester.makeini("""
[pytest]
collect_imported_tests = true
""")

run_import_functions_test(pytester, passed=2, errors=0, failed=1)
reprec = pytester.inline_run()
items_collected = reprec.getcalls("pytest_itemcollected")
assert len(items_collected) == 3


class TestHookBehaviour:
Expand Down Expand Up @@ -255,11 +259,12 @@ def _test_hook_behaviour_when_collect_off(self, pytester: Pytester) -> None:
"reports": reports,
}

# Now check that the two tests above did indeed result in the same outcome.
def _test_hook_behaviour(self) -> None:
print("ABCD", self.collect_outcomes)
default = self.collect_outcomes["default"]
collect_off = self.collect_outcomes["collect_off"]

# Check that the two tests above did indeed result in the same outcome.
assert default["result"] == collect_off["result"]

assert len(default["modified"]) == len(collect_off["modified"]) == 1
Expand All @@ -282,13 +287,14 @@ def _test_hook_behaviour(self) -> None:
len(default["items_collected"]) == len(collect_off["items_collected"]) == 1
)

# Check if the same tests got collected
def_items_record: RecordedHookCall = default["items_collected"][0]
off_items_record: RecordedHookCall = collect_off["items_collected"][0]
def_items = def_items_record.__dict__["item"]
off_items = off_items_record.__dict__["item"]
assert def_items.name == off_items.name

# TODO: fix diff:
# TODO: fix diff: (This will get cleaned up)
# [
# <CollectReport '' lenresult=1 outcome='passed'>,
# - <CollectReport 'src' lenresult=0 outcome='passed'>,
Expand All @@ -301,12 +307,24 @@ def _test_hook_behaviour(self) -> None:
# ? ^
# ]

# assert len(default['reports']) == len(collect_off['reports'])
# for i in range(len(default['reports'])):
# print("def",default['reports'][i].__dict__)
# print("off",collect_off['reports'][i].__dict__)
for x in default["reports"]:
print("def", x.__dict__)

for x in collect_off["reports"]:
print("off", x.__dict__)

# The two above loops prints:

# def {'nodeid': '', 'outcome': 'passed', 'longrepr': None, 'result': [<Dir test_hook_behaviour0>], 'sections': []} # noqa: E501
# def {'nodeid': 'tests/foo_test.py::TestDomain', 'outcome': 'passed', 'longrepr': None, 'result': [<Function test_important>], 'sections': []} # noqa: E501
# def {'nodeid': 'tests/foo_test.py', 'outcome': 'passed', 'longrepr': None, 'result': [<Class TestDomain>], 'sections': []} # noqa: E501
# def {'nodeid': 'tests', 'outcome': 'passed', 'longrepr': None, 'result': [<Module foo_test.py>], 'sections': []} # noqa: E501
# def {'nodeid': '.', 'outcome': 'passed', 'longrepr': None, 'result': [<Dir tests>], 'sections': []} # noqa: E501
# off {'nodeid': '', 'outcome': 'passed', 'longrepr': None, 'result': [<Dir test_hook_behaviour1>], 'sections': []} # noqa: E501
# off {'nodeid': 'src', 'outcome': 'passed', 'longrepr': None, 'result': [], 'sections': []}
# off {'nodeid': 'tests/foo_test.py::TestDomain', 'outcome': 'passed', 'longrepr': None, 'result': [<Function test_important>], 'sections': []} # noqa: E501
# off {'nodeid': 'tests/foo_test.py', 'outcome': 'passed', 'longrepr': None, 'result': [<Class TestDomain>], 'sections': []} # noqa: E501
# off {'nodeid': 'tests', 'outcome': 'passed', 'longrepr': None, 'result': [<Module foo_test.py>], 'sections': []} # noqa: E501
# off {'nodeid': '.', 'outcome': 'passed', 'longrepr': None, 'result': [<Dir src>, <Dir tests>], 'sections': []} # noqa: E501
Copy link
Author

Choose a reason for hiding this comment

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

At this point, I need some guidance.

The problem now is that while the CollectReport's for classes and functions are correct, the directory gets a report (even though no classes/functions were collected from it). What should be the expected behaviour in this case? Since the directory will be collected before any of the classes/functions I would need to filter it out later on somehow. Not sure what the correct approach is here.

I do feel like collecting the report for the directory (even though it's not "used") makes sense but I would need a maintainer to check.

Copy link
Member

@nicoddemus nicoddemus Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem now is that while the CollectReport's for classes and functions are correct, the directory gets a report (even though no classes/functions were collected from it). What should be the expected behaviour in this case?
I do feel like collecting the report for the directory (even though it's not "used") makes sense but I would need a maintainer to check.

You are correct -- currently on main if we implement pytest_collectreport and run pytest on a bunch of directories without any tests, we get collection reports for all directories, even though no test items were collected.

My previous concern was that we were generating collection reports for items that were in fact being discarded later, and that would definitely be a problem -- but collection reports for directories it is expected that some of them might turn out to not collect anything.


# from pprint import pprint
# pprint(default['reports'])
# pprint(collect_off['reports'])
# assert default['reports'] == collect_off['reports']
assert len(default["reports"]) == len(collect_off["reports"])
Loading