[Django] #25746: Isolate inlined test models
Django
------------------------------------------------+------------------------
Reporter: charettes | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Many test in Django's own test suite inline model definition in order to
prevent table creation or simply isolate them.
Unfortunately most of these model don't explicitly declare an explicit
`apps` and end up being registered to the global `apps` instance. This has
three downsides:
1. The call to `register_model` busts the `apps` and registered models
(1k+) `_meta` cache. I didn't run any benchmark here but I suspect this
slow down things a bit.
2. If a model is already registered for the specified
`app_label.ModelName` (see the `schema` issue detailed in #25745) a
`RuntimeWarning` is raised.
3. If no model is registered for this particular `app_label.ModelName` any
future call to `migrate` will end up creating the table since we don't
have explicit migration defined for the test apps.
To prevent the enumerated issues the inlined models should use a per-test
isolated registry. Examples of such a pattern can be found on this
[https://github.com/django/django/pull/5613 PR].
e.g.
{{{#!python
def test_foo(self):
test_apps = Apps(['test_app'])
class Foo(models.Model):
class Meta:
apps = test_apps
}}}
In order to reduce the boiler plate required to achieve isolation I
suggest introducing a test decorator/context manager that register models
declared in it's body in an isolated `Apps()` instance.
e.g.
{{{#!python
@isolate_apps('test_app', 'test_app_2')
def test_foo(self):
class Foo(models.Model):
pass
with isolate_apps('test_app3') as apps:
class Bar(models.Model):
class Meta:
app_label = 'test_app3'
assert Bar is apps.get_model('test_app3', 'Bar')
}}}
This would also make fixing the actual and future isolation problems quite
easy by simply decorating the faulty test.
--
Ticket URL: <https://code.djangoproject.com/ticket/25746>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* owner: nobody => charettes
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:1>
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Old description:
New description:
Many tests in Django's own test suite inline model definition in order to
e.g.
e.g.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:2>
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/5687 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:3>
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:4>
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_docs: 1 => 0
Comment:
Added docs and rebased on top of master.
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:5>
Django
Reporter: charettes | Owner: charettes
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:6>
Django
Reporter: charettes | Owner: charettes
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a08fda2111d811aa53f11218fa03f3300dfff4cb" a08fda21]:
{{{
#!CommitTicketReference repository=""
revision="a08fda2111d811aa53f11218fa03f3300dfff4cb"
Fixed #25746 -- Isolated inlined test models registration.
Thanks to Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:8>
Django
Reporter: charettes | Owner: charettes
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"7bb373e3097fe8e000e0bba005ff2dcfc18ab9a5" 7bb373e3]:
{{{
#!CommitTicketReference repository=""
revision="7bb373e3097fe8e000e0bba005ff2dcfc18ab9a5"
Refs #25746 -- Added a test utility to isolate inlined model registration.
Thanks to Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25746#comment:7>