| name | review-tests |
| description | Reviews newly written tests — PHPUnit mocks instead of fakes, missing CoversClass, missing Group('core') on API-free integration tests, tests that assert implementation instead of behaviour, missing regression coverage. Use AFTER the tests are written. Report findings only, never edit. |
Review: Tests
Follow .agents/guidelines/reviewing.md for the shared protocol, then this checklist.
Also read .agents/guidelines/testing.md. The first check: for a bug fix, is there an
assertion that fails without the fix? Name it. If there is none, that is your first finding.
Run this after the tests are written.
- No PHPUnit mocks.
createMock(), getMockBuilder(), prophesize() are findings.
Use a Fake from the neighbouring Fake/ folder, or add one. When the assertion is about
what was sent, use a Spy — a Fake that records its calls — not a mock.
- Every new Fake is a duplicate until proven otherwise. For each added
Fake* file run
both checks before accepting it: find tests -name '<the exact file name>' — a Fake whose
name already exists was overwritten, not added, and the contract grep will not show it because
the old one may satisfy the contract differently (a TranslatorInterface fake is invisible to a
grep for extends AbstractTranslator); and a grep for the contract itself (implements <Interface>, extends <Class>) across tests/Unit — a second Fake for a contract that already
has one is a finding. An existing Fake that was replaced rather than extended is the worst
case: check git status for a Fake* file that shows as modified. Existing callers break at
runtime, not at review time. Two fakes for the same name but different contracts is fine — give
the new one a distinguishing name (FakeShopwareTranslator next to FakeTranslator).
- A Fake that extends a Shopware
Abstract* class must not return $this from
getDecorated() without checking the base. Shopware's decorator bases often implement their
non-abstract methods as $this->getDecorated()->sameMethod(...); a fake that inherits one of
them and points getDecorated() at itself recurses until the PHP process segfaults - the
suite dies mid-run with no failure message and no stack trace. Check with
grep -n 'getDecorated()->' <the abstract class>: if there are hits, override every one of
those methods in the fake and let getDecorated() throw instead.
#[CoversClass(...)] declared on every unit test class.- Builders for complex fixtures instead of long inline object setup — reuse
tests/Unit/Builder/ and the per-component Builder/ folders.
#[Group('core')] on every integration test that does not call the Mollie API;
API-dependent tests stay untagged — why, in .agents/guidelines/testing.md.- Asserted behaviour, not implementation. A test that only asserts a setter was called,
or that mirrors the production code line by line, is a finding.
- Shopware fixtures that blow up at runtime. Three that a green-looking test hides:
a
set*(null) call on a Shopware entity — almost every association setter is not
nullable, so "without a country" means never calling setCountry(), not passing null
(only OrderEntity::setLanguage() and setPrimaryOrderDelivery() accept it); an entity
without setId() that goes into an EntityCollection — it throws on
$_uniqueIdentifier; and two CalculatedTax entries with the same taxRate in one
CalculatedTaxCollection — that collection is keyed by rate, so the second silently
replaces the first and the multi-rate branch is never reached.
- The regression is covered. For a bug fix: is there a test that fails without the fix?
Name the assertion. If none, that is the most important finding.
- Edge cases from the correctness review — each accepted one should have a case.
- No new abstraction in the test layer for a single test.
- Test placement mirrors the production path:
shopware/Component/Refund/X.php →
tests/Unit/Refund/XTest.php.
