#4488 Make the three diagnostics tests assert over their own data instead of the whole schema

[keystone-guru-bot]

🤖 Closes #4488

Scoped to three of the five tests: the SpellTuningSnapshotLoaderTest pair is #4501's, which was already in progress when this started. Agreed with Wotuu up front rather than closing #4501 as a duplicate.

The premise changed, but the defect is real

The three tests in scope already pass on master with an isolated cache and a freshly provisioned schema — #4489 (#4481) and #4490 (#4485) landed after #4488 was filed. Verified with the issue's own command, plus a plain run and two consecutive runs, all green before touching anything.

So the first half of the definition of done was already met. The second half — "each test creates or seeds the state it depends on rather than inheriting it" — was not, and that is what this PR does. The failures were never cache effects: all three read schema-wide and assert on absolute counts or list positions, so a single foreign row decides the outcome.

Reproduced both reported symptoms from one stray row

Inserting one benefit-holding PatreonUserLink into the phpunit schema reproduces both symptoms the issue reported, exactly:

Reported	Reproduced	Assertion
Failed asserting that true is false	same	PatreonDiagnosticsServiceReconciliationTest:108
Failed asserting that 8 is identical to 50	Failed asserting that 97 is identical to 132	APIPatreonDiagnosticsControllerTest:306

Both numbers in the second are patreon_user_links.id values, not counts. With the fixes applied and the same stray row still present, all three classes pass; with the fixes stashed, the two assertions fail again. That is the before/after evidence for this PR.

What changed

PatreonDiagnosticsServiceReconciliationTest
assertFalse($reconciliation->needsAttention()) is computed over every benefit-holding link in the schema (PatreonDiagnosticsService.php:314-326), so the developer's own Patreon link — or one row leaked by an aborted earlier run — makes it true regardless of the test. Replaced with a scoped assertNoneOfThisTestsLinksAreReported(), which still catches the bug the original guarded (a matched link wrongly dumped into a reported bucket) without depending on the rest of the schema. This follows the principle the class's own findHolder() docblock already states; that one assertion was the exception to it.

APIPatreonDiagnosticsControllerTest

• unmatched_holders.0.* → located by patreon_user_link_id. The list covers every benefit-holding link in no defined order, so any other such link owns index 0.
• data.0/data.1 over sync runs → asserted over this class's own two runs. The endpoint returns every run in the schema, so any run recorded in the last two hours shifts every index.
• missed_by_latest_run → its run's started_at is now derived from the current maximum, so it wins "most recent" outright instead of relying on subMinutes(5) beating whatever else exists.
• tearDown() ran PatreonSyncRun::query()->delete(), wiping every sync-run row in the schema, not just its own. That both masked pollution for its own later tests and destroyed rows belonging to anything else. Now scoped to the ids it created — verified a foreign run survives a full class run.

AdminToolsCombatLogControllerTest

• The rendered failure count ((%d) — 1 ⚠ …) is a count over every failure row for that (dungeon, mapping version, npc), not the one row the test creates. Now read before the insert and asserted as before + 1.
• setUp() flushes the tmp_file cache store. MapContextMappingVersionData memoises the enemy/floor payload through RemembersToFile, a file cache with a 24h TTL that phpunit.xml redirects to /tmp/phpunit_cache but never clears — so the rendered map could come from a previous run's mapping data. This is the one cache these tests genuinely inherited, and the one CACHE_STORE=array does not cover.

Doing this systemically in TestCase::setUp() instead would be the stronger fix, but it flushes on every one of ~3,170 tests and would surface unrelated failures; worth its own issue if wanted.

Verification

• The three classes: pass clean, pass with a stray benefit-holding link present, pass with a stray recent sync run present.
• --group=Patreon — 79 passed. --group=Controller — 753 passed.
• composer run analyse — no errors. composer run fix — no changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_017gFvTB77bu1QKhL1U9Y48m

[keystone-guru-bot]

🤖 Cold review (codex) @ ff64e4e:

The isolation improvements remove an existing behavior check without equivalent replacement coverage. Tests could not be run because Docker access is blocked by the sandbox.

Review comment:

• [P2] Preserve coverage of the false needsAttention result — /home/wouterkoppenol/Git/private/keystone.guru-worktrees/4488-tests-depend-on-warm-cache/tests/Feature/App/Service/Patreon/PatreonDiagnosticsServiceReconciliationTest.php:110-110
  The replacement helper repeats the preceding findHolder() assertion: both only search the two holder lists. Neither checks needsAttention(), which uses the counts instead, so a regression that always returns true—or incorrectly increments unmatchedCount without listing a holder—now passes this test. Keep the database-independent scoped assertion, but preserve the removed behavior check with an isolated reconciliation DTO test or controlled repository fixtures. This also follows AGENTS.md:113, which prohibits weakening existing tests.

[keystone-guru-bot]

🤖 The needsAttention() finding was right and is fixed in c6d3b36. needsAttention() reads unmatchedCount/blockedCount, which are true totals, while the scoped helper only walked the two capped lists — so a regression that always returned true, or that miscounted without adding a listed holder, would indeed have passed.

Restored in two places, both database-independent as suggested:

tests/Unit/App/Service/Patreon/Dtos/Diagnostics/PatreonBenefitReconciliationTest.php (new) covers needsAttention() from constructed counts: the six count combinations including downgrades-only staying false, plus the capped-list case (unmatchedCount: 120 with unmatchedHolders: []) that pins it to the counts rather than the lists. This is what catches "always returns true".

The reconciliation test now measures the counts as deltas against a baseline taken after the link exists but before it holds any benefit row — a link with no benefits is not a holder, so the baseline is the rest of the schema with everything else about the test already in place. It asserts unmatchedCount and blockedCount are unchanged and holderCount went up by exactly one. That is what catches "miscounts unmatchedCount without adding a listed holder", without depending on the schema being clean.

Re-verified with a stray benefit-holding link present in the phpunit schema: 21 passed. --group=Patreon: 86 passed. composer run analyse: no errors.

No changes needed on points 2–4.