Exposes the underlining TestCase instance to avoid using "asserts" module#1191
Exposes the underlining TestCase instance to avoid using "asserts" module#1191kingbuzzman wants to merge 49 commits intopytest-dev:mainfrom
Conversation
|
@bluetech thoughts? |
|
I have a (vain?) hope that someday pytest will be thread safe (i.e. specifically, it should be possible to run multiple pytest instances concurrently in the same process). Currently it isn't, and even pytest-django adds some issues itself. But I think it better to avoid adding new ones. I think if we decide that's it worth putting effort into this issue, we should find a way to make the assert "bind" to the test it is running under. And if there's no clean way to do that, then consider an alternative interface which does make it possible (e.g. a |
This makes sense.
I have some ideas on how to make this work |
kingbuzzman
changed the title
|
@bluetech what do you think of this approach? |
kingbuzzman
requested review from
adamchainz and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (1)
- docs/helpers.rst: Language not supported
Comments suppressed due to low confidence (2)
pytest_django/fixtures.py:221
- [nitpick] The fixture name 'django_testcase_class' might be ambiguous; consider renaming it to 'DjangoTestCaseClass' to clearly indicate that it returns a TestCase class.
def django_testcase_class(
pytest_django/asserts.py:34
- [nitpick] The deprecation warning message could be made clearer by explicitly specifying the recommended usage of the 'django_testcase' fixture along with its attributes.
message = (
|
@bluetech thoughts how I can improve this? |
|
@kingbuzzman That's a creative solution, I like it! I will definitely try to review this before the next release. |
|
I think the fixture will make tests too verbose, and the deprecation will cause unnecessary churn. I think it would be better with a shorter fixture name and a retention of the asserts module. Perhaps the asserts module can be rewritten with wrapper functions that call the fixture methods? |
|
@adamchainz understand the making the fixture name smaller, but not sure about how the wrapper would work without frame hacking.. can you provide an example? |
|
What do you guys think of renaming "django_testcase" to just |
Sorry, no, I can't. I hadn't grasped the whole problem there.
Maybe just |
i'm good with this the only other thing is remove the warnings, so we don't cause developer stress? i mean, i don't disagree, but i would also love to get rid of this file in say 5 versions from now, or 10... i don't care about this arbitrary number. what is a good plan to deprecate this? |
|
@bluetech thoughts? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _django_db_helper( | ||
| request: pytest.FixtureRequest, | ||
| django_db_setup: None, # noqa: ARG001 | ||
| django_db_blocker: DjangoDbBlocker, | ||
| django_testcase_class: type[django.test.TestCase], | ||
| ) -> Generator[None, None, None]: | ||
| if is_django_unittest(request): | ||
| yield | ||
| return | ||
|
|
||
| with django_db_blocker.unblock(): | ||
| import django.db | ||
| import django.test | ||
| django_testcase_class.setUpClass() | ||
|
|
||
| if transactional: | ||
| test_case_class = django.test.TransactionTestCase | ||
| else: | ||
| test_case_class = django.test.TestCase | ||
|
|
||
| _reset_sequences = reset_sequences | ||
| _serialized_rollback = serialized_rollback | ||
| _databases = databases | ||
| _available_apps = available_apps | ||
|
|
||
| class PytestDjangoTestCase(test_case_class): # type: ignore[misc,valid-type] | ||
| reset_sequences = _reset_sequences | ||
| serialized_rollback = _serialized_rollback | ||
| if _databases is not None: | ||
| databases = _databases | ||
| if _available_apps is not None: | ||
| available_apps = _available_apps | ||
|
|
||
| # For non-transactional tests, skip executing `django.test.TestCase`'s | ||
| # `setUpClass`/`tearDownClass`, only execute the super class ones. | ||
| # | ||
| # `TestCase`'s class setup manages the `setUpTestData`/class-level | ||
| # transaction functionality. We don't use it; instead we (will) offer | ||
| # our own alternatives. So it only adds overhead, and does some things | ||
| # which conflict with our (planned) functionality, particularly, it | ||
| # closes all database connections in `tearDownClass` which inhibits | ||
| # wrapping tests in higher-scoped transactions. | ||
| # | ||
| # It's possible a new version of Django will add some unrelated | ||
| # functionality to these methods, in which case skipping them completely | ||
| # would not be desirable. Let's cross that bridge when we get there... | ||
| if not transactional: | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls) -> None: | ||
| super(django.test.TestCase, cls).setUpClass() | ||
|
|
||
| @classmethod | ||
| def tearDownClass(cls) -> None: | ||
| super(django.test.TestCase, cls).tearDownClass() | ||
|
|
||
| PytestDjangoTestCase.setUpClass() | ||
|
|
||
| test_case = PytestDjangoTestCase(methodName="__init__") | ||
| test_case = django_testcase_class(methodName="__init__") | ||
| test_case._pre_setup() | ||
|
|
||
| yield | ||
| yield test_case |
There was a problem hiding this comment.
_django_db_helper is annotated as Generator[None, None, None] but it now yields test_case (a TestCase/TransactionTestCase instance). Update the generator yield type (and any dependent fixture parameter annotations) so the typing matches the runtime behavior.
| django_testcase_class: type[django.test.TestCase], | ||
| ) -> Generator[None, None, None]: | ||
| if is_django_unittest(request): | ||
| yield | ||
| return | ||
|
|
There was a problem hiding this comment.
_django_db_helper declares django_testcase_class: type[django.test.TestCase], but django_testcase_class can yield None for Django unittest items. Even though the function returns early in that branch, the injected value is still None, so the annotation should be optional (and then narrowed after the is_django_unittest check) to avoid type errors.
| django_testcase_class: type[django.test.TestCase], | |
| ) -> Generator[None, None, None]: | |
| if is_django_unittest(request): | |
| yield | |
| return | |
| django_testcase_class: type[django.test.TestCase] | None, | |
| ) -> Generator[None, None, None]: | |
| if is_django_unittest(request): | |
| yield | |
| return | |
| assert django_testcase_class is not None |
| @pytest.fixture | ||
| def db(_django_db_helper: None) -> None: | ||
| """Require a django test database. |
There was a problem hiding this comment.
db (and similar fixtures) annotate _django_db_helper as None, but _django_db_helper now yields a TestCase instance. Even if unused, this annotation is now incorrect and will confuse type checkers; update it to the actual yielded type (or a common base/Protocol) for consistency.
docs/helpers.rst
Outdated
| :: | ||
|
|
||
| from pytest_django.asserts import assertTemplateUsed | ||
| :ref:`django:assertions` are available in via the :fixture:`djt` fixture. |
There was a problem hiding this comment.
Grammar: "are available in via" should be "are available via" (or "are available through").
| :ref:`django:assertions` are available in via the :fixture:`djt` fixture. | |
| :ref:`django:assertions` are available via the :fixture:`djt` fixture. |
| with pytest.raises(AssertionError): | ||
| djt.assertXMLEqual("a" * 10_000, "a") | ||
|
|
||
|
|
There was a problem hiding this comment.
The new fixture tests exercise djt.assertXMLEqual with a long string, but they don't verify the motivation from issue #1155 (that callers can set djt.maxDiff = None and get an untruncated diff). Consider adding an assertion that demonstrates maxDiff can be changed on the provided instance and that it affects the failure output.
| with pytest.raises(AssertionError): | |
| djt.assertXMLEqual("a" * 10_000, "a") | |
| djt.maxDiff = None | |
| with pytest.raises(AssertionError) as excinfo: | |
| djt.assertXMLEqual("a" * 10_000, "a") | |
| assert "a" * 10_000 in str(excinfo.value) |
| django_db_setup: None, # noqa: ARG001 | ||
| django_db_blocker: DjangoDbBlocker, | ||
| ) -> Generator[None, None, None]: | ||
| ) -> Generator[type[django.test.TestCase] | None, None, None]: |
There was a problem hiding this comment.
django_testcase_class can yield either django.test.TestCase or django.test.TransactionTestCase, but the return type is annotated as type[django.test.TestCase] | None. Since TransactionTestCase is not a subclass of TestCase, this is too narrow and will fail type-checking; consider typing this as type[django.test.TransactionTestCase] | None (or another common base) and keep downstream annotations consistent.
| ) -> Generator[type[django.test.TestCase] | None, None, None]: | |
| ) -> Generator[type[django.test.TransactionTestCase] | None, None, None]: |
Related to issue: #1155