fix: add unit test for check the id on event listener#565
fix: add unit test for check the id on event listener#565andrestejerina97 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesAudit listener changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-565/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/OpenTelemetry/Formatters/AuditEventListenerTest.php`:
- Around line 225-267: The test in AuditEventListenerTest is asserting a
deferred “buffer then dispatch” flow, but AuditEventListener::onFlush currently
audits entity insertions immediately, so the captured ID will still be 0. Update
the test to match the actual behavior or, if buffering is intended, add the
missing postFlush dispatch path in AuditEventListener and move the audit call
out of onFlush. Also fix the assertion logic/messages so they reflect the real
expectation around when IAuditStrategy::audit is invoked and what ID value
should be observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 368f12d9-43c3-4726-8852-951776905f9b
📒 Files selected for processing (2)
app/Audit/AuditEventListener.phptests/OpenTelemetry/Formatters/AuditEventListenerTest.php
| public function testCreationAuditIsBufferedInOnFlushAndDispatchedInPostFlush(): void | ||
| { | ||
| // Simulate a new entity not yet persisted: getId() returns 0 (pre-INSERT state). | ||
| $entity = $this->getMockBuilder(\models\summit\SummitEvent::class) | ||
| ->disableOriginalConstructor() | ||
| ->onlyMethods(['getId']) | ||
| ->getMock(); | ||
| $entity->method('getId')->willReturn(0); | ||
|
|
||
| $uow = $this->createMock(UnitOfWork::class); | ||
| $uow->method('getScheduledEntityInsertions')->willReturn([$entity]); | ||
| $uow->method('getScheduledEntityUpdates')->willReturn([]); | ||
| $uow->method('getScheduledEntityDeletions')->willReturn([]); | ||
| $uow->method('getScheduledCollectionDeletions')->willReturn([]); | ||
| $uow->method('getScheduledCollectionUpdates')->willReturn([]); | ||
|
|
||
| $em = $this->createMock(EntityManagerInterface::class); | ||
| $em->method('getUnitOfWork')->willReturn($uow); | ||
|
|
||
| $strategy = $this->createMock(IAuditStrategy::class); | ||
|
|
||
| $listener = $this->getMockBuilder(AuditEventListener::class) | ||
| ->onlyMethods(['shouldSkipInTest', 'getAuditStrategy', 'buildAuditContext']) | ||
| ->getMock(); | ||
| $listener->method('shouldSkipInTest')->willReturn(false); | ||
| $listener->method('getAuditStrategy')->willReturn($strategy); | ||
| $listener->method('buildAuditContext')->willReturn(new AuditContext()); | ||
|
|
||
| $capturedId = null; | ||
| $strategy->method('audit')->willReturnCallback( | ||
| function ($subject) use (&$capturedId) { | ||
| $capturedId = method_exists($subject, 'getId') ? $subject->getId() : null; | ||
| } | ||
| ); | ||
|
|
||
| $listener->onFlush(new OnFlushEventArgs($em)); | ||
|
|
||
| // Bug: audit() was called during onFlush — before the INSERT — so the captured id is 0. | ||
| $this->assertNotNull($capturedId, | ||
| 'audit() must not be called during onFlush (INSERT has not run yet)'); | ||
| $this->assertNotEquals(0, $capturedId, | ||
| 'entity id captured during onFlush is 0 — the INSERT has not assigned the real id yet'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\b(postFlush|getSubscribedEvents|buffer|deferred?)\b' app/Audit/AuditEventListener.php
ast-grep outline app/Audit/AuditEventListener.phpRepository: OpenStackweb/summit-api
Length of output: 208
🏁 Script executed:
ast-grep outline app/Audit/AuditEventListener.phpRepository: OpenStackweb/summit-api
Length of output: 208
🏁 Script executed:
sed -n '/function onFlush/,/^ }$/p' app/Audit/AuditEventListener.phpRepository: OpenStackweb/summit-api
Length of output: 2213
🏁 Script executed:
#!/bin/bash
# Inspect the specific test logic to confirm the assertion contradiction
sed -n '225,267p' tests/OpenTelemetry/Formatters/AuditEventListenerTest.phpRepository: OpenStackweb/summit-api
Length of output: 2257
Production listener ignores intended "buffer then dispatch" strategy; test assertions and messages are inverted.
The onFlush implementation in app/Audit/AuditEventListener.php executes audits immediately during the onFlush event:
foreach ($uow->getScheduledEntityInsertions() as $entity) {
$strategy->audit($entity, [], IAuditStrategy::EVENT_ENTITY_CREATION, $ctx);
}There is no buffering logic or postFlush subscriber method to defer this until the database INSERT completes. Consequently, the test testCreationAuditIsBufferedInOnFlushAndDispatchedInPostFlush correctly captures the pre-INSERT ID 0, causing the assertNotEquals(0, $capturedId) to fail.
Additionally, the assertion messages are logically inverted:
assertNotNullpasses ifauditruns, yet the message claims "must not be called".assertNotEqualsfails because ID IS 0, yet the message states "INSERT has not assigned the real id yet" as if asserting a success condition.
Align the assertion logic with the current behavior or implement the deferred postFlush dispatching if that is the desired requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/OpenTelemetry/Formatters/AuditEventListenerTest.php` around lines 225 -
267, The test in AuditEventListenerTest is asserting a deferred “buffer then
dispatch” flow, but AuditEventListener::onFlush currently audits entity
insertions immediately, so the captured ID will still be 0. Update the test to
match the actual behavior or, if buffering is intended, add the missing
postFlush dispatch path in AuditEventListener and move the audit call out of
onFlush. Also fix the assertion logic/messages so they reflect the real
expectation around when IAuditStrategy::audit is invoked and what ID value
should be observed.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests