diff --git a/README.md b/README.md index be528ec4..d2919104 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ If you previously installed `gh-tooling` from this marketplace, Claude Code drop ### test-writing -Generates and validates PHPUnit unit tests for Shopware 6. Analyzes source classes, detects the test category (DTO, Service, Flow/Event, DAL, Exception), generates tests, reviews them against 44 Shopware-specific rules, and iterates fixes until they pass. Also supports team-based consensus review running as a multi-agent Claude Code Workflow (experimental) — one read-only reviewer for unit, integration, and migration tests, with cross-cutting coverage and integration-to-unit placement flags. +Generates and validates PHPUnit unit tests for Shopware 6. Analyzes source classes, detects the test category (DTO, Service, Flow/Event, DAL, Exception), generates tests, reviews them against the 43 rules of the Shopware unit-test catalog, and iterates fixes until they pass. Also supports team-based consensus review running as a multi-agent Claude Code Workflow (experimental) — one read-only reviewer for unit, integration, and migration tests, with cross-cutting coverage and integration-to-unit placement flags. ```bash /plugin install test-writing@shopware-ai-coding-tools diff --git a/plugin-tests/test-writing/build_rule_package.bats b/plugin-tests/test-writing/build_rule_package.bats index 7040303e..7bd2c664 100644 --- a/plugin-tests/test-writing/build_rule_package.bats +++ b/plugin-tests/test-writing/build_rule_package.bats @@ -265,7 +265,7 @@ _scoped_render() { # Inspect only rule-header lines (# ID — Title); bodies never start that way. run grep -E '^# [A-Z]+-[0-9]+ ' "${pkg}" assert_success - assert_line --partial "CONV-007" # class-structure + assert_line --partial "CONV-005" # class-structure assert_line --partial "DESIGN-004" # class-bodies refute_line --partial "CONV-001" # method — outside the requested set } @@ -302,8 +302,8 @@ _scoped_render() { run grep -E '^# [A-Z]+-[0-9]+ ' "${pkg}" assert_success - refute_line --partial "CONV-007" # scoped-review=exclude refute_line --partial "CONV-005" # scoped-review=exclude + refute_line --partial "CONV-017" # scoped-review=exclude assert_line --partial "CONV-001" # scoped-review=include — kept } @@ -438,10 +438,10 @@ _catalog_ids() { pkg="$(_pkg_path)" # Every convention rule declares test-types: all, so the composed migration - # catalog carries all sixteen — named, so a rule that silently stops + # catalog carries all fifteen — named, so a rule that silently stops # reaching migration tests fails here rather than passing under a count. assert_equal "$(_catalog_ids "${pkg}" CONV)" "$(printf '%s\n' \ - CONV-001 CONV-002 CONV-003 CONV-004 CONV-005 CONV-006 CONV-007 CONV-008 \ + CONV-001 CONV-002 CONV-003 CONV-004 CONV-005 CONV-006 CONV-008 \ CONV-009 CONV-010 CONV-011 CONV-012 CONV-013 CONV-014 CONV-016 \ CONV-017)" diff --git a/plugin-tests/test-writing/review_unit.bats b/plugin-tests/test-writing/review_unit.bats index 5ac3ab9c..1882e69d 100644 --- a/plugin-tests/test-writing/review_unit.bats +++ b/plugin-tests/test-writing/review_unit.bats @@ -27,7 +27,7 @@ _assert_indexed_review_unit() { } @test "_build_rule_index parses a class-structure rule" { - _assert_indexed_review_unit CONV-007 class-structure + _assert_indexed_review_unit CONV-005 class-structure } @test "_build_rule_index parses a class-bodies rule" { @@ -84,7 +84,7 @@ _assert_indexed_review_unit() { run _filter_rules "" "" "" "" "" "" "class-bodies" assert_success refute_line "CONV-001" # method - refute_line "CONV-007" # class-structure (proves exact match, not prefix) + refute_line "CONV-005" # class-structure (proves exact match, not prefix) } @test "review_unit filter composes with group filter" { @@ -92,7 +92,6 @@ _assert_indexed_review_unit() { run _filter_rules "convention" "" "" "" "" "" "class-structure" assert_success assert_line "CONV-005" - assert_line "CONV-007" refute_line "MIGRATION-008" # class-structure but not in the convention group } @@ -102,7 +101,7 @@ _assert_indexed_review_unit() { @test "get_rules exposes review-unit in the per-rule metadata header" { _build_rule_index "${RULES_DIR}" - run tool_get_rules '{"ids":"CONV-007"}' + run tool_get_rules '{"ids":"CONV-005"}' assert_success assert_output --partial "Review unit: class-structure" } @@ -218,7 +217,7 @@ _assert_indexed_scoped_review() { } @test "_build_rule_index parses a scoped-review=exclude rule" { - _assert_indexed_scoped_review CONV-007 exclude + _assert_indexed_scoped_review CONV-005 exclude } @test "_build_rule_index parses a scoped-review=include rule" { @@ -253,7 +252,7 @@ _assert_indexed_scoped_review() { scoped="$(_filter_rules "" "" "" "" "" "true" "" | sort)" full="$(_filter_rules "" "" "" "" "" "" "" | sort)" excluded="$(comm -23 <(printf '%s\n' "${full}") <(printf '%s\n' "${scoped}"))" - assert_equal "${excluded}" "$(printf 'CONV-005\nCONV-007\nCONV-017\nDESIGN-006\nINTEGRATION-008')" + assert_equal "${excluded}" "$(printf 'CONV-005\nCONV-017\nDESIGN-006\nINTEGRATION-008')" } @test "scoped_review=true keeps scoped-review=include rules in the result" { @@ -270,7 +269,7 @@ _assert_indexed_scoped_review() { run _filter_rules "" "" "" "" "" "" "" assert_success assert_line "CONV-005" - assert_line "CONV-007" + assert_line "CONV-017" } # ============================================================================ @@ -279,7 +278,7 @@ _assert_indexed_scoped_review() { @test "get_rules exposes scoped-review=exclude in the per-rule metadata header" { _build_rule_index "${RULES_DIR}" - run tool_get_rules '{"ids":"CONV-007"}' + run tool_get_rules '{"ids":"CONV-005"}' assert_success assert_output --partial "Scoped review: exclude" } diff --git a/plugins/test-writing/.claude-plugin/plugin.json b/plugins/test-writing/.claude-plugin/plugin.json index 15def37e..8f2c3ca9 100644 --- a/plugins/test-writing/.claude-plugin/plugin.json +++ b/plugins/test-writing/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "test-writing", - "version": "5.1.1", + "version": "5.2.0", "description": "Generate and validate PHPUnit tests for Shopware 6. Unit tests: analyzes source classes to detect category (DTO, Service, Flow/Event, DAL, Exception) and applies category-specific templates. Migration tests: analyzes SQL operations to generate pattern-appropriate migration tests with 8 migration-specific rules. Integration tests: generates wired-up tests via phpunit-integration-test-generation for controller, message-handler, indexer, DAL-flow, and multi-service patterns (defers to unit generation when the SUT is unit-shape); 8 quality rules plus a placement smoke check via phpunit-integration-test-reviewing; a separate user-invoked phpunit-integration-to-unit-migrating skill audits placement with 8 deep-reasoning rules and migrates load-bearing-free tests to the unit suite. Workflow-based team review: wave-orchestrated agents coordinated through a shared blackboard (no agent-to-agent messaging), with adversarial red team and defense rounds. Bundles test-rules MCP server for Shopware compliance rules. Orchestrator runs inline fix loop (max 4 iterations) with oscillation detection. Optional dev-tooling plugin enables PHPStan/PHPUnit/ECS validation in the fix loop.", "author": { "name": "Shopware Labs" diff --git a/plugins/test-writing/AGENTS.md b/plugins/test-writing/AGENTS.md index 0654afa7..7e8324c6 100644 --- a/plugins/test-writing/AGENTS.md +++ b/plugins/test-writing/AGENTS.md @@ -40,12 +40,12 @@ plugins/test-writing/ │ ├── test-reviewer.md │ └── test-adversary.md ├── rules/ -│ ├── convention/CONV-{001..014,016,017}.md +│ ├── convention/CONV-{001..006,008..014,016,017}.md │ ├── design/DESIGN-{001..010}.md │ ├── isolation/ISOLATION-{001..006}.md │ ├── provider/PROVIDER-{001..005}.md │ ├── unit/UNIT-{001,003,004,007..010}.md -│ ├── migration/MIGRATION-{001..009}.md +│ ├── migration/MIGRATION-{001..006,008,009}.md │ ├── integration/INTEGRATION-{001..008}.md │ └── placement/PLACEMENT-{001..008}.md ├── mcp-server-test-rules/ diff --git a/plugins/test-writing/CHANGELOG.md b/plugins/test-writing/CHANGELOG.md index 19168895..7381c650 100644 --- a/plugins/test-writing/CHANGELOG.md +++ b/plugins/test-writing/CHANGELOG.md @@ -5,6 +5,27 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [5.2.0] - 2026-09-05 + +### Removed +- **CONV-007 (test class member ordering) and MIGRATION-007 (assertSame over assertEquals in migration tests).** Both duplicate deterministic gates verified active in the target repository: php-cs-fixer's `ordered_class_elements` runs over `tests/` and its default group order covers all six of CONV-007's member slots (the `phpunit` group places setUp/tearDown ahead of test methods, and a `public static` provider sorts before private helpers), and phpstan-phpunit's `AssertEqualsIsDiscouragedRule` is active on every analyzed path including `tests/migration`. Same pattern as the CONV-015/UNIT-002 removals in 5.0.0. The unit-review catalog drops from 44 rules to 43. + +### Fixed +- **UNIT-004 no longer prescribes remediations the PHPStan gate rejects.** A shared `createMock()` double whose sibling methods set expectations is exempt from the drop-`expects()` remedy (`NoCreateMockWithoutExpectationsRule` requires every method touching such a double to carry an expectation), UNIT-004 and UNIT-003 defer to that rule where it already reports a double, a cross-method `never()`/`once()` reachability pairing counts as the assertion that pins which branch ran, and the remediation notes quote the Shopware guideline's two stub-conversion gotchas (`->with()` exists on mocks only; a fixture-helper parameter takes `Foo&Stub`, not `MockObject`) that pass PHPUnit and fail only in PHPStan. +- **CONV-014 no longer relocates position-load-bearing assertions.** An assertion between two actions is intentional when moving it past the next action changes the observed value — reported at most as `consider`, with no relocation suggested — and the rule's own Fix example no longer moves an `assertCount` past a call that could mutate the counted collection. +- **DESIGN-003 detection compares assertions, not bodies.** Merging is safe only when neither test asserts the distinguishing detail, and a fold into an existing provider names that provider and respects its documented scope. +- **DESIGN-004 keeps boundary inputs whose distinctness the implementation deliberately erases** — the equivalence between them is itself the contract under test. +- **CONV-011 suppresses annotations that mechanically expand the method name**, decided by a deterministic camelCase-split comparison run before the finding is emitted. +- **The migration reviewing skill's `warnings` example matches CONV-014's semantics**: reorder-only, with empty `deleted_methods`/`removed_assertions`. + +### Changed +- **CONV-005 is a `consider` rule with a single-category exemption.** A class deliberately holding one half of a split (for example only the error cases, with the happy paths in a sibling class) is no longer told to grow the categories it does not contain. +- **UNIT-001 findings name the enforcing mechanism** — return type, PHPStan level, constructor contract, or a named rule such as `NoCreateMockWithoutExpectationsRule` — so a partially wrong triviality premise is checkable before any assertion is removed. +- **CONV-017 reconciles with the guideline's `setUp()` stance.** State reused across test methods stays in `setUp()`; the identity/accumulated-setup exemption applies only when the property is read more than once inside its single referencing method and those reads must observe the same instance. + +### Added +- **Remediation invariant** in the three reviewing skills' output requirements and the four team-review prompt builders (reviewer, reconcile, red team, defense): a suggested fix never changes what an existing assertion pins and never introduces an assertion as a means of satisfying a structural, layout, naming, or style constraint; coverage-gap rules (MIGRATION-008, DESIGN-006, INTEGRATION-007) carry new assertions as the finding's explicit deliverable; deletions ride `removed_assertions`; re-expressing the same pinned fact (CONV-012's `assertSame` conversion) stays legal. + ## [5.1.1] - 2026-09-05 ### Fixed diff --git a/plugins/test-writing/README.md b/plugins/test-writing/README.md index 77ba3789..a7344af6 100644 --- a/plugins/test-writing/README.md +++ b/plugins/test-writing/README.md @@ -17,7 +17,7 @@ Generate and validate PHPUnit unit tests for Shopware 6. Automatically analyzes - **MCP Rule Server**: Dynamic rule discovery with `mcp__plugin_test-writing_test-rules__get_rules` for context-efficient reviews - **Team-Based Consensus Review**: The single Workflow-based reviewer for unit, integration, and migration tests over one mixed manifest — `test_type` (resolved by path) routes each file to its rule catalog, per-type reviewing sub-skill, decomposition track, and adversary lenses. 3 independent reviewers per unit and K independent per-file adversaries (one per lens — tautology / weak-assertion / missed-coverage — each reading a single file). Oversized test classes are decomposed by rule track — method-shards plus a whole-class or body-free structural-digest track — so large files no longer overflow the context window, and large changesets are partitioned into review shards that run as a **campaign of sequential workflow launches** with every stage result persisted to disk, so an interrupted campaign resumes from where it stopped. Stages: independent review + peer reconciliation per shard (consensus), a whole-changeset signals run (cross-file consistency, adoption), and a cost-gated adversarial run (red team, defense, hard-capped arbitration). Deterministic cross-cutting SUT-coverage map and informational integration-to-unit placement flags computed at merge. Findings carry a method-primary locator, a per-finding branch-scope flag (`branch_touched`) on diff runs, a source-change escalation when a fix cannot be made in the test alone, and deletion accounting (`deleted_methods`, `removed_assertions`) naming what a remediation removes. A finding is identified as `rule_id|method`, so reviewers describing one defect differently pool their votes instead of fragmenting into contested singletons. 2-of-3 majority consensus per track, and a review unit that comes back with fewer than two live reviewer stances fails its shard rather than reporting a clean pass. Strictly read-only (see [Team Review](#team-review) below) - **Migration Test Generation**: Analyzes migration source classes (SQL operations, updateDestructive logic) to generate pattern-appropriate migration tests -- **Migration Test Reviewing**: 9 migration-specific rules covering idempotency, cleanup, assertion patterns, and Shopware conventions +- **Migration Test Reviewing**: 8 migration-specific rules covering idempotency, cleanup, assertion patterns, and Shopware conventions - **Integration Test Generation**: Analyzes source classes to detect supported integration patterns (controller/route, message-handler, indexer, DAL-flow, multi-service) and generates `IntegrationTestBehaviour`-based tests. Defers to unit test generation when the SUT is unit-shape - **Integration Test Reviewing**: 8 integration-specific rules covering integration-base usage, real-collaborator policy, transactional cleanup, determinism, independence, and a placement smoke check - **Integration-to-Unit Migration**: User-invoked audit workflow that walks 8 placement-reasoning rules per test, buckets into migrate/split/keep/delete, and applies one of 6 codified refactoring patterns. Separate skill, never auto-invoked @@ -137,7 +137,6 @@ The generator analyzes the migration's SQL operations and selects appropriate te | MIGRATION-004 | Test-created tables/data not cleaned up | | MIGRATION-005 | Multiple SQL in single try/catch, or catching Exception instead of Throwable | | MIGRATION-006 | String interpolation for table/column names in SQL | -| MIGRATION-007 | assertEquals used instead of assertSame | | MIGRATION-008 | Missing testGetCreationTimestamp method | | MIGRATION-009 | setUp/tearDown mutates DB state | @@ -270,7 +269,6 @@ Rules are organized by group and enforce level. | DESIGN-004 | Test redundancy (unjustified cases or methods covering same path) | | CONV-006 | TestDox phrasing doesn't follow guidelines | | UNIT-003 | Over-mocking (should use StaticEntityRepository or real impl) | -| CONV-007 | Test class structure order incorrect | | CONV-008 | Exception expectation set after throwing call | | ISOLATION-001 | Shared mutable state between tests (FIRST: Independent) | | ISOLATION-002 | Non-deterministic inputs without mocking (FIRST: Repeatable) | @@ -295,7 +293,6 @@ Rules are organized by group and enforce level. | DESIGN-006 | Unbalanced coverage distribution (< 20% edge+error cases) | | CONV-014 | Unclear AAA structure (assertions interspersed with setup) | | ISOLATION-004 | Opaque test data identifiers (UUID hex strings instead of descriptive strings like `'product-id'`) | -| CONV-005 | Test method ordering doesn't follow pattern | | PROVIDER-003 | Data provider uses `return []` instead of `yield`/`iterable` | | CONV-017 | Single-use test property (assigned in `setUp()`, used in only one test method — inline it) | | CONV-016 | `Test` prefix on non-test helper class (reserve `Test` for classes extending `TestCase`; use `Stub*`, `Fake*`) | @@ -313,6 +310,7 @@ Rules are organized by group and enforce level. | DESIGN-008 | Potential preservation value in redundant test (regression/bug documentation) | | ISOLATION-006 | Consider real fixture files for file I/O testing | | DESIGN-009 | Duplicated inline Arrange code (identical construction in multiple test methods; extract to setUp() or private helper) | +| CONV-005 | Test method ordering doesn't follow pattern | ### Migration Rules (Must-Fix) @@ -324,7 +322,6 @@ Rules are organized by group and enforce level. | MIGRATION-004 | Test-created tables/data not cleaned up | | MIGRATION-005 | Multiple SQL in single try/catch, or catching Exception instead of Throwable | | MIGRATION-006 | String interpolation for table/column names in SQL | -| MIGRATION-007 | assertEquals used instead of assertSame | | MIGRATION-008 | Missing testGetCreationTimestamp method | | MIGRATION-009 | setUp/tearDown mutates DB state | @@ -517,12 +514,12 @@ Reference files provide detailed guidance: ### Rule Files Individual rule files are in `rules/` organized by group: -- `rules/convention/` — PHPUnit and Shopware coding conventions (CONV-001 through CONV-017, less CONV-015) +- `rules/convention/` — PHPUnit and Shopware coding conventions (CONV-001..006, CONV-008..014, CONV-016, CONV-017) - `rules/design/` — Test design principles (DESIGN-001 through DESIGN-010) - `rules/isolation/` — Test independence and isolation (ISOLATION-001 through ISOLATION-006) - `rules/provider/` — Data provider patterns (PROVIDER-001 through PROVIDER-005) - `rules/unit/` — Unit test-specific rules (UNIT-001 through UNIT-010, less UNIT-002) -- `rules/migration/` — Migration test rules (MIGRATION-001 through MIGRATION-009) +- `rules/migration/` — Migration test rules (MIGRATION-001..006, MIGRATION-008, MIGRATION-009) - `rules/integration/` — Integration test rules (INTEGRATION-001 through INTEGRATION-008) - `rules/placement/` — Placement reasoning prompts (PLACEMENT-001 through PLACEMENT-008) diff --git a/plugins/test-writing/rules/convention/CONV-005.md b/plugins/test-writing/rules/convention/CONV-005.md index 4dc7eda5..5222e9f1 100644 --- a/plugins/test-writing/rules/convention/CONV-005.md +++ b/plugins/test-writing/rules/convention/CONV-005.md @@ -2,7 +2,7 @@ id: CONV-005 title: Test Method Ordering group: convention -enforce: should-fix +enforce: consider test-types: all test-categories: A,B,C,D,E scope: general @@ -12,7 +12,7 @@ scoped-review: exclude ## Test Method Ordering -**Scope**: A,B,C,D,E | **Enforce**: Should fix +**Scope**: A,B,C,D,E | **Enforce**: Consider Test methods MUST follow a logical progression pattern. @@ -76,6 +76,10 @@ A name can match more than one row. Apply the rows in this precedence and take t `testThrowsWhenNameEmpty` matches both the Error case and the Edge case row; it is an Error case. +### Single-Category Exemption + +A test class whose methods all fall into one category is exempt from this rule — it never reports a missing happy-path (or any other missing) category for such a class. A class that deliberately holds one half of a split (for example, all its error-case methods, with the happy-path methods living in a sibling class) has nothing to reorder relative to a category it does not contain. + ### Exceptions A method whose docblock or inline comment records a required adjacency to another method is not reordered. diff --git a/plugins/test-writing/rules/convention/CONV-007.md b/plugins/test-writing/rules/convention/CONV-007.md deleted file mode 100644 index 4022dcca..00000000 --- a/plugins/test-writing/rules/convention/CONV-007.md +++ /dev/null @@ -1,78 +0,0 @@ ---- -id: CONV-007 -title: Class Structure Order -group: convention -enforce: must-fix -test-types: all -test-categories: A,B,C,D,E -scope: general -review-unit: class-structure -scoped-review: exclude ---- - -## Class Structure Order - -**Scope**: A,B,C,D,E | **Enforce**: Must fix - -Test class members MUST follow consistent ordering. Inconsistent structure makes navigation difficult and hinders code reviews. - -### Required Order - -```php -class ProductServiceTest extends TestCase -{ - use SomeTrait; // 1. Traits - - public const TEST_PRODUCT_ID = '123'; // 2. Constants - - private ProductService $service; // 3. Properties - private StaticEntityRepository $repo; - - protected function setUp(): void // 4. setUp/tearDown - { - $this->repo = new StaticEntityRepository([]); - $this->service = new ProductService($this->repo); - } - - protected function tearDown(): void - { - // cleanup if needed - } - - public function testCreates(): void {} // 5. Test methods - public function testDeletes(): void {} - - private function createProduct(): Product // 6. Helper methods - { - return new Product('test'); - } -} -``` - -### Detection - -```php -// INCORRECT - wrong order -class ProductServiceTest extends TestCase -{ - private function createProduct(): Product {} // Helper before tests — WRONG - - public function testCreates(): void {} - - private ProductService $service; // Property after tests — WRONG - - protected function setUp(): void {} // setUp after tests — WRONG - - use SomeTrait; // Trait after everything — WRONG -} -``` - -### Fix - -Reorder class members: -1. Traits (`use Trait;`) -2. Constants (`public const`) -3. Properties (`private $repo;`) -4. setUp/tearDown methods -5. Test methods (following CONV-005 ordering) -6. Helper methods (private functions) diff --git a/plugins/test-writing/rules/convention/CONV-011.md b/plugins/test-writing/rules/convention/CONV-011.md index 6a3d9917..1852cd50 100644 --- a/plugins/test-writing/rules/convention/CONV-011.md +++ b/plugins/test-writing/rules/convention/CONV-011.md @@ -38,3 +38,14 @@ public function testCalculatesPrice(float $gross, float $net, float $tax): void #[TestDox('calculates price: gross=$gross, net=$net, tax=$tax')] public function testCalculatesPrice(float $gross, float $net, float $tax): void ``` + +### Suppression: Mechanical Expansion of the Method Name + +Suppress the finding when the proposed `TestDox` text is a mechanical expansion of the method name — camelCase-split the method name (minus its `test` prefix), normalize it (lowercase, join with spaces), and compare it against the proposed TestDox text normalized the same way. An exact match after normalization means the annotation would say nothing the method name did not already say, so no finding is emitted. This is a deterministic string comparison, not a judgment call: run it before emitting the finding, not after. + +```php +// SUPPRESSED - TestDox is the mechanical expansion of the method name, adds nothing +#[DataProvider('priceProvider')] +#[TestDox('calculates price')] +public function testCalculatesPrice(float $gross, float $net, float $tax): void +``` diff --git a/plugins/test-writing/rules/convention/CONV-014.md b/plugins/test-writing/rules/convention/CONV-014.md index 0acbd68f..a6273460 100644 --- a/plugins/test-writing/rules/convention/CONV-014.md +++ b/plugins/test-writing/rules/convention/CONV-014.md @@ -42,6 +42,8 @@ public function testProcessesOrder(): void ### Fix +`process()` here only computes a result and does not touch `$order`'s items, so moving the count assertion past it observes the same value — this relocation is safe. (When the action under test DOES mutate or consume the state an assertion reads, see the carve-out below instead of relocating.) + ```php // AAA structure - assertions at end public function testProcessesOrder(): void @@ -51,10 +53,30 @@ public function testProcessesOrder(): void $order->addItem($this->product); // Act - $result = $this->service->process($order); + $result = $this->service->process($order); // reads $order, does not mutate its items // Assert static::assertCount(1, $order->getItems()); static::assertTrue($result->isSuccess()); } ``` + +### Position-Load-Bearing Carve-Out + +An assertion sitting between two actions can be intentional rather than a structure defect: moving it past the next action changes the value it observes, because that next action mutates or destroys the state the assertion reads. Report such a placement at most as `consider`, never propose relocating it, and never remediate by adding assertions the test did not previously make just to compensate for the move — a remedy that must invent a new assertion to cover for a relocation is wrong by construction. + +```php +// CORRECT (position load-bearing) - process() consumes the order's items, +// so asserting the pre-call count after process() would observe a different value +public function testProcessesOrderAndDrainsQueue(): void +{ + $order = new Order(); + $order->addItem($this->product); + + static::assertCount(1, $order->getItems()); // observes state process() is about to consume + + $result = $this->service->process($order); // mutates $order — moving the assertion past this changes what it reads + + static::assertTrue($result->isSuccess()); +} +``` diff --git a/plugins/test-writing/rules/convention/CONV-017.md b/plugins/test-writing/rules/convention/CONV-017.md index 0adb961f..67ec114b 100644 --- a/plugins/test-writing/rules/convention/CONV-017.md +++ b/plugins/test-writing/rules/convention/CONV-017.md @@ -51,6 +51,8 @@ Do NOT flag when: - Property is a mock/stub that also appears in assertions or `expects()` calls - Property is the system-under-test (`$this->service`, `$this->route`) +CONV-017 targets single-use state. It is reused-across-tests, not identity/accumulated-setup, that exempts a property from this rule on its own — a property referenced by 2+ test methods is already covered by "Property is used in 2+ test methods" above. The narrower identity/accumulated-setup case per Shopware's test-writing guideline applies only within the one test method that references it: the property (or a getter backed by it) is read or called more than once *inside that single method*, and the calls must observe the same instance or accumulated state rather than each getting a fresh one — for example a registry populated across several calls, or a command tester whose captured output is read after multiple invocations. A property called exactly once in its one referencing method has no same-instance requirement to protect and is not exempted by this clause; inline it per the Fix below. + ### Fix Relocate the construction into the one test method that references it and drop the property declaration. `setUp()` keeps every collaborator the remaining properties still need. diff --git a/plugins/test-writing/rules/design/DESIGN-003.md b/plugins/test-writing/rules/design/DESIGN-003.md index 3bed09c9..dcbd3b76 100644 --- a/plugins/test-writing/rules/design/DESIGN-003.md +++ b/plugins/test-writing/rules/design/DESIGN-003.md @@ -83,3 +83,11 @@ public function testAcceptsValidEmail(string $email): void | `#[DataProvider]` | Large datasets, complex objects | | `#[DataProvider]` | Shared data across tests | | `#[DataProvider]` | Dynamic data generation | + +### Similar Bodies Are Not Enough — Check the Assertions Too + +Similarity for this rule is not established by matching arrange/act code alone. Two tests can read almost identically and still verify different things: consolidating them into one provider row is safe only when their assertions differ solely in the discriminating value, never in what they check. Before proposing a merge, read every candidate test's assertions and confirm none of them asserts a detail the others omit — a body match paired with a distinguishing assertion is not "3+ similar tests" under this rule, and stays as separate methods regardless of how alike the setup looks. + +A finding that folds tests into an **existing** data provider names that provider and confirms the fold does not contradict its documented scope: a provider whose docblock or key naming claims a specific input tier (for example, "valid emails only") does not absorb a case outside that tier just because the test bodies look alike. + +Shopware's guideline states the fold condition and its limit: "Fold two tests into one provider when they differ only in their input and their expectation; pass the discriminating value together with the expectation instead of copying the whole scenario. This does not override the rule above about keeping one focused test per distinct behavior: a case that carries its own meaning stays its own test." diff --git a/plugins/test-writing/rules/design/DESIGN-004.md b/plugins/test-writing/rules/design/DESIGN-004.md index 378e1fef..6bfb53a7 100644 --- a/plugins/test-writing/rules/design/DESIGN-004.md +++ b/plugins/test-writing/rules/design/DESIGN-004.md @@ -207,6 +207,10 @@ If none apply, the case is redundant (DESIGN-004). 6. **Name the survivor before deleting**: before any provider row or assertion is deleted, the finding names the surviving row or assertion that exercises the same production branch AND reads the same object. Where none exists, the finding states so and the deletion does not proceed. +### Boundary-Input Carve-Out + +Two provider cases that a language construct collapses onto one code path (for example, two inputs both satisfying the same `||` operand, or both falling into the same `switch` arm) can still be justified: when each input is meaningful at a system boundary and the equivalence between them is itself the contract under test, the two are not redundant even though the implementation happens to erase their distinctness on this path. Shopware's guideline states the same conservatism as a floor: "Be conservative when deleting \"duplicate\" provider cases. Remove only exact semantic duplicates that add no coverage, and keep similar-looking cases when they cover distinct edge behavior." Do not flag such a pairing as DESIGN-004 redundancy; name the boundary each case pins when justifying the keep. + ### Relationship to DESIGN-010 Where both rules fire on the same provider rows, they are complementary and compose in one order: DESIGN-004 decides redundancy first, then DESIGN-010 checks guard isolation per surviving row. diff --git a/plugins/test-writing/rules/migration/MIGRATION-007.md b/plugins/test-writing/rules/migration/MIGRATION-007.md deleted file mode 100644 index 42c4077c..00000000 --- a/plugins/test-writing/rules/migration/MIGRATION-007.md +++ /dev/null @@ -1,35 +0,0 @@ ---- -id: MIGRATION-007 -title: assertSame over assertEquals -group: migration -enforce: must-fix -test-types: migration -test-categories: all -scope: shopware -review-unit: method -scoped-review: include ---- - -## assertSame over assertEquals - -**Scope**: all | **Enforce**: Must fix - -Use `assertSame()` (strict identity, type-safe) instead of `assertEquals()` (loose comparison, type-coercing). - -### Detection - -Detect any usage of `assertEquals(` or `static::assertEquals(` in the test file. - -```php -// INCORRECT - loose comparison -static::assertEquals(1718615305, $migration->getCreationTimestamp()); -static::assertEquals('test', $salutationLetter); -``` - -### Fix - -```php -// CORRECT - strict comparison -static::assertSame(1718615305, $migration->getCreationTimestamp()); -static::assertSame('test', $salutationLetter); -``` diff --git a/plugins/test-writing/rules/migration/MIGRATION-008.md b/plugins/test-writing/rules/migration/MIGRATION-008.md index 3b7a60b0..7925080f 100644 --- a/plugins/test-writing/rules/migration/MIGRATION-008.md +++ b/plugins/test-writing/rules/migration/MIGRATION-008.md @@ -44,7 +44,7 @@ public function testGetCreationTimestamp(): void ### Incorrect Patterns ```php -// INCORRECT - uses assertEquals (also triggers MIGRATION-007) +// INCORRECT - uses assertEquals instead of assertSame public function testGetCreationTimestamp(): void { static::assertEquals(1718615305, (new Migration1718615305AddEuToCountryTable())->getCreationTimestamp()); diff --git a/plugins/test-writing/rules/unit/UNIT-001.md b/plugins/test-writing/rules/unit/UNIT-001.md index dd45c261..e11b9001 100644 --- a/plugins/test-writing/rules/unit/UNIT-001.md +++ b/plugins/test-writing/rules/unit/UNIT-001.md @@ -165,6 +165,20 @@ public function testFullNameCombinesFirstAndLastName(): void } ``` +### Name the Enforcing Mechanism + +A finding that calls an assertion trivial states which mechanism enforces the property being asserted, so the claim is checkable before any assertion is removed. A compound property can be half-enforced — flagging the whole assertion trivial when only one half has a mechanism behind it removes real coverage. Name the specific mechanism, from this vocabulary where it applies: + +- **Return types** — a method with a single non-nullable return type already guarantees what an `assertInstanceOf`/`assertIsArray`/etc. against that exact type would re-check (see "When Type Assertions ARE Valid" above for the narrowing exception). This is the exact declared type only: a return type that is a base class or an interface does not guarantee a narrower concrete type (an `EntityDefinition`-typed return does not guarantee `ProductDefinition`), so an assertion narrowing to a specific subtype is not covered by this mechanism +- **PHPStan level** — a null-safety or type-coercion property the configured PHPStan level already rejects at compile time +- **Constructor contract** — a property-assignment constructor with no validation logic, whose invariant is the parameter's own type +- **`NoCreateMockWithoutExpectationsRule`** — guarantees a `createMock()` double is never left without an `->expects()`; an assertion re-checking "was this mock called" duplicates what the PHPStan rule already guarantees in Shopware's unit/DevOps test namespaces +- **`CodeCoverageIgnoreEvaluationRule`** — guarantees a method annotated `@codeCoverageIgnore` is truly pass-through (no branching, mutation, or side-effecting calls) *when no `@see` to a dedicated integration/DevOps test lifts the check*; a `@see` present on the annotation means the rule no longer enforces pass-through-only, so this mechanism does not apply and an assertion proving the method's actual behavior is not redundant +- **`TestPackageMatchRule`** — guarantees a test's `#[Package]` attribute matches its covered class's package; an assertion re-checking that pairing duplicates the PHPStan rule +- **`shopware.reflectionOnNonPublicMethod`** — guarantees reflective access to a non-public method of a Shopware class is rejected; an assertion whose only purpose is proving such access is unreachable duplicates that rule + +A finding naming one of these mechanisms for only part of a compound assertion states which part remains unenforced, rather than treating the whole assertion as covered. + ### Deletion Safety - A finding that deletes a test names, per removed assertion, the surviving test that covers it, or states that nothing does. diff --git a/plugins/test-writing/rules/unit/UNIT-003.md b/plugins/test-writing/rules/unit/UNIT-003.md index d44d8742..08db63d1 100644 --- a/plugins/test-writing/rules/unit/UNIT-003.md +++ b/plugins/test-writing/rules/unit/UNIT-003.md @@ -34,6 +34,7 @@ Three-tier dependency substitution hierarchy: - `createStub()` communicates "I only care about what this returns, not how it's called" - `createMock()` communicates "I will verify the interaction via `expects()` or argument callbacks" - A double carrying `->with()` is a mock, not a stub: create it with `createMock()` and give it an explicit `expects()`. UNIT-004 covers what happens when it has neither +- Where `NoCreateMockWithoutExpectationsRule` has already reported a double as never expected, UNIT-003 defers to it rather than re-deriving the same stub-or-expectation transform as a separate finding — suppress the finding and cite the PHPStan rule instead. `NoCreateMockWithoutExpectationsRule` is enabled for `Shopware\Tests\Unit\` and `Shopware\Tests\DevOps\`; UNIT-003's own `test-types: unit` scope means this deferral is live wherever UNIT-003 fires. ### Mocking Decision Matrix diff --git a/plugins/test-writing/rules/unit/UNIT-004.md b/plugins/test-writing/rules/unit/UNIT-004.md index bb805d85..c7219655 100644 --- a/plugins/test-writing/rules/unit/UNIT-004.md +++ b/plugins/test-writing/rules/unit/UNIT-004.md @@ -75,6 +75,14 @@ public function testLoadsProductWithCriteriaVerification(): void } ``` +### Exception: Shared Mock With Sibling Expectations + +A `createMock()` double declared once (typically in `setUp()`) and reused across several test methods, where sibling methods set their own `->expects()` on it, is exempt from the Case 1 remedy (dropping `expects()` entirely). `NoCreateMockWithoutExpectationsRule` requires every method that touches such a double to carry an expectation, but only inside `Shopware\Tests\Unit\` and `Shopware\Tests\DevOps\` — the namespaces listed under `createMockWithoutExpectationsEnabledNamespaces` in Shopware's PHPStan `common.neon`. Applying the Case 1 remedy to one method of a shared double in those namespaces would make that method fail `NoCreateMockWithoutExpectationsRule`, even though this rule's over-coupling anti-pattern still applies to it. This rule's own `test-types: unit` scope means every test it reviews already lives under `Shopware\Tests\Unit\`, one of the two enabled namespaces, so the exception is live wherever UNIT-004 fires; it is documented here for completeness, not because UNIT-004 is ever routed to a migration or integration review. + +### Deferral to NoCreateMockWithoutExpectationsRule + +Where `NoCreateMockWithoutExpectationsRule` has already reported a double as never expected, UNIT-004 (and UNIT-003) do not re-derive the same stub-or-expectation transform as a separate finding. Suppress the finding and cite the PHPStan rule instead. As above, UNIT-004's `test-types: unit` scope means this deferral is live for every test it reviews. + ### Anti-Pattern 2: Missing expects() on with(callback) chain **Rule**: When `->with(static::callback(...))` is present on a mock chain, `->expects(...)` MUST also be present. `->method()` alone registers `expects(any())` — the default any-invocation matcher, which zero calls satisfy. If the mocked method is never called, the constraint is never evaluated and the callback never runs, so the assertions inside it silently do not execute. @@ -124,3 +132,30 @@ $this->filesystem ->method('write') ->with('output/report.csv', static::isString()); ``` + +Also legitimate: a call-count expectation that forms a **reachability pairing** across two test methods — `never()` on the collaborator in one method's branch, `once()` or `atLeastOnce()` on it in the other branch's method. The expectation is not redundant with an outcome assertion here; it is itself the assertion that pins which branch was reachable. Never apply the over-coupling remedy to either half of such a pairing. + +```php +// CORRECT - never()/once() pairing pins which branch ran; neither half is redundant +public function testSkipsNotificationWhenAlreadySent(): void +{ + $this->notifier + ->expects($this->never()) + ->method('send'); + + $this->service->process($this->alreadyNotifiedOrder); +} + +public function testSendsNotificationWhenNotYetSent(): void +{ + $this->notifier + ->expects($this->once()) + ->method('send'); + + $this->service->process($this->newOrder); +} +``` + +### Remediation Notes — Stub Conversion Gotchas + +Two API details from Shopware's unit-test guideline matter when a remedy converts a mock to a stub: "Two API details matter when converting: `->with()` exists on mocks only, so a stub asserts its arguments inside `willReturnCallback()` instead, and a fixture helper receiving a stub must type that parameter as `Stub` (`Foo&Stub`) rather than `MockObject`. Both slip past PHPUnit at runtime and only fail in PHPStan." A remedy that drops `->with()` from a mock without moving its assertion into a `willReturnCallback()`, or that leaves a fixture-helper parameter typed `MockObject` after handing it a stub, is incomplete even though the test still runs green under PHPUnit. diff --git a/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md index 32d53c8c..09f6f72a 100644 --- a/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-test-generation -version: 5.1.1 +version: 5.2.0 description: Use this skill when the user asks to generate, write, or create integration tests for a Shopware 6 source class whose contract requires wired-up code — phrases like "generate integration tests for X", "write an integration test for this controller", "test this indexer", "create an integration test for the message handler". Detects supported integration patterns (controller/route, scheduled-task, message-handler, indexer, DAL-persistence flow, multi-service coordinator) and applies a template producing an INTEGRATION-001..008-compliant test using IntegrationTestBehaviour against the real DAL, container, and HTTP/messaging. When the source class is unit-shape (no persistence, no kernel state, no wiring under test), returns SKIPPED and points at phpunit-unit-test-writing. Do NOT activate for unit tests or migration tests (use phpunit-migration-test-generation). user-invocable: true context: fork diff --git a/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md index c26e633e..3db34fdc 100644 --- a/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-test-reviewing -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules @@ -105,7 +105,7 @@ When `{methods}` is provided, apply detection only to the named methods and thei When `{digest}` is set, the supplied text is the only artifact under review: - Do NOT `Read` the test file or the source class. The digest is body-free (class declaration, `#[CoversClass]`, member order, method signatures, attribute lines, property declarations) and self-contained for class-structure rules. -- Force `review_unit=class-structure`. In Phase 4, call `get_rules(test_type=integration, review_unit=class-structure)` with NO `scoped_review`. Apply whatever rules the filter returns — the composed catalog's class-structure rules are CONV-005 and CONV-007. When `{rules}` is also set, instead select the class-structure rules from the inline text per Inline-Rules Mode (`Review unit` == `class-structure`). +- Force `review_unit=class-structure`. In Phase 4, call `get_rules(test_type=integration, review_unit=class-structure)` with NO `scoped_review`. Apply whatever rules the filter returns — the composed catalog's only class-structure rule is CONV-005. When `{rules}` is also set, instead select the class-structure rules from the inline text per Inline-Rules Mode (`Review unit` == `class-structure`). - Report `location` as a member name or attribute from the digest (line numbers are unavailable without the file body). - `{methods}` and `{review_unit}` inputs are subsumed: the digest defines the scope and the unit. @@ -144,6 +144,8 @@ Include for each issue: - **Suggested Fix** — the complete method body after the change. Empty only where the remediation deletes the method entirely; `deleted_methods` then names that method. - **Issue** — names every line present in Current Code and absent from Suggested Fix. Each such line is a removal, and an unnamed removal is a defect in the finding. (The team-review schema names this field `summary`.) +A suggested remediation never changes what an existing assertion pins, and never introduces an assertion as a means of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction: do not emit it. Where the finding IS itself a missing test or a missing assertion (a coverage-gap rule such as INTEGRATION-007 or DESIGN-006), those new assertions are that finding's explicit deliverable: name them in the finding, and never smuggle them into a remediation for an unrelated rule. Deletions ride `removed_assertions`. Re-expressing the same pinned fact in a different call — CONV-012's `assertTrue($a === $b)` becoming `assertSame($b, $a)`, or an ISOLATION-004 literal swap — does not change what the assertion pins. + Include the placement hint as a single line in the Informational section when INTEGRATION-008 fires. Include full passed checks list. diff --git a/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md b/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md index 1e6fd86f..40f66d2e 100644 --- a/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-to-unit-migrating -version: 5.1.1 +version: 5.2.0 description: Use this skill ONLY when the user explicitly requests an audit, migration, or evaluation of whether a Shopware integration test belongs in the unit suite — trigger phrases like "audit integration tests", "migrate integration tests to unit", "is this an integration test or a unit test", "evaluate integration tests for migration", "should this be a unit test instead". Audits tests under tests/integration/ for misplacement and migrates load-bearing-free tests to tests/unit/ using one of six codified refactoring patterns. NOT invoked automatically by reviewing skills — phpunit-integration-test-reviewing emits a placement smoke-alarm hint pointing here, but the user must invoke this skill explicitly to run the deep audit. user-invocable: true allowed-tools: Glob, Grep, Read, Edit, Write, AskUserQuestion, Bash, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md index 7229b29e..30a3bede 100644 --- a/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-migration-test-generation -version: 5.1.1 +version: 5.2.0 description: Use this skill when the user asks to generate, write, or create migration tests for a Shopware 6 migration class — phrases like "generate migration tests", "write a migration test", "create migration test", "test this migration", "test Migration1234Foo". Analyzes the source migration's SQL operations to pick an appropriate pattern (schema-add, schema-remove, data-update, config, mail template), then generates a test that runs against a real database and passes PHPStan and PHPUnit validation. Do NOT activate for unit tests (use phpunit-unit-test-writing) or integration tests of non-migration source classes (use phpunit-integration-test-generation). user-invocable: true context: fork diff --git a/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md index 958205d0..bde67346 100644 --- a/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-migration-test-reviewing -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules @@ -12,7 +12,7 @@ Review a Shopware PHPUnit migration test for compliance with migration testing c ## Overview -Review the test against the composed migration catalog — MIGRATION-001 through MIGRATION-009, all must-fix, together with every convention, design, isolation, and provider rule whose `test-types` declares `migration`, at whatever enforce level each carries. +Review the test against the composed migration catalog — MIGRATION-001..006, MIGRATION-008 and MIGRATION-009, all must-fix, together with every convention, design, isolation, and provider rule whose `test-types` declares `migration`, at whatever enforce level each carries. ## Input @@ -105,7 +105,7 @@ When `{methods}` is provided, apply detection only to the named methods and thei When `{digest}` is set, the supplied text is the only artifact under review: - Do NOT `Read` the test file or the source class. The digest is body-free (class declaration, `#[CoversClass]`, member order, method signatures, attribute lines, property declarations) and self-contained for class-structure rules. -- Force `review_unit=class-structure`. In Phase 4, call `get_rules(test_type=migration, review_unit=class-structure)` with NO `scoped_review`. Apply whatever rules the filter returns — the composed catalog's class-structure rules are CONV-005, CONV-007, and MIGRATION-008. When `{rules}` is also set, instead select the class-structure rules from the inline text per Inline-Rules Mode (`Review unit` == `class-structure`). +- Force `review_unit=class-structure`. In Phase 4, call `get_rules(test_type=migration, review_unit=class-structure)` with NO `scoped_review`. Apply whatever rules the filter returns — the composed catalog's class-structure rules are CONV-005 and MIGRATION-008. When `{rules}` is also set, instead select the class-structure rules from the inline text per Inline-Rules Mode (`Review unit` == `class-structure`). - Report `location` as a member name or attribute from the digest (line numbers are unavailable without the file body). - `{methods}` and `{review_unit}` inputs are subsumed: the digest defines the scope and the unit. @@ -143,6 +143,8 @@ Include for each issue: - **Suggested Fix** — the complete method body after the change. Empty only where the remediation deletes the method entirely; `deleted_methods` then names that method. - **Issue** — names every line present in Current Code and absent from Suggested Fix. Each such line is a removal, and an unnamed removal is a defect in the finding. (The team-review schema names this field `summary`.) +A suggested remediation never changes what an existing assertion pins, and never introduces an assertion as a means of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction: do not emit it. Where the finding IS itself a missing test or a missing assertion (a coverage-gap rule such as MIGRATION-008 or DESIGN-006), those new assertions are that finding's explicit deliverable: name them in the finding, and never smuggle them into a remediation for an unrelated rule. Deletions ride `removed_assertions`. Re-expressing the same pinned fact in a different call — CONV-012's `assertTrue($a === $b)` becoming `assertSame($b, $a)`, or an ISOLATION-004 literal swap — does not change what the assertion pins. + Include full passed checks list. ### Output Contract @@ -163,22 +165,20 @@ errors: # fixed code implies_src_change: false # true ONLY when the fix cannot be made in the test alone deleted_methods: [] # test methods this fix removes ENTIRELY, by bare name; [] when it removes none - removed_assertions: [] # [{assertion, covered_by_test}] per assertion the fix removes + removed_assertions: [] # [{assertion, covered_by_test}] per removed assertion; covered_by_test names the surviving test, or the literal "none — coverage lost" warnings: - - rule_id: CONV-005 - title: "Test Method Ordering" + - rule_id: CONV-014 + title: "Unclear AAA Structure" enforce: should-fix location: MigrationTest.php:60 - method: class-level + method: testMigration current: | # code suggested: | # reordered code implies_src_change: false - deleted_methods: [testMigrationRunsTwice] - removed_assertions: - - assertion: "static::assertTrue(TableHelper::columnExists($this->connection, 'foo', 'bar'))" - covered_by_test: testMigration # the surviving test, or the literal "none — coverage lost" + deleted_methods: [] # a CONV-014 fix reorders statements; it never deletes a method + removed_assertions: [] # nor an assertion — reordering must not change what any assertion pins informational: - rule_id: DESIGN-007 title: "Data Provider Consolidation" @@ -205,7 +205,7 @@ Set `implies_src_change: true` on a finding whose fix cannot be made in the test | ISSUES_FOUND | 1+ errors | | FAILED | Invalid input (file not found, not in tests/migration/, source class missing or not a `MigrationStep`), or a refusal from the deletion after-state check | -MIGRATION-001 through MIGRATION-009 are all must-fix; the composed catalog's should-fix rules (e.g. CONV-005) populate `warnings`, and its consider-level rules populate `informational`. Informational entries never raise `status` — consider-level findings and the guard's `UNRESOLVED` entry alike. A `fail` `{baseline}` sets `ISSUES_FOUND` regardless of this table. A guard refusal sets `FAILED` regardless of this table and of the baseline — `FAILED` outranks every other status. +MIGRATION-001..006, MIGRATION-008 and MIGRATION-009 are all must-fix; the composed catalog's should-fix rules (e.g. CONV-014) populate `warnings`, and its consider-level rules (e.g. CONV-005) populate `informational`. Informational entries never raise `status` — consider-level findings and the guard's `UNRESOLVED` entry alike. A `fail` `{baseline}` sets `ISSUES_FOUND` regardless of this table. A guard refusal sets `FAILED` regardless of this table and of the baseline — `FAILED` outranks every other status. ## Track-Scoped Invocations diff --git a/plugins/test-writing/skills/phpunit-migration-test-reviewing/references/output-format.md b/plugins/test-writing/skills/phpunit-migration-test-reviewing/references/output-format.md index 97e82135..31fb3844 100644 --- a/plugins/test-writing/skills/phpunit-migration-test-reviewing/references/output-format.md +++ b/plugins/test-writing/skills/phpunit-migration-test-reviewing/references/output-format.md @@ -100,7 +100,7 @@ Every other finding carries `implies_src_change: false` and omits the line. The | ISSUES_FOUND | 1+ errors | | FAILED | Invalid input (file not found, not a migration test, source not a MigrationStep), or a refusal from the deletion after-state check | -MIGRATION-001 through MIGRATION-009 are all must-fix. The composed catalog also includes every convention, design, isolation, and provider rule whose `test-types` declares `migration`, some of which are should-fix or consider. A should-fix finding is a warning and yields `NEEDS_ATTENTION` rather than `ISSUES_FOUND`. Informational entries never change status — consider-level findings and the guard's `UNRESOLVED` entry alike. A `fail` `{baseline}` sets `ISSUES_FOUND` regardless of the above; a guard refusal sets `FAILED`, which outranks both. +MIGRATION-001..006, MIGRATION-008 and MIGRATION-009 are all must-fix. The composed catalog also includes every convention, design, isolation, and provider rule whose `test-types` declares `migration`, some of which are should-fix or consider. A should-fix finding is a warning and yields `NEEDS_ATTENTION` rather than `ISSUES_FOUND`. Informational entries never change status — consider-level findings and the guard's `UNRESOLVED` entry alike. A `fail` `{baseline}` sets `ISSUES_FOUND` regardless of the above; a guard refusal sets `FAILED`, which outranks both. ## Example @@ -141,7 +141,7 @@ MIGRATION-001 through MIGRATION-009 are all must-fix. The composed catalog also ## Passed Checks - ✓ Test must not reuse migration helper methods (MIGRATION-003) -- ✓ assertSame over assertEquals (MIGRATION-007) +- ✓ testGetCreationTimestamp present (MIGRATION-008) ``` ## Error Outputs diff --git a/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md index 30e6e3ee..39a17bec 100644 --- a/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-adversarial-reviewing -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md b/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md index 6aea64d9..b0e6c61f 100644 --- a/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-reconciling -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Read, Glob, Grep, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md index f9cb2ad9..82663984 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-team-reviewing -version: 5.1.1 +version: 5.2.0 description: Use this skill when the user asks for a team-based, consensus, multi-reviewer, or red-team review of Shopware PHPUnit tests — trigger phrases like "team review these tests", "consensus review the tests in PR #N", "red-team this test suite", "multi-reviewer audit of tests/...". Reviews unit (tests/unit/), integration (tests/integration/), and migration (tests/migration/) tests in one run over a mixed manifest, routing each file by test type. Accepts file paths, directories, commits, branches, and PRs as input. For a single-reviewer pass, use the matching per-type reviewing skill instead. allowed-tools: Bash, Read, Glob, Grep, AskUserQuestion, Workflow, mcp__plugin_test-writing_test-rules__build_rule_package --- diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/red-team-context.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/red-team-context.md index 3a619dea..c7348de2 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/red-team-context.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/red-team-context.md @@ -8,8 +8,8 @@ Each file gets **K independent adversaries, one per active lens** — `K` is the The three lenses are the three orthogonal ways a test fails its purpose. The axes are **test-agnostic**; each lens draws the rules it cites from its `## RULES` block (the full catalog for the file's `test_type`) rather than a hardcoded per-type ID list: -- **L1 — Tautology hunter** *(does it run for real?)*: would the test pass even if the SUT were broken — over-mocking the SUT or its real collaborators, asserting on stubs/doubles, call-count coupling, guard-clause leakage. (Unit catalog instances: UNIT-001/003/004/005, ISOLATION-001/002, DESIGN-010; integration: INTEGRATION-002 over-mock; migration: idempotency/verification gaps.) -- **L2 — Weak-assertion hunter** *(does it assert enough?)*: do assertions pin the real contract, and are edge and error cases covered? (Unit: CONV-009, DESIGN-005/006, PROVIDER-*; integration: INTEGRATION-007 setup/assertion ratio; migration: MIGRATION-007 assertSame.) +- **L1 — Tautology hunter** *(does it run for real?)*: would the test pass even if the SUT were broken — over-mocking the SUT or its real collaborators, asserting on stubs/doubles, call-count coupling, guard-clause leakage. (Unit catalog instances: UNIT-001/003/004, ISOLATION-001/002, DESIGN-010; integration: INTEGRATION-002 over-mock; migration: idempotency/verification gaps.) +- **L2 — Weak-assertion hunter** *(does it assert enough?)*: do assertions pin the real contract, and are edge and error cases covered? (Unit: CONV-009, DESIGN-005/006, PROVIDER-*; integration: INTEGRATION-007 setup/assertion ratio; migration: CONV-012 assertion-method choice.) - **L3 — Missed-coverage / completeness hunter** *(is it there at all?)*: enumerate the SUT's public surface, branches, and error paths, and introduce findings for those with no test. **No convention lens.** Convention/structure rules are rule-mechanical and high-agreement — the reviewer wave's strength, with the cross-file agent covering cross-file convention drift. A convention adversary would mostly duplicate the reviewers; L3 may opportunistically flag a glaring convention issue, but convention is not a dedicated axis. diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs index 7cd8abef..2c46c6bb 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs @@ -1033,6 +1033,7 @@ function reviewerPrompt(unit, file, label) { lensLine, isDigest ? '\nSTRUCTURAL DIGEST (review this; do not Read the test file):\n```\n' + (file.digest || '') + '\n```' : '', '', + 'REMEDIATION INVARIANT: a `suggested` never changes what an existing assertion pins, and never introduces an assertion as a MEANS of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction, so do not emit it. Where the finding IS itself a missing test or a missing assertion (coverage-gap rules such as MIGRATION-008, DESIGN-006, INTEGRATION-007, and missed-coverage findings), those new assertions are the explicit deliverable of that finding: name them in the finding, never smuggle them into a remediation for an unrelated rule. An assertion a fix removes rides `removed_assertions`. Re-expressing the same pinned fact in a different call (assertTrue($a === $b) becoming assertSame($b, $a), or an ISOLATION-004 literal swap) does not change what the assertion pins.', `STEP 2 — Return your findings strictly as the output schema. ${catLine} If the sub-skill produces an informational placement hint (e.g. the INTEGRATION-008 unit-shape smoke check), include it in findings with enforce="consider" and its rule_id, so it reaches the consensus merge — the schema has no separate informational channel. Set clean=true with findings=[] if nothing fires.`, '', '## RULES', @@ -1073,6 +1074,7 @@ function reconcilePrompt(unit, file, label, own, peers, subsetRules) { '', `PEERS' current findings on this unit:\n${JSON.stringify(peers, null, 1)}`, '', + 'REMEDIATION INVARIANT: a `suggested` never changes what an existing assertion pins, and never introduces an assertion as a MEANS of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction, so do not emit it. Where the finding IS itself a missing test or a missing assertion (coverage-gap rules such as MIGRATION-008, DESIGN-006, INTEGRATION-007, and missed-coverage findings), those new assertions are the explicit deliverable of that finding: name them in the finding, never smuggle them into a remediation for an unrelated rule. An assertion a fix removes rides `removed_assertions`. Re-expressing the same pinned fact in a different call (assertTrue($a === $b) becoming assertSame($b, $a), or an ISOLATION-004 literal swap) does not change what the assertion pins.', 'STEP 2 — Return your revised binding stance (findings + withdrawn) as the schema.', '', '## RULES', @@ -1096,6 +1098,7 @@ function redTeamPrompt(pkg, impression, lens, label, rulesText, degraded) { '', `Your Wave-0 impression (this lens):\n${JSON.stringify(impression, null, 1)}`, '', + 'REMEDIATION INVARIANT: a `suggested` never changes what an existing assertion pins, and never introduces an assertion as a MEANS of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction, so do not emit it. Where the finding IS itself a missing test or a missing assertion (coverage-gap rules such as MIGRATION-008, DESIGN-006, INTEGRATION-007, and missed-coverage findings), those new assertions are the explicit deliverable of that finding: name them in the finding, never smuggle them into a remediation for an unrelated rule. An assertion a fix removes rides `removed_assertions`. Re-expressing the same pinned fact in a different call (assertTrue($a === $b) becoming assertSame($b, $a), or an ISOLATION-004 literal swap) does not change what the assertion pins.', 'STEP 2 — Return challenges/resurrections/new_findings/endorsements/cross_file_inconsistencies for this ONE file as the schema (files array with a single entry).', '', '## RULES', @@ -1117,6 +1120,7 @@ function defensePrompt(file, label, consensus, challenges, subsetRules) { '', `Adversary challenges/resurrections/new_findings for this file:\n${JSON.stringify(challenges, null, 1)}`, '', + 'REMEDIATION INVARIANT: a `suggested` never changes what an existing assertion pins, and never introduces an assertion as a MEANS of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction, so do not emit it. Where the finding IS itself a missing test or a missing assertion (coverage-gap rules such as MIGRATION-008, DESIGN-006, INTEGRATION-007, and missed-coverage findings), those new assertions are the explicit deliverable of that finding: name them in the finding, never smuggle them into a remediation for an unrelated rule. An assertion a fix removes rides `removed_assertions`. Re-expressing the same pinned fact in a different call (assertTrue($a === $b) becoming assertSame($b, $a), or an ISOLATION-004 literal swap) does not change what the assertion pins.', 'STEP 2 — Return revised stance as the schema.', '', '## RULES', diff --git a/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md index 60b32ecf..e819620b 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-generation -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false context: fork diff --git a/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md index 49f36dc3..c1f43606 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-reviewing -version: 5.1.1 +version: 5.2.0 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules @@ -148,6 +148,8 @@ Include for each issue: - **Suggested Fix** — the complete method body after the change. Empty only where the remediation deletes the method entirely; `deleted_methods` then names that method. - **Issue** — names every line present in Current Code and absent from Suggested Fix. Each such line is a removal, and an unnamed removal is a defect in the finding. (The team-review schema names this field `summary`.) +A suggested remediation never changes what an existing assertion pins, and never introduces an assertion as a means of satisfying a structural, layout, naming, or style constraint — a fix that must compensate that way is wrong by construction: do not emit it. Where the finding IS itself a missing test or a missing assertion (a coverage-gap rule such as DESIGN-006), those new assertions are that finding's explicit deliverable: name them in the finding, and never smuggle them into a remediation for an unrelated rule. Deletions ride `removed_assertions`. Re-expressing the same pinned fact in a different call — CONV-012's `assertTrue($a === $b)` becoming `assertSame($b, $a)`, or an ISOLATION-004 literal swap — does not change what the assertion pins. + Include full passed checks list. ### Output Contract diff --git a/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md index e1578f66..cd805a18 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-writing -version: 5.1.1 +version: 5.2.0 description: Use this skill when the user asks to write, generate, create, or add PHPUnit unit tests for a Shopware 6 source class — phrases like "write unit tests for X", "generate tests for ClassName", "create PHPUnit tests", "add test coverage", "test this class", "cover this with tests", "I need tests for", "unit test this", "SW6 unit tests", "Shopware unit tests", "PHPUnit tests for Shopware". Orchestrates the full workflow — source-class category detection (DTO, Service, Flow/Event, DAL, Exception), test generation, MCP-driven review against Shopware unit-test rules, and an inline fix loop that iterates until tests pass. Do NOT activate for integration tests (use phpunit-integration-test-generation), migration tests (use phpunit-migration-test-generation), e2e tests, or non-PHP testing. allowed-tools: Skill, Edit, Read, Glob, TodoWrite, AskUserQuestion, mcp__plugin_dev-tooling_php-tooling ---