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

FreerGit · 2024-09-12T17:37:59Z

Allows a user to define whether or not pytest should treat imported (but not in testpaths) as test classes.
Imagine a class is called TestFoo defined in src/ dir, when discover_imports is disabled (default), TestFoo is not treated as a test class.

- Allows a user to define whether or not pytest should treat imported (but not in testpaths) as test classes. - Imagine a class is called TestFoo defined in src/ dir, when discover_imports is disabled, TestFoo is not treated as a test class.

for more information, see https://pre-commit.ci

RonnyPfannschmidt

Thanks for getting this started

We have to do some considerations for backwards compatibility and type safety

Also I'm not yet sure if the selection approach is generally intuitive (it solving your usecase nicely

RonnyPfannschmidt · 2024-09-12T19:27:02Z

src/_pytest/main.py

+    parser.addini(
+        "discover_imports",
+        "Whether to discover tests in imported modules outside `testpaths`",
+        default=False,


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

Alternatively none with a informative warning

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.

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.

RonnyPfannschmidt · 2024-09-12T19:31:09Z

src/_pytest/python.py

@@ -741,6 +741,12 @@ def newinstance(self):
        return self.obj()

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


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

I think changing the default value for the option to False and changing this to if self.config.getinit("discover_imports"): should be enough.

FreerGit · 2024-09-12T19:42:11Z

@RonnyPfannschmidt Hi! Thanks for reviewing :)

To be clear if I would set True to default (as you suggested) this would mean that the base case is that a class like "Testament" in a seperate folder would make pytest error out (see #12749). Then a user would opt in (set to false) to make sure that the discovery does not happen for those classes. Your suggestion seems more reasonable I think.

I saw some checks failed, which didn't happen locally for some reason.
I will look over your comments and try and solve this tommorrow!

nicoddemus · 2024-09-12T22:35:27Z

src/_pytest/main.py

@@ -78,6 +78,11 @@ def pytest_addoption(parser: Parser) -> None:
        type="args",
        default=[],
    )
+    parser.addini(
+        "discover_imports",


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

nicoddemus · 2024-09-12T22:36:34Z

changelog/12749.feature.rst

@@ -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.


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

treat_imports_as_tests?
imports_are_tests?
crawl_imports?

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.

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!

nicoddemus · 2024-09-12T22:41:18Z

I also wonder if we should be using a more general approach, and not only classes: shouldn't we also account for test_ functions imported from somewhere else (src)?

nicoddemus · 2024-09-13T10:37:17Z

Thought about this some more:

I think this should actually be expanded to the general idea/feature of "pytest will only collect objects from modules if they are defined in that module" (by inspecting its __module__ attribute). So if the user enables that option, classes/functions that appear in a module will only be collected if their __module__ points to that module.

nicoddemus · 2024-09-15T11:48:50Z

Another thing that occurred to me: __test__ should also be handled by the new option correctly as way to opt-in for something to be considered a test, regardless of where it was defined.

@pytest-dev/core how do you feel about this feature, should we go ahead with it or are there other concerns to discuss?

Zac-HD · 2024-09-15T22:45:50Z

I'm completely unsurprised that classes or functions imported into a test file are collected, but I'm also obviously atypical of pytest users, and so having a setting for this could make sense.

If we do, I think the setting should be named collect_imported_tests; it's not about discovery at all.

I think this should actually be expanded to the general idea/feature of "pytest will only collect objects from modules if they are defined in that module" (by inspecting its __module__ attribute). So if the user enables that option, classes/functions that appear in a module will only be collected if their __module__ points to that module.

Agree with all of this, __test__ = definitely takes priority over a global option. I'd reverse the phrasing a little though; we should collect anything where __module__ is unset or none even though that doesn't match the current module.

Finally, I'm concerned that this opens up another avenue for the worst kind of testing bug, where you think you have tests but they can't actually fail (because they're not collected!) and so the whole exercise is silently useless. I'm unsure whether the benefits are worth this risk, given that there are plenty of other workarounds based on our existing features - including collection patterns, plugins, etc.

nicoddemus · 2024-09-21T13:05:57Z

I'm unsure whether the benefits are worth this risk, given that there are plenty of other workarounds based on our existing features - including collection patterns, plugins, etc.

I'm not sure about this hiding bugs etc (the behavior seems somewhat more sane for those not used to how pytest works I think), but of course I might be wrong. However I do agree with the general sentiment of "yet another flag" that this brings. In this case however seems reasonable to me (but not that strongly TBH), because if I were designing pytest from the ground up today, without backward compatibility concerns, I would vote that it should behave this way (only collecting classes/functions defined in their own module).

wpietri · 2024-09-23T12:02:09Z

Yeah, as the report of this issue, I found it wildly surprising, and spent a lot of time trying to find a fix. I'd rather have there be a clear option for this, as that's what I first looked for. But failing that, "plenty of other workarounds" was part of the problem for me, as I had to try a lot of stuff and couldn't find one that let me fix exactly the issue once in a clear and clean way. I am fine with me having to fix it once, but didn't want it to be something every engineer had to learn and remember every time they created a new domain class (or function) with test in the name.

So if something like this doesn't make it in, then I'd at least suggest a clear, official workaround recommendation the docs where people can solve it in one spot permanently for their project.

FreerGit · 2024-09-24T12:36:21Z

There seems to be some disagreement about the issue, either way I pushed the new implementation. Maybe it helps in the discussion :)

nicoddemus · 2024-09-26T14:30:13Z

src/_pytest/main.py

        self.trace("genitems", node)
        if isinstance(node, nodes.Item):
            node.ihook.pytest_itemcollected(item=node)
+            if self.config.getini("collect_imported_tests"):


The logic for the variable is inverted: if collect_imported_tests is True, we are actually excluding imported tests instead of collecting them.

So the default for collect_imported_tests should be True for backward compatibility as discussed previously, and the logic here should be inverted:

Suggested change

if self.config.getini("collect_imported_tests"):

if not self.config.getini("collect_imported_tests"):

nicoddemus · 2024-09-26T14:30:39Z

src/_pytest/main.py

            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"):


My other comment applies here as well.

nicoddemus · 2024-09-26T14:32:30Z

changelog/12749.feature.rst

@@ -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.


As per my comment, this description needs an update:

Suggested change

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 True will make pytest collect classes/functions from test files only if they are defined in that file (as opposed to imported there).

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.

nicoddemus

Thanks @Bjwebb,

Unfortunately we are not quite there yet.

However feel free to hold on to updating the PR in case other maintainers want to comment further before we move this along.

doc/en/reference/reference.rst

src/_pytest/main.py

testing/test_collect_imports.py

- pytest_collectreport hook currently gets more reports (TODO) - `pytest_collection_modifyitems` and `pytest_itemcollected` is correct.

testing/test_collect_imports.py

We should not collect classes at the module level when the config is set. Doing so is not only the correct thing, as we will not collect the class, but also lets us avoid having the same logic at the Class collector (as it will not be collected at all now).

nicoddemus · 2024-12-01T12:28:28Z

Thanks @FreerGit for the patience and sorry for the delay!

I changed your original implementation a bit: we only need to prevent the collection of classes/functions at the module level, because at that point they will not even appear as a Class later for their methods to be collected. This simplifies the implementation and tests significantly.

Also improved the docs a bit providing an example for easier understanding.

Thanks again!

@RonnyPfannschmidt could you review it too? Thanks.

nicoddemus · 2024-12-01T12:36:58Z

Pinging @Zac-HD in case he wants to review too. 👍

Zac-HD

Looks great - thanks everyone!

nicoddemus · 2024-12-01T21:19:04Z

@Zac-HD please remember to squash next time. 😢

Also I would have liked for @RonnyPfannschmidt's review before merging.

Zac-HD · 2024-12-02T03:59:52Z

Gah, sorry! I was rushing out and should have left it for when I had more time 😓

nicoddemus · 2024-12-02T10:24:42Z

No worries, it happens. 👍

FreerGit · 2024-12-03T19:57:51Z

Great to see this PR getting merged! Thanks for all the help and appreciate the cleanup @nicoddemus , admittedly the PR required more understanding of the internals than I first thought...

Anyhow, great to see it move along :)

Add discover_imports in conf

222457d

- Allows a user to define whether or not pytest should treat imported (but not in testpaths) as test classes. - Imagine a class is called TestFoo defined in src/ dir, when discover_imports is disabled, TestFoo is not treated as a test class.

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 12, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

fa3b631

for more information, see https://pre-commit.ci

RonnyPfannschmidt reviewed Sep 12, 2024

View reviewed changes

nicoddemus reviewed Sep 12, 2024

View reviewed changes

Snoopy1866 mentioned this pull request Sep 24, 2024

[bug] pytest collect class 'TestType' in src folder by mistake Snoopy1866/pystatpower#41

Open

FreerGit added 2 commits September 24, 2024 14:28

collect_imported_tests option

68ac4a1

merge: rename from discover to collect

a6ee0bc

nicoddemus requested changes Sep 26, 2024

View reviewed changes

update default and add docs

eb8592c

nicoddemus requested changes Oct 1, 2024

View reviewed changes

nicoddemus reviewed Oct 4, 2024

View reviewed changes

testing/test_collect_imports.py Outdated Show resolved Hide resolved

WIP: don't collect instead of filtering out

935c06d

FreerGit commented Nov 4, 2024

View reviewed changes

testing/test_collect_imports.py Outdated Show resolved Hide resolved

FreerGit added 2 commits November 12, 2024 20:00

WIP: modified items, reports and items collected

191456e

- pytest_collectreport hook currently gets more reports (TODO) - `pytest_collection_modifyitems` and `pytest_itemcollected` is correct.

WIP - report collection needs triage

f1821ea

FreerGit commented Nov 17, 2024

View reviewed changes

testing/test_collect_imports.py Outdated Show resolved Hide resolved

Ensure correct collection of reports

022d316

nicoddemus added this to the 8.4 milestone Dec 1, 2024

nicoddemus added 2 commits December 1, 2024 09:03

Improve docs

e2ec64e

nicoddemus approved these changes Dec 1, 2024

View reviewed changes

nicoddemus requested a review from RonnyPfannschmidt December 1, 2024 12:28

nicoddemus added 2 commits December 1, 2024 09:30

Pass tests directory explicitly

43865ed

Rename test helper

d2327d9

Zac-HD approved these changes Dec 1, 2024

View reviewed changes

Zac-HD merged commit 9d4f36d into pytest-dev:main Dec 1, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

FreerGit commented Sep 12, 2024

RonnyPfannschmidt left a comment

RonnyPfannschmidt Sep 12, 2024

wpietri Sep 12, 2024

nicoddemus Sep 12, 2024

RonnyPfannschmidt Sep 12, 2024

nicoddemus Sep 12, 2024

FreerGit commented Sep 12, 2024 •

edited

Loading

nicoddemus Sep 12, 2024

nicoddemus Sep 12, 2024

FreerGit Sep 13, 2024

nicoddemus Sep 13, 2024 •

edited

Loading

FreerGit Sep 13, 2024

nicoddemus commented Sep 12, 2024

nicoddemus commented Sep 13, 2024

nicoddemus commented Sep 15, 2024 •

edited

Loading

Zac-HD commented Sep 15, 2024

nicoddemus commented Sep 21, 2024

wpietri commented Sep 23, 2024

FreerGit commented Sep 24, 2024

nicoddemus Sep 26, 2024

nicoddemus Sep 26, 2024

nicoddemus Sep 26, 2024

nicoddemus left a comment

nicoddemus commented Dec 1, 2024

nicoddemus commented Dec 1, 2024 •

edited

Loading

Zac-HD left a comment

nicoddemus commented Dec 1, 2024

Zac-HD commented Dec 2, 2024

nicoddemus commented Dec 2, 2024

FreerGit commented Dec 3, 2024

		@@ -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.

	if self.config.getini("collect_imported_tests"):
	if not self.config.getini("collect_imported_tests"):

		@@ -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.

-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 True will make pytest collect classes/functions from test files only if they are defined in that file (as opposed to imported there).

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

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

Conversation

FreerGit commented Sep 12, 2024

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FreerGit commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Sep 12, 2024

nicoddemus commented Sep 13, 2024

nicoddemus commented Sep 15, 2024 • edited Loading

Zac-HD commented Sep 15, 2024

nicoddemus commented Sep 21, 2024

wpietri commented Sep 23, 2024

FreerGit commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Dec 1, 2024

nicoddemus commented Dec 1, 2024 • edited Loading

Zac-HD left a comment

Choose a reason for hiding this comment

nicoddemus commented Dec 1, 2024

Zac-HD commented Dec 2, 2024

nicoddemus commented Dec 2, 2024

FreerGit commented Dec 3, 2024

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

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

FreerGit commented Sep 12, 2024 •

edited

Loading

nicoddemus Sep 13, 2024 •

edited

Loading

nicoddemus commented Sep 15, 2024 •

edited

Loading

nicoddemus commented Dec 1, 2024 •

edited

Loading