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.

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

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

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

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

Are you sure you want to change the base?

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

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