Opened 4 years ago

Last modified 3 years ago

#23746 new New feature

Allow assertNumQueries to clear caches before it runs

Reported by: Wojtek Ruszczewski Owned by: nobody
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: Tim Graham Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Wojtek Ruszczewski)

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 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 1.

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 PR 3426 disappeared 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] 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] Only tested with Python 3.3 / Postgres,

Change History (7)

comment:1 Changed 4 years ago by Wojtek Ruszczewski

Description: modified (diff)

comment:2 Changed 4 years ago by Tim Graham

Patch needs improvement: set
Summary: Remedy assertNumQueries dependence on some caches stateAllow assertNumQueries to clear caches before it runs
Triage Stage: UnreviewedAccepted
Type: BugNew feature

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.

comment:3 Changed 4 years ago by Wojtek Ruszczewski

Patch needs improvement: unset

A somewhat 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 too 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.

Last edited 4 years ago by Wojtek Ruszczewski (previous) (diff)

comment:4 Changed 4 years ago by Wojtek Ruszczewski

Just one more note on this one, recently I've came across #11505 (there are also some discussions on the subject: 7/09, 7/11, 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.

comment:5 Changed 3 years ago by Tim Graham <timograham@…>

In 7cd3f1c29595d1da7f37d29e7c3bc6a7a314cd1d:

Fixed cache state dependence for assertNumQueries in test_group_permission_performance.

Refs #20432 and #23746.

comment:6 Changed 3 years ago by Tim Graham

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.

comment:7 Changed 3 years ago by Tim Graham

Cc: Tim Graham added
Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top