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

gh-84867: Do not load tests from TestCase and FunctionTestCase #100497

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Dec 24, 2022

There are several changes:

  1. We no longer create suits for TestCase and FunctionTestCase when loading with loadTestsFromModule. I've added two tests cases for this. Previously it was like this for imported TestCase and FunctionTestCase in a module:
[<unittest.suite.TestSuite tests=[]>, <unittest.suite.TestSuite tests=[]>]

Now it is: []

  1. loadTestsFromTestCase(unittest.FunctionTestCase) caused a bug. FunctionTestCase expects a function, but it was given a 'runTest' string by map(testCaseClass, testCaseNames). And later it caused an exception. This is now also fixed, I've added a test case for it.

Inspired by #20237

@sobolevn
Copy link
Member Author

@csabella since your are the one who reviewed the previous PR, maybe you will be interested in reviewing this one as well? I understand that two years have passed, but nevertheless :)

@serhiy-storchaka serhiy-storchaka self-requested a review September 9, 2023 16:07
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I have one question.

Do you have some concrete use case for _shouldBeLoaded()? I do not like introducing yet one private method which can became de-facto a part of the interface.

I think that for now it would be better to inline _shouldBeLoaded() and remove test test_loadTestsFromModule__TestCase_subclass_custom. Later we can design more official way to declare TestCase subclass a "base class" which should not be loaded.

Lib/test/test_unittest/test_loader.py Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member Author

Do you have some concrete use case for _shouldBeLoaded()? I do not like introducing yet one private method which can became de-facto a part of the interface.

Yes, I had some unittest plugins in mind. Examples:

# Some typical django project's tests:
from django.test import (
    LiveServerTestCase,
    SimpleTestCase,
    TestCase,
    TransactionTestCase,
)

class MyTest(TestCase):
    def test_sum(self):
        self.assertEqual(1 + 1, 2)

Now, let try to load these tests:

In [1]: from server.apps.main import tests

In [2]: import unittest

In [3]: loader = unittest.TestLoader()

In [4]: loader.loadTestsFromModule(tests)
Out[5]: <unittest.suite.TestSuite tests=[<unittest.suite.TestSuite tests=[]>, <unittest.suite.TestSuite tests=[<server.apps.main.tests.MyTest testMethod=test_sum>]>, <unittest.suite.TestSuite tests=[]>, <unittest.suite.TestSuite tests=[]>, <unittest.suite.TestSuite tests=[]>]>

I also know other projects that define unittest-style test helpers, including my own ones: https://github.com/wemake-services/django-test-migrations/blob/553725f88d8927a7be6b7d0ca916aac393c5a053/django_test_migrations/contrib/unittest_case.py#L13

But, I think it is fine to inline this for now, so we can later design a better API and refactor these changes to use it instead.

@serhiy-storchaka
Copy link
Member

Let keep this change more narrow.

The idea about official method to exclude the class from automatic test loading already was discussed. Try to search in old issues. Although the issue where it was discussed may be closed.

Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes awaiting merge and removed needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes awaiting core review labels Sep 12, 2023
@serhiy-storchaka serhiy-storchaka merged commit 66d1d7e into python:main Sep 12, 2023
26 checks passed
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Sep 12, 2023
@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 12, 2023

GH-109327 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Sep 12, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 12, 2023

GH-109328 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Sep 12, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2023
serhiy-storchaka pushed a commit that referenced this pull request Sep 12, 2023
Oct 2, 2023
…GH-100497) (#109327)

gh-84867: Do not load tests from TestCase and FunctionTestCase (GH-100497)
(cherry picked from commit 66d1d7e)

Co-authored-by: Nikita Sobolev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants