Skip to content

Commit 7539d84

Browse files
committed
feat: surface query findings on the pull request, not just in logs
The warning tier was invisible in three separate ways. The trigger_error call was prefixed with @, so PHPUnit never saw it at all — nothing appeared even with --display-warnings. Only one of eighteen phpunit configs in this repo set displayDetailsOnTestsThatTriggerWarnings, so even a working warning printed no detail. And a developer who relies on CI rather than local runs would have to read the middle of a job log to find either. So: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when it is set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations — errors for N+1s, warnings for non-scaling repetition — plus a table in the run summary. Annotations attach to the pull request, which is where the audience that most needs them is looking.
1 parent 5247b83 commit 7539d84

21 files changed

Lines changed: 82 additions & 1 deletion

File tree

.github/workflows/REUSABLE_backend.yml

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,12 +287,43 @@ jobs:
287287
fi
288288
working-directory: ${{ inputs.backend_directory }}
289289
env:
290+
FLARUM_REPEATED_QUERY_LOG: ${{ runner.temp }}/repeated-queries.jsonl
290291
DB_HOST: 127.0.0.1
291292
DB_PORT: ${{ (matrix.driver == 'mysql' && job.services.mysql.ports['3306']) || (matrix.driver == 'mariadb' && job.services.mariadb.ports['3306']) || (matrix.driver == 'pgsql' && job.services.postgres.ports['5432']) }}
292293
DB_PREFIX: ${{ matrix.prefix }}
293294
DB_DRIVER: ${{ matrix.driver }}
294295
COMPOSER_PROCESS_TIMEOUT: 600
295296

297+
# Surface the query findings on the pull request itself. A developer who
298+
# only ever looks at the checks page would otherwise never see the
299+
# warnings PHPUnit prints mid-log. Runs even when the tests fail, since a
300+
# failing N+1 is exactly what we want to point at.
301+
- name: Report repeated queries
302+
if: ${{ !cancelled() }}
303+
run: |
304+
log="${{ runner.temp }}/repeated-queries.jsonl"
305+
306+
[ -s "$log" ] || exit 0
307+
308+
# One annotation per distinct finding; identical findings recur across
309+
# tests, so collapse them first.
310+
sort -u "$log" | while read -r finding; do
311+
level=$(echo "$finding" | jq -r 'if .scaling then "error" else "warning" end')
312+
title=$(echo "$finding" | jq -r 'if .scaling then "N+1 query" else "Repeated query" end')
313+
echo "$finding" | jq -r --arg level "$level" --arg title "$title" \
314+
'"::\($level) title=\($title)::\(.where) — \(.count) executions, \(.distinctBindings) distinct: \(.sql)"'
315+
done
316+
317+
{
318+
echo "### Query findings"
319+
echo
320+
echo "| | Where | Executions | Distinct | Query |"
321+
echo "|---|---|---|---|---|"
322+
sort -u "$log" | jq -r '"| \(if .scaling then "🛑 N+1" else "⚠️ repeated" end) | `\(.where)` | \(.count) | \(.distinctBindings) | `\(.sql | .[0:120])` |"'
323+
echo
324+
echo "🛑 grows with your data — fix it. ⚠️ repeats a few values — consider memoising."
325+
} >> "$GITHUB_STEP_SUMMARY"
326+
296327
phpstan:
297328
runs-on: ${{ inputs.runner_type }}
298329

extensions/akismet/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/approval/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/audit/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/flags/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/gdpr/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/likes/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/lock/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/mentions/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

extensions/messages/tests/phpunit.integration.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
backupGlobals="false"
66
cacheDirectory=".phpunit.cache"
77
backupStaticProperties="false"
8+
displayDetailsOnTestsThatTriggerWarnings="true"
89
colors="true"
910
processIsolation="true"
1011
stopOnFailure="false"

0 commit comments

Comments
 (0)