Skip to content

Commit 8cc7cf9

Browse files
loevgaardclaude
loevgaard
and
claude
committed
Add unit tests for all EventSubscriber classes and archive openspec change
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 333edf7 commit 8cc7cf9

11 files changed

Lines changed: 648 additions & 0 deletions

File tree

‎openspec/changes/archive/2026-02-16-test-event-subscribers/.openspec.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-02-16

‎openspec/changes/archive/2026-02-16-test-event-subscribers/design.md‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
## Context
2+
3+
The plugin has 5 event subscriber classes with no unit tests:
4+
- `ReviewAutoApprovalSubscriber` — Doctrine `prePersist` listener that auto-approves store/product reviews
5+
- `CheckEligibilityChecksSubscriber` — Cancels review requests exceeding max eligibility checks
6+
- `IncrementEligibilityChecksSubscriber` — Increments the eligibility check counter
7+
- `ResetSubscriber` — Clears processing state before each processing cycle
8+
- `UpdateNextEligibilityCheckSubscriber` — Calculates exponential backoff for next eligibility check
9+
10+
Existing unit tests in the project use Prophecy for mocking, BDD-style naming (`it_*`), and mirror the source directory structure under `tests/Unit/`.
11+
12+
## Goals / Non-Goals
13+
14+
**Goals:**
15+
- Achieve full unit test coverage for all 5 event subscribers
16+
- Test happy paths, edge cases, and boundary conditions
17+
- Follow existing test conventions (Prophecy, BDD naming, directory mirroring)
18+
19+
**Non-Goals:**
20+
- Integration/functional tests (these are pure unit tests)
21+
- Modifying any production code
22+
- Testing the event dispatcher wiring (that's Symfony's responsibility)
23+
24+
## Decisions
25+
26+
**1. Use Prophecy for all mocks (not PHPUnit mocks)**
27+
Rationale: Project convention per CLAUDE.md. All existing tests use `ProphecyTrait`.
28+
29+
**2. Mirror source directory structure**
30+
- `tests/Unit/EventSubscriber/Doctrine/ReviewAutoApprovalSubscriberTest.php`
31+
- `tests/Unit/EventSubscriber/ReviewRequest/CheckEligibilityChecksSubscriberTest.php`
32+
- `tests/Unit/EventSubscriber/ReviewRequest/IncrementEligibilityChecksSubscriberTest.php`
33+
- `tests/Unit/EventSubscriber/ReviewRequest/ResetSubscriberTest.php`
34+
- `tests/Unit/EventSubscriber/ReviewRequest/UpdateNextEligibilityCheckSubscriberTest.php`
35+
36+
Rationale: Matches existing `tests/Unit/` layout (e.g., `Checker/`, `Factory/`, `Form/`).
37+
38+
**3. Prophesize `ReviewRequestInterface` for ReviewRequest subscribers**
39+
The four ReviewRequest subscribers all operate on `ReviewRequestProcessingStarted` which holds a `ReviewRequestInterface`. Use Prophecy to mock the interface and verify method calls.
40+
41+
**4. Prophesize `PrePersistEventArgs` for Doctrine subscriber**
42+
`ReviewAutoApprovalSubscriber.prePersist()` takes Doctrine's `PrePersistEventArgs`. Mock `getObject()` to return different entity types for each scenario.
43+
44+
**5. Test `getSubscribedEvents()` static method**
45+
Verify the event-to-method mapping and priority for each `EventSubscriberInterface` implementation.
46+
47+
## Risks / Trade-offs
48+
49+
**[Risk] Prophecy mocking of `PrePersistEventArgs`** — Doctrine event args may have constructor requirements.
50+
→ Mitigation: Prophecy handles this since it doesn't call constructors. Verified pattern works with Prophecy.
51+
52+
**[Risk] Time-dependent test for `UpdateNextEligibilityCheckSubscriber`** — Uses `new \DateTimeImmutable()` internally.
53+
→ Mitigation: Assert the calculated datetime is within an acceptable range rather than exact equality, or freeze time via `ClockMock` if available. Since the calculation is deterministic (based on eligibility checks count), we can compute the expected value independently.

‎openspec/changes/archive/2026-02-16-test-event-subscribers/proposal.md‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
## Why
2+
3+
The 5 event subscribers in `src/EventSubscriber/` (Doctrine and ReviewRequest namespaces) have zero unit test coverage. Adding tests ensures correct behavior for auto-approval logic, eligibility check lifecycle, and exponential backoff scheduling, and prevents regressions.
4+
5+
## What Changes
6+
7+
- Add unit tests for `ReviewAutoApprovalSubscriber` (Doctrine prePersist auto-approval for store and product reviews)
8+
- Add unit tests for `CheckEligibilityChecksSubscriber` (cancels review request when max checks exceeded)
9+
- Add unit tests for `IncrementEligibilityChecksSubscriber` (increments eligibility check counter)
10+
- Add unit tests for `ResetSubscriber` (clears ineligibility reason and processing error)
11+
- Add unit tests for `UpdateNextEligibilityCheckSubscriber` (exponential backoff for next eligibility check)
12+
13+
## Capabilities
14+
15+
### New Capabilities
16+
- `event-subscriber-tests`: Unit tests covering all 5 event subscriber classes in `src/EventSubscriber/`
17+
18+
### Modified Capabilities
19+
20+
_(none)_
21+
22+
## Impact
23+
24+
- New test files in `tests/Unit/EventSubscriber/`
25+
- No production code changes
26+
- No dependency changes

‎openspec/changes/archive/2026-02-16-test-event-subscribers/specs/event-subscriber-tests/spec.md‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
## ADDED Requirements
2+
3+
### Requirement: ReviewAutoApprovalSubscriber sets accepted status for auto-approved store reviews
4+
The test SHALL verify that when a `StoreReviewInterface` entity triggers `prePersist` and the store auto-approval checker returns true, the entity's status is set to `ReviewInterface::STATUS_ACCEPTED`.
5+
6+
#### Scenario: Store review is auto-approved
7+
- **WHEN** `prePersist` is called with a `StoreReviewInterface` entity and the store checker returns `shouldAutoApprove() === true`
8+
- **THEN** the entity's status SHALL be set to `ReviewInterface::STATUS_ACCEPTED`
9+
10+
#### Scenario: Store review is not auto-approved
11+
- **WHEN** `prePersist` is called with a `StoreReviewInterface` entity and the store checker returns `shouldAutoApprove() === false`
12+
- **THEN** the entity's status SHALL NOT be changed
13+
14+
### Requirement: ReviewAutoApprovalSubscriber sets accepted status for auto-approved product reviews
15+
The test SHALL verify that when a `ProductReviewInterface` entity triggers `prePersist` and the product auto-approval checker returns true, the entity's status is set to `ReviewInterface::STATUS_ACCEPTED`.
16+
17+
#### Scenario: Product review is auto-approved
18+
- **WHEN** `prePersist` is called with a `ProductReviewInterface` entity and the product checker returns `shouldAutoApprove() === true`
19+
- **THEN** the entity's status SHALL be set to `ReviewInterface::STATUS_ACCEPTED`
20+
21+
#### Scenario: Product review is not auto-approved
22+
- **WHEN** `prePersist` is called with a `ProductReviewInterface` entity and the product checker returns `shouldAutoApprove() === false`
23+
- **THEN** the entity's status SHALL NOT be changed
24+
25+
### Requirement: ReviewAutoApprovalSubscriber ignores unrelated entities
26+
The test SHALL verify that entities not implementing `StoreReviewInterface` or `ProductReviewInterface` are ignored.
27+
28+
#### Scenario: Unrelated entity triggers prePersist
29+
- **WHEN** `prePersist` is called with an entity that is neither a store review nor a product review
30+
- **THEN** no checker SHALL be called and no status SHALL be changed
31+
32+
### Requirement: CheckEligibilityChecksSubscriber cancels requests exceeding maximum checks
33+
The test SHALL verify that when a review request's eligibility checks exceed the maximum, the workflow cancel transition is applied and a processing error is set.
34+
35+
#### Scenario: Eligibility checks exceed maximum
36+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns a value greater than `maximumChecks`
37+
- **THEN** the workflow `cancel` transition SHALL be applied and `setProcessingError` SHALL be called with an error message
38+
39+
#### Scenario: Eligibility checks equal maximum
40+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns exactly `maximumChecks`
41+
- **THEN** no workflow transition SHALL be applied (checks <= max is allowed)
42+
43+
#### Scenario: Eligibility checks below maximum
44+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns a value less than `maximumChecks`
45+
- **THEN** no workflow transition SHALL be applied
46+
47+
### Requirement: CheckEligibilityChecksSubscriber subscribes with correct priority
48+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
49+
50+
#### Scenario: Subscribed events mapping
51+
- **WHEN** `getSubscribedEvents()` is called
52+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['check', 200]`
53+
54+
### Requirement: IncrementEligibilityChecksSubscriber increments the counter
55+
The test SHALL verify that `incrementEligibilityChecks()` is called on the review request.
56+
57+
#### Scenario: Counter is incremented
58+
- **WHEN** a `ReviewRequestProcessingStarted` event fires
59+
- **THEN** `incrementEligibilityChecks()` SHALL be called on the review request
60+
61+
### Requirement: IncrementEligibilityChecksSubscriber subscribes with correct priority
62+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
63+
64+
#### Scenario: Subscribed events mapping
65+
- **WHEN** `getSubscribedEvents()` is called
66+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['incrementEligibilityChecks', 300]`
67+
68+
### Requirement: ResetSubscriber clears processing state
69+
The test SHALL verify that both `setIneligibilityReason(null)` and `setProcessingError(null)` are called.
70+
71+
#### Scenario: State is reset
72+
- **WHEN** a `ReviewRequestProcessingStarted` event fires
73+
- **THEN** `setIneligibilityReason(null)` and `setProcessingError(null)` SHALL both be called on the review request
74+
75+
### Requirement: ResetSubscriber subscribes with correct priority
76+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
77+
78+
#### Scenario: Subscribed events mapping
79+
- **WHEN** `getSubscribedEvents()` is called
80+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['reset', 400]`
81+
82+
### Requirement: UpdateNextEligibilityCheckSubscriber calculates exponential backoff
83+
The test SHALL verify that `setNextEligibilityCheckAt()` is called with a datetime computed as `initialDelayHours * 2^(eligibilityChecks - 1)` hours from now.
84+
85+
#### Scenario: First eligibility check (checks = 1, default delay = 24h)
86+
- **WHEN** a `ReviewRequestProcessingStarted` event fires with `getEligibilityChecks() === 1` and default `initialDelayHours` of 24
87+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 24 hours from now
88+
89+
#### Scenario: Second eligibility check (checks = 2, default delay = 24h)
90+
- **WHEN** a `ReviewRequestProcessingStarted` event fires with `getEligibilityChecks() === 2` and default `initialDelayHours` of 24
91+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 48 hours from now
92+
93+
#### Scenario: Custom initial delay
94+
- **WHEN** the subscriber is constructed with `initialDelayHours = 12` and event fires with `getEligibilityChecks() === 1`
95+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 12 hours from now
96+
97+
### Requirement: UpdateNextEligibilityCheckSubscriber subscribes with correct priority
98+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
99+
100+
#### Scenario: Subscribed events mapping
101+
- **WHEN** `getSubscribedEvents()` is called
102+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['updateNextEligibilityCheck', 100]`

‎openspec/changes/archive/2026-02-16-test-event-subscribers/tasks.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
## 1. Doctrine Event Subscriber Tests
2+
3+
- [x] 1.1 Create `tests/Unit/EventSubscriber/Doctrine/ReviewAutoApprovalSubscriberTest.php` with tests: store review auto-approved, store review not auto-approved, product review auto-approved, product review not auto-approved, unrelated entity ignored
4+
5+
## 2. ReviewRequest Event Subscriber Tests
6+
7+
- [x] 2.1 Create `tests/Unit/EventSubscriber/ReviewRequest/CheckEligibilityChecksSubscriberTest.php` with tests: cancels when checks exceed max, no-op when checks equal max, no-op when checks below max, correct subscribed events mapping
8+
- [x] 2.2 Create `tests/Unit/EventSubscriber/ReviewRequest/IncrementEligibilityChecksSubscriberTest.php` with tests: increments counter, correct subscribed events mapping
9+
- [x] 2.3 Create `tests/Unit/EventSubscriber/ReviewRequest/ResetSubscriberTest.php` with tests: clears ineligibility reason and processing error, correct subscribed events mapping
10+
- [x] 2.4 Create `tests/Unit/EventSubscriber/ReviewRequest/UpdateNextEligibilityCheckSubscriberTest.php` with tests: exponential backoff for checks 1 and 2 with default delay, custom initial delay, correct subscribed events mapping
11+
12+
## 3. Verify
13+
14+
- [x] 3.1 Run full unit test suite and confirm all new tests pass
15+
- [x] 3.2 Run PHPStan to confirm no static analysis errors

‎openspec/specs/event-subscriber-tests/spec.md‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
## Requirements
2+
3+
### Requirement: ReviewAutoApprovalSubscriber sets accepted status for auto-approved store reviews
4+
The test SHALL verify that when a `StoreReviewInterface` entity triggers `prePersist` and the store auto-approval checker returns true, the entity's status is set to `ReviewInterface::STATUS_ACCEPTED`.
5+
6+
#### Scenario: Store review is auto-approved
7+
- **WHEN** `prePersist` is called with a `StoreReviewInterface` entity and the store checker returns `shouldAutoApprove() === true`
8+
- **THEN** the entity's status SHALL be set to `ReviewInterface::STATUS_ACCEPTED`
9+
10+
#### Scenario: Store review is not auto-approved
11+
- **WHEN** `prePersist` is called with a `StoreReviewInterface` entity and the store checker returns `shouldAutoApprove() === false`
12+
- **THEN** the entity's status SHALL NOT be changed
13+
14+
### Requirement: ReviewAutoApprovalSubscriber sets accepted status for auto-approved product reviews
15+
The test SHALL verify that when a `ProductReviewInterface` entity triggers `prePersist` and the product auto-approval checker returns true, the entity's status is set to `ReviewInterface::STATUS_ACCEPTED`.
16+
17+
#### Scenario: Product review is auto-approved
18+
- **WHEN** `prePersist` is called with a `ProductReviewInterface` entity and the product checker returns `shouldAutoApprove() === true`
19+
- **THEN** the entity's status SHALL be set to `ReviewInterface::STATUS_ACCEPTED`
20+
21+
#### Scenario: Product review is not auto-approved
22+
- **WHEN** `prePersist` is called with a `ProductReviewInterface` entity and the product checker returns `shouldAutoApprove() === false`
23+
- **THEN** the entity's status SHALL NOT be changed
24+
25+
### Requirement: ReviewAutoApprovalSubscriber ignores unrelated entities
26+
The test SHALL verify that entities not implementing `StoreReviewInterface` or `ProductReviewInterface` are ignored.
27+
28+
#### Scenario: Unrelated entity triggers prePersist
29+
- **WHEN** `prePersist` is called with an entity that is neither a store review nor a product review
30+
- **THEN** no checker SHALL be called and no status SHALL be changed
31+
32+
### Requirement: CheckEligibilityChecksSubscriber cancels requests exceeding maximum checks
33+
The test SHALL verify that when a review request's eligibility checks exceed the maximum, the workflow cancel transition is applied and a processing error is set.
34+
35+
#### Scenario: Eligibility checks exceed maximum
36+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns a value greater than `maximumChecks`
37+
- **THEN** the workflow `cancel` transition SHALL be applied and `setProcessingError` SHALL be called with an error message
38+
39+
#### Scenario: Eligibility checks equal maximum
40+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns exactly `maximumChecks`
41+
- **THEN** no workflow transition SHALL be applied (checks <= max is allowed)
42+
43+
#### Scenario: Eligibility checks below maximum
44+
- **WHEN** a `ReviewRequestProcessingStarted` event fires and `getEligibilityChecks()` returns a value less than `maximumChecks`
45+
- **THEN** no workflow transition SHALL be applied
46+
47+
### Requirement: CheckEligibilityChecksSubscriber subscribes with correct priority
48+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
49+
50+
#### Scenario: Subscribed events mapping
51+
- **WHEN** `getSubscribedEvents()` is called
52+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['check', 200]`
53+
54+
### Requirement: IncrementEligibilityChecksSubscriber increments the counter
55+
The test SHALL verify that `incrementEligibilityChecks()` is called on the review request.
56+
57+
#### Scenario: Counter is incremented
58+
- **WHEN** a `ReviewRequestProcessingStarted` event fires
59+
- **THEN** `incrementEligibilityChecks()` SHALL be called on the review request
60+
61+
### Requirement: IncrementEligibilityChecksSubscriber subscribes with correct priority
62+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
63+
64+
#### Scenario: Subscribed events mapping
65+
- **WHEN** `getSubscribedEvents()` is called
66+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['incrementEligibilityChecks', 300]`
67+
68+
### Requirement: ResetSubscriber clears processing state
69+
The test SHALL verify that both `setIneligibilityReason(null)` and `setProcessingError(null)` are called.
70+
71+
#### Scenario: State is reset
72+
- **WHEN** a `ReviewRequestProcessingStarted` event fires
73+
- **THEN** `setIneligibilityReason(null)` and `setProcessingError(null)` SHALL both be called on the review request
74+
75+
### Requirement: ResetSubscriber subscribes with correct priority
76+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
77+
78+
#### Scenario: Subscribed events mapping
79+
- **WHEN** `getSubscribedEvents()` is called
80+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['reset', 400]`
81+
82+
### Requirement: UpdateNextEligibilityCheckSubscriber calculates exponential backoff
83+
The test SHALL verify that `setNextEligibilityCheckAt()` is called with a datetime computed as `initialDelayHours * 2^(eligibilityChecks - 1)` hours from now.
84+
85+
#### Scenario: First eligibility check (checks = 1, default delay = 24h)
86+
- **WHEN** a `ReviewRequestProcessingStarted` event fires with `getEligibilityChecks() === 1` and default `initialDelayHours` of 24
87+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 24 hours from now
88+
89+
#### Scenario: Second eligibility check (checks = 2, default delay = 24h)
90+
- **WHEN** a `ReviewRequestProcessingStarted` event fires with `getEligibilityChecks() === 2` and default `initialDelayHours` of 24
91+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 48 hours from now
92+
93+
#### Scenario: Custom initial delay
94+
- **WHEN** the subscriber is constructed with `initialDelayHours = 12` and event fires with `getEligibilityChecks() === 1`
95+
- **THEN** `setNextEligibilityCheckAt()` SHALL be called with a datetime approximately 12 hours from now
96+
97+
### Requirement: UpdateNextEligibilityCheckSubscriber subscribes with correct priority
98+
The test SHALL verify `getSubscribedEvents()` returns the correct event class, method, and priority.
99+
100+
#### Scenario: Subscribed events mapping
101+
- **WHEN** `getSubscribedEvents()` is called
102+
- **THEN** it SHALL return `ReviewRequestProcessingStarted::class` mapped to `['updateNextEligibilityCheck', 100]`

0 commit comments

Comments
 (0)