[Django] #23746: Remedy assertNumQueries dependence on some caches state
Django
-----------------------------------+--------------------
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
The `assertNumQueries` manager / method may return different results
depending on what is in various caches. Consequently, some tests using it
to rely on test ordering, particularly those that count some content type
queries. For instance if you [https://code.djangoproject.com/ticket/23742
run the suite in reverse] you'll see `test_group_permission_performance`
failing due to `ContentType` being or not being asked about `auth.Group`
in some preceding test ^[#f1 1]^.
[https://github.com/wrwrwr/django/compare/fix/assert-num-queries-cache-
dependence The proposed solution] resets "query" caches on entering the
manager (with a switch to disable it for actual caching tests). After
introducing the change 10 tests started to fail (`ContentType` cache tests
and some `prefetch_related` tests), 2 specific workarounds were no longer
necessary (#17377, #20432) and a failure from
[https://github.com/django/django/pull/3426 PR 3426] disappeared ^[#f2
2]^.
\\
Note to prospective reviewers: Are there any other caches that should be
cleared besides the `ContentType` and `Site`? Related objects caches
maybe? Do the affected `prefetch_related` operations actually require
content type queries? Would it be better to clear all the caches after
every test case to avoid other kinds of dependencies?
![1][[=#f1]] What happens in the likewise failing with "--reverse"
`test_user_permission_performance` is a bit intriguing and probably more
involved -- two identical content type queries, savepoint, content type
insertion and a release?
![2][[=#f2]] Only tested with Python 3.3 / Postgres,
--
Ticket URL: <https://code.djangoproject.com/ticket/23746>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
The `assertNumQueries` manager / method may return different results
depending on what is in various caches. Consequently, some tests using it
rely on test ordering, particularly those that count some content type
queries. For instance if you [https://code.djangoproject.com/ticket/23742
run the suite in reverse] you'll see `test_group_permission_performance`
failing due to `ContentType` being or not being asked about `auth.Group`
in some preceding test ^[#f1 1]^.
[https://github.com/wrwrwr/django/compare/fix/assert-num-queries-cache-
dependence The proposed solution] resets "query" caches on entering the
manager (with a switch to disable it for actual caching tests). After
introducing the change 10 tests started to fail (`ContentType` cache tests
and some `prefetch_related` tests), 2 specific workarounds were no longer
necessary (#17377, #20432) and a failure from
[https://github.com/django/django/pull/3426 PR 3426] disappeared ^[#f2
2]^.
\\
Note to prospective reviewers: Are there any other caches that should be
cleared besides the `ContentType` and `Site`? Related objects caches
maybe? Do the affected `prefetch_related` operations actually require
content type queries? Would it be better to clear all the caches after
every test case to avoid other kinds of dependencies?
![1][[=#f1]] What happens in the likewise failing with "--reverse"
`test_user_permission_performance` is a bit intriguing and probably more
involved -- two identical content type queries, savepoint, content type
insertion and a release?
![2][[=#f2]] Only tested with Python 3.3 / Postgres,
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:1>
Django
-----------------------------------+------------------------------------
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
* type: Bug => New feature
Comment:
The idea seems good, but rather than hard-coding a list of functions in
`clear_query_caches()` I think it would be better to have some way to
register a function to be included in that list. That way third-party code
that uses caching can also take advantage of the functionality.
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:2>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: 1 => 0
Comment:
A somewhat [https://github.com/wrwrwr/django/compare/ticket_23746?expand=1
different solution], with the same effect:
* added a new `pre_capture_queries` signal sent when entering the
`CaptureQueriesContext` (also for the base manager not only
`assertNumQueries`);
* dropped the `clear_caches` toggle as in the new context it seemed to
general (which caches?); if you really want to miss some queries,
selectively disconnecting their respective receivers is a less convenient,
but more fine-grained option.
There are some other possibilities to consider: -- a more general non-
testing-only `clear_caches` signal, -- some `register` function on the
test runner, or just bringing back the toggle for convenience.
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:3>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by wrwrwr):
Just one more note on this one, recently I've came across #11505 (there
are also some discussions on the subject:
[https://groups.google.com/forum/#!topic/django-users/OOGO-3MIO_c 7/09],
[https://groups.google.com/forum/#!topic/django-
developers/zlaPsP13dUY/discussion 7/11],
[https://groups.google.com/forum/#!msg/django-
developers/Ex7X2oFAHoc/TTma3dJU1ccJ 10/11]). This ticket could be replaced
by generalizing the cache clearing / restoring to include site / content
type caches and possibly some of the lru_caches.
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:4>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"7cd3f1c29595d1da7f37d29e7c3bc6a7a314cd1d"]:
{{{
#!CommitTicketReference repository=""
revision="7cd3f1c29595d1da7f37d29e7c3bc6a7a314cd1d"
Fixed cache state dependence for assertNumQueries in
test_group_permission_performance.
Refs #20432 and #23746.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:5>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
I think it would be interesting to see a benchmark of adding a similar
signal in `SimpleTestCase._pre_setup()` to clear the sites and
contenttypes caches before each test. How much would that increase the
runtime of the test suite? If we went with that approach, we probably
wouldn't need the `pre_capture_queries` signal.
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:6>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: timgraham (added)
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:7>
Django
Reporter: Wojtek Ruszczewski | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23746#comment:8>