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

[FreerGit]

closes #12749

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

[FreerGit]

@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 https://github.com/pytest-dev/pytest/issues/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]

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]

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]

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]

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]

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]

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]

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

[nicoddemus]

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]

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

[nicoddemus]

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

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

[Zac-HD]

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

[nicoddemus]

No worries, it happens. :+1:

[FreerGit]

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