Skip to content

4680 Instrument tests to detect bad DB access patterns - #4681

Draft
andrew8er wants to merge 1 commit into
improve-test-setupfrom
4680-instrument-tests
Draft

andrew8er wants to merge 1 commit into
improve-test-setupfrom
4680-instrument-tests

Conversation

@andrew8er

@andrew8er andrew8er commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Short description

Instruments tests with two DB query related functionality:

  • Include https://github.com/taobojlen/django-zeal in tests to detect and flag N+1 usage patterns per view (currently with budgets set to our current usage patterns).
  • Ability to generate a detailed query report for every test and route. This should help us to detect bad DB access patterns.

Proposed changes

Instrument tests for django-zeal and query-report generation.

Side effects

The currently existing query budgets per view are now a hard limit. We could optionally make this a warning only, but I suspect, these will go unheeded, as we currently have LOTS of warnings in the tests that we just ignore.

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

How to test

  • Run normal tests (with django-zeal query budgets): tools/test.py
  • Run tests with query report generation: tools/test.py --query-report

Resolved issues

Fixes: #4680


Pull Request Review Guidelines

@andrew8er
andrew8er changed the base branch from develop to improve-test-setup November 3, 2026 12:42
@andrew8er andrew8er linked an issue Nov 3, 2026 that may be closed by this pull request
7 tasks
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch 2 times, most recently from 2c8f68b to 14b3f0b Compare November 3, 2026 13:10
@andrew8er
andrew8er added this pull request to stack #4623 November 3, 2026 13:36
@andrew8er
andrew8er marked this pull request as ready for review November 3, 2026 13:43
@andrew8er andrew8er added this to the Roadmap T44K milestone Nov 4, 2026
@andrew8er andrew8er moved this to In Review in Integreat 💛 Nov 4, 2026
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch from 14b3f0b to efa2e41 Compare November 4, 2026 14:09
@deliverino

deliverino Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

LLM PR Review (verdigado-code)

Analyzed diff, but not the whole repository. Important context might be missing.

Scope

The PR contains only what the description states: django-zeal in the test settings/conftest, the opt-in query report package, budget tests, docs and the dev dependency. No production, API, model, migration or i18n code is touched, so no release note is needed (internal test change).

Correctness

  • tests/query_report/capture.py:136 – _on_request_started(environ=None, **_) reads environ["PATH_INFO"], but current Django sends request_started with a request keyword argument (the old environ argument has been gone for years). If that matches the installed Django, environ is always None, resolve("") raises Resolver404, and every client request is recorded with route "" – which would also make tests/query_report/test_capture.py:28 (test_records_route_for_client_request) fail, since it asserts the resolved route. Please verify against the Django version in uv.lock.
  • tests/query_report/capture.py:157-158,168-169 – record_queries() connects the request signals with a fixed dispatch_uid="query_report". The context is nestable (the plugin's per-phase context is active while tests/query_report/test_capture.py calls record_queries() directly, e.g. when running with --query-report), and a second connect() with the same uid is a no-op, so the inner context's disconnect() in its finally removes the handler while the outer context is still active. After the first such test, route attribution is silently disabled for the rest of that worker's run. A unique uid per context would avoid this.
  • integreat_cms/core/test_settings.py, ZEAL_ALLOWLIST – seven entries use "field": "get()" (e.g. cms.Language, cms.Page, cms.User, contenttypes.ContentType, linkcheck.Link, linkcheck.Url, cms.ExternalCalendar). get() is a queryset method, not a model field; if the allowlist is matched by model/field of a lazily loaded relation, these entries never match and the corresponding N+1s would still fail tests. Worth confirming the format against what django-zeal actually reports.
  • tests/conftest.py:~244 – the autouse zeal fixture also covers fixture setup, but only load_test_data_transactional (~line 128) is wrapped in zeal_ignore(). If any other fixture performs data loading or repeated lazy loads (e.g. a separate non-transactional load_test_data, API snapshot fixtures), the N+1 detection would fire on fixture code rather than the test. The diff only shows the transactional loader being excluded.
  • tests/query_report/analysis.py, merge_results() – per-test SQL is truncated to SQL_LENGTH (1000), but the suite-wide shapes aggregation reuses the full normalized statement as key/value, so query-report.json can contain untruncated multi-kilobyte statements (the HTML display truncates, the documented "for scripting" JSON does not).

Minor / nitpick

  • The N+1 threshold is duplicated: ZEAL_NPLUSONE_THRESHOLD = 5 in test_settings.py and N1_THRESHOLD: Final = 5 in tests/query_report/analysis.py. The docs say the two must match, but nothing enforces it.
  • tests/query_report/__init__.py makes the directory a package. If tests/ itself has no __init__.py, pytest will import these modules as query_report.test_* while tests/conftest.py imports tests.query_report.plugin – the same files then exist twice in sys.modules (two ContextVars, two itertools.counts). Probably harmless, but worth checking.
  • tests/query_report/test_plugin.py – test_registered_only_with_flag and test_controller_ignores_relayed_tests lack the pytest.mark.unit mark their sibling tests carry; if the suite treats unmarked tests differently (e.g. DB setup), this is inconsistent.
  • tests/cms/views/test_query_budgets.py – docstring references :attr:QUERY_BUDGETS``; a module-level constant is :data: in Sphinx.
  • nitpick: test_settings.py allowlist comment # e.g. (logging of) __init__.py:400 does not identify the module.

Conventions

No issues: all new Python files start with from __future__ import annotations (after the docstring), type hints are present, docstrings follow the Sphinx format, no new # noqa, and the commit message describes the change and its parts.

@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch from efa2e41 to 029cb0b Compare November 4, 2026 14:34
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch from 029cb0b to 3facc45 Compare November 4, 2026 17:06
@andrew8er
andrew8er marked this pull request as draft November 4, 2026 17:53
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch 3 times, most recently from 2633e7e to 6cd7991 Compare November 9, 2026 11:53
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch from 6cd7991 to 07c5db1 Compare November 9, 2026 20:37
- Opt-in --query-report pytest flag recording the queries of every test
  (counts, duplicates, N+1 candidates, slowest queries, call sites),
  merged across xdist workers into JSON and a static HTML report
- Fail tests on new N+1 queries with django-zeal, with an allowlist of
  the existing ones
- Pin the query counts of the CMS views with the most queries
@andrew8er
andrew8er force-pushed the 4680-instrument-tests branch from 07c5db1 to 9bee03b Compare November 10, 2026 18:01

This branch has not been deployed

No deployments
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.

[Task]: Instrument tests to generate a query report

1 participant