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.

nicoddemus · 2024-10-01T22:03:56Z