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.

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

Don't auto-discover tests in particular folders #12749

Open
wpietri opened this issue Aug 29, 2024 · 10 comments · May be fixed by #12810
Open

Don't auto-discover tests in particular folders #12749

wpietri opened this issue Aug 29, 2024 · 10 comments · May be fixed by #12810

Comments

@wpietri
Copy link

wpietri commented Aug 29, 2024

This inspired by a discussion: #12748

What's the problem this feature will solve?

We have a project that's about a different kind of test. We have a lot of classes with "Test" in the name, including some that start with "Test". We would also like to organize some of our pytest tests in classes whose names start with "Test". We distinguish them by directory; all our pytest tests are in tests, and all our production code is in src. We'd like an easy way to set this up with no distortion to our production code and minimal shenanigans in the test code, while getting no warnings.

Naively one might think that limiting discovery to a particular directory, as with pytest tests or testpaths=["tests"]. However, if a production file called TestFoo is imported from production code into test code, it will still be discovered, resulting in a warning like:

src/pytest_collection_issue/domain.py:6
  src/pytest_collection_issue/domain.py:6: PytestCollectionWarning: cannot collect test class 'Testament' because it has a __init__ constructor (from: tests/test_domain.py)
    class Testament(object):

Describe the solution you'd like

For me, the best solution is just to by default disable discovery beyond the requested testpaths. Trying to run something just because it's included from wherever strikes me as counterintuitive behavior. If somebody needs that behavior, perhaps an option like discover_imports would be useful.

The second-best solution would be an option where we can specify paths to definitely not discover things in. E.g.:

testpaths = ["tests"]
excludepaths = ["src"]

Alternative Solutions

There are many ways to solve this, including manually adding a __test__ = false to each production class, using a marker mixin like NotAPytestTest to achieve the same end, importing production classes into the tests under different names, doing only local imports, or renaming our test classes. All of these are a little clunky, and require people to remember a ritual under certain circumstances.

The solution I went with is to put

for a_class in [i[1] for i in (globals().items()) if inspect.isclass(i[1])]:
    if a_class.__name__.startswith('Test'):
        a_class.__test__ = False

after my domain imports and before my test classes. It still seems clunky to me, but only has to happen once per file of tests. I think it would be better still if this could be fixed in one spot, or have it work by default.

Additional context

Please see this GitHub discussion and this example repository that demonstrates the problem.

@FreerGit
Copy link

id like to fix this issue.

After reading the discussion I would propose something to the effect of:

Add exclude_paths to the config such that:
exclude_paths = [ "testing/ignore" ]
Would not consider any test (valid or not) in (proj_root)/testing/ignore.

In your specific case it would be:
exclude_paths = [ "src" ]
If this seems reasonable I'll submit a PR with the fix.

@nicoddemus
Copy link
Member

@FreerGit just to make sure we are on the same page, note that in the OP the test files are in tests, BUT they import classes from src, and those classes get collected because they start with Test.

@FreerGit
Copy link

FreerGit commented Sep 11, 2024

@nicoddemus In this case, the entire directory would not be picked up during collection. Thus never considered even if the imported.

To clarify:
Say I have a passing test in aa_test.py:

import ignore.test_never_ran

def test_answer():
    testament = ignore.test_never_ran.Testament()
    some_property = testament.personal_property()
    assert True
 

And then a test file in testing/ignore called test_never_ran.py,:

 class TestCollector:
    def test_always_fails(self) -> None:
        """This function, or any other tests in the `ignore` should never run. 
        The directory is set to be ignored by pytest"""
        assert 1 == 2
        
class Testament(object):
    """This class will not be collected even though it starts with `Test`"""
    def __init__(self):
        super().__init__()
        self.collections = ["stamp", "coin"]

    def personal_property(self):
        return [f"my {x} collection" for x in self.collections]

This would not fail (the tests would be ignored) with the excluded_paths strategy.

I believe we are on the same page :)

Edit:
I have the fix in place, but I do need to review the contribution method (im not familiar with open source tbh). Im going away to campus but I will try to get the PR in today so devs can review it :)

@nicoddemus
Copy link
Member

I believe we are on the same page :)

I might be wrong, but I don't think so.

In the OP scenario, the test files are not being ignored, they should be collected normally. The problem is the imported classes (from modules in src) starting with Test*, and being considered tests.

And then a test file in testing/ignore called test_never_ran.py,:

In your example you excluded testing/ignore, correct? But this is not the point of the OP, the testing directory should not be excluded at all, all test files in that directory should be collected. What the OP wants is that the classes imported by the test files that start with Test but are imported from the src directory to be excluded from the test run.

@nicoddemus
Copy link
Member

nicoddemus commented Sep 11, 2024

Perhaps an example helps:

src/
  foo.py
tests/
  test_foo.py
# content of foo.py
class Testament: ...
# content of test_foo.py
from foo import Testament

class TestTestament: 
   def test_foo(): ...

In this situation, the user executes:

$ pytest tests

And pytest collects test_foo.py, as expected, however it looks at the test_foo namespace and finds two classes, Testament and TestTestament: because both start with Test*, both are considered tests by pytest. The OP wants only for TestTestament to be considered a test by pytest.

The OP would like some way to signal that classes defined in src should not be considered tests automatically.

@FreerGit
Copy link

In your example you excluded testing/ignore, correct? But this is not the point of the OP,

Yes, the entire ignore dir is not collected.

My attempted solution was formed by:

The second-best solution would be an option where we can specify paths to definitely not discover things in. E.g.:

from OP.

The way I see it either a directory has to be explicitly ignored (what I proposed) or implicitly ignored by telling pytest to only considering tests from testpaths = ["tests"] and then internally excluding tests that are collected but not contained in tests. Which I believe is what you are describing (and OP in his first part of "Describe the solution you'd like").

Ill try and figure out the implicit route

@nicoddemus
Copy link
Member

The second-best solution would be an option where we can specify paths to definitely not discover things in. E.g.:

Just excluding some directories from collection today is already possible via norecursedirs.

I was channeling the OP here and assuming they meant that classes from these directories not to be collected (src), even if they appear in modules collected in other directories (tests).

@wpietri
Copy link
Author

wpietri commented Sep 11, 2024

Yes, @nicoddemus, that's what I'm looking for. In my example, Testament should not be treated as test, even though its file src/pytest_collection_issue/domain.py is imported from tests/test_domain.py.

And just so people understand that this comes from an actual need, this is for https://github.com/mlcommons/modelbench and https://github.com/mlcommons/modelgauge, where we're measuring the safety of LLMs. A Test is a core concept in the domain, so we legitimately have classes like Test and TestItem in the domain. We would ideally like to set one config option such that Test* is discovered in the test code only, not the main code.

@FreerGit
Copy link

Thanks for the help! Hope it wasn't too much back and forth.

Let me know there's any misunderstandings or problems.

@RonnyPfannschmidt
Copy link
Member

Having a opt in that turns imported Test classes into collect time skips might be a nice way to enable this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants