From da446b6cc2911c37ae78631927e13cc9da03334f Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 31 Jul 2026 17:52:23 +0100 Subject: [PATCH 1/3] Document N+1 query detection in integration tests Companion to flarum/framework#4871, which fails integration tests whose requests run N+1 queries. Explains how to read a failure, what the binding count means, and how to exempt a legitimately repeated query shape. --- docs/extend/testing.md | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/docs/extend/testing.md b/docs/extend/testing.md index 3633477f0..59d039d3f 100644 --- a/docs/extend/testing.md +++ b/docs/extend/testing.md @@ -421,6 +421,53 @@ This is an extreme edge case, but note that MySQL does not update the fulltext i ::: +#### N+1 Query Detection + +Requests sent with `send()` are checked for N+1 query patterns, and **the test fails if one is found**. An N+1 is a single query shape executed once per record — a relationship loaded per model, a permission check per row, an API field that hits the database for each item. It looks harmless on a test fixture of three records and becomes hundreds of queries on a real forum. + +A failure looks like this: + +``` +GET /api/posts ran repeated queries — likely an N+1. + + 10x (10 distinct bindings): select * from `warnings` where `warnings`.`post_id` = ? +``` + +The two numbers say different things: + +- **Distinct bindings ≈ the repeat count** — one query per record. This is the N+1: load the data for the whole page instead, with [eager loading](api.md#eager-loading), a batched relationship, or one grouped query. +- **One distinct binding, many repeats** — the same rows fetched over and over. Not an N+1, but usually worth memoising. + +If a repetition is genuinely necessary, exempt that one query shape rather than switching the check off, so the rest of the request stays covered: + +```php +class MyTest extends TestCase +{ + protected function allowedRepeatedQueries(): array + { + return [ + // The lifecycle test deliberately re-reads the token each request. + 'from `access_tokens`', + ]; + } +} +``` + +Each entry is matched as a substring of the normalised SQL (literals and `IN` lists replaced), so `'from `access_tokens`'` covers every query against that table. + +Two blunter switches exist for when that isn't enough — a whole test case: + +```php +protected function detectsRepeatedQueries(): bool +{ + return false; +} +``` + +and a whole run, `FLARUM_DETECT_REPEATED_QUERIES=0 composer test:integration`, which is mostly useful when bisecting an unrelated failure. The repetition threshold can be raised with `FLARUM_REPEATED_QUERY_THRESHOLD` or by overriding `repeatedQueryThreshold()`. + +Batched queries never trigger the check: an `IN (…)` list is recognised as one shape regardless of how many ids it carries, which is exactly what eager loading produces. + #### Console Tests If you want to test custom console commands, you can extend `Flarum\Testing\integration\ConsoleTestCase` (which itself extends the regular `Flarum\Testing\integration\TestCase`). It provides 2 useful methods: From 74fe3b9a042c26b4621b187c601d5b62c64a6ea3 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 31 Jul 2026 21:02:57 +0100 Subject: [PATCH 2/3] Distinguish N+1 failures from non-scaling query warnings Follows the two-tier behaviour in flarum/framework#4871: one query per record fails the test, while a query repeated for the same few values raises a warning instead. --- docs/extend/testing.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/extend/testing.md b/docs/extend/testing.md index 59d039d3f..7bfe47a7f 100644 --- a/docs/extend/testing.md +++ b/docs/extend/testing.md @@ -423,20 +423,27 @@ This is an extreme edge case, but note that MySQL does not update the fulltext i #### N+1 Query Detection -Requests sent with `send()` are checked for N+1 query patterns, and **the test fails if one is found**. An N+1 is a single query shape executed once per record — a relationship loaded per model, a permission check per row, an API field that hits the database for each item. It looks harmless on a test fixture of three records and becomes hundreds of queries on a real forum. +Requests sent with `send()` are checked for repeated queries. An N+1 — a single query shape executed once per record — **fails the test**; a query that merely repeats itself for the same few values raises a warning instead. A failure looks like this: ``` -GET /api/posts ran repeated queries — likely an N+1. +GET /api/posts ran one query per record — an N+1. 10x (10 distinct bindings): select * from `warnings` where `warnings`.`post_id` = ? ``` -The two numbers say different things: +Ten executions, ten different post ids: the work grows with the data. Twenty posts on the page would mean twenty queries, and a busy forum thousands. Load the data for the whole page instead — [eager loading](api.md#eager-loading), a batched relationship, or one grouped query. -- **Distinct bindings ≈ the repeat count** — one query per record. This is the N+1: load the data for the whole page instead, with [eager loading](api.md#eager-loading), a batched relationship, or one grouped query. -- **One distinct binding, many repeats** — the same rows fetched over and over. Not an N+1, but usually worth memoising. +A warning looks like this: + +``` +POST /api/posts repeated queries for the same few values — consider memoising. + + 5x (2 distinct bindings): select * from `users` where `users`.`id` = ? limit 1 +``` + +Five executions but only two distinct users: wasteful, but bounded — it stays five queries whether the forum has two users or two million. Worth tidying when you're in the area; it won't fail your build. If a repetition is genuinely necessary, exempt that one query shape rather than switching the check off, so the rest of the request stays covered: From e43bb6f6b1b401ac5809973eef8f14f454b9d472 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 31 Jul 2026 21:30:36 +0100 Subject: [PATCH 3/3] Document where query findings surface Follows flarum/framework#4871: the example phpunit config now displays warning details, and findings appear as pull request annotations and a run summary in CI via FLARUM_REPEATED_QUERY_LOG. --- docs/extend/testing.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/extend/testing.md b/docs/extend/testing.md index 7bfe47a7f..71e9f6555 100644 --- a/docs/extend/testing.md +++ b/docs/extend/testing.md @@ -50,6 +50,7 @@ This is just an example [phpunit config file](https://docs.phpunit.de/en/12.5/co backupGlobals="false" backupStaticProperties="false" cacheDirectory=".phpunit.cache" + displayDetailsOnTestsThatTriggerWarnings="true" colors="true" processIsolation="true" stopOnFailure="false"> @@ -445,6 +446,10 @@ POST /api/posts repeated queries for the same few values — consider memoising. Five executions but only two distinct users: wasteful, but bounded — it stays five queries whether the forum has two users or two million. Worth tidying when you're in the area; it won't fail your build. +Warning details are printed when your `phpunit.integration.xml` sets `displayDetailsOnTestsThatTriggerWarnings="true"` (the example config above does). Without it PHPUnit still reports the count — `OK, but there were issues! … Warnings: 11` — but not what they were. + +In CI, findings also appear as annotations on the pull request and in a table in the run summary, so they don't have to be dug out of the log. Flarum's [reusable backend workflow](github-actions.md) does this by pointing `FLARUM_REPEATED_QUERY_LOG` at a file the job reads afterwards; set that variable yourself if you run tests through your own workflow. + If a repetition is genuinely necessary, exempt that one query shape rather than switching the check off, so the rest of the request stays covered: ```php