test: Add verification for exception property handling in ErrorHandler class.
#103
Conversation
WalkthroughAdds tests for ErrorHandler and StatelessApplication and a configurable logger-flush property on StatelessApplication; tests verify ErrorHandler leaves its internal exception property null and that exceptions are logged when a FileTarget is configured. StatelessApplication.terminate() now conditionally flushes the instance logger based on a new public property. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant App as StatelessApplication
participant Handler as ErrorHandler
participant Logger as AppLogger
Client->>App: send request
App->>App: handleRequest()
alt exception thrown
App->>Handler: handleException(e)
Handler-->>App: Response(500)
note right of Handler #E8F5E9: test reads internal exception\nproperty before/after and asserts null
App->>Logger: log(error, e)
end
alt flushLogger == true
App->>Logger: flush(true)
note right of Logger #FFF3E0: instance logger flush called\n($this->getLog()->getLogger()->flush(true))
else flushLogger == false
note right of App #FFEBEE: no logger flush performed
end
App-->>Client: send Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 319 320 +1
===========================================
Files 12 12
Lines 809 810 +1
===========================================
+ Hits 809 810 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/http/ErrorHandlerTest.php (1)
156-184: Align test name with assertions or assert interim “set” state.The method name suggests verifying both that the exception property is set during handling and reset after. Currently it only checks null before and null after. Either:
- Rename to better reflect behavior, or
- Assert that the property is set during rendering via a small test double.
Example (preferred): capture the property during render to assert it was set, then still verify it resets:
- public function testHandleExceptionSetsExceptionPropertyAndResetsIt(): void + public function testHandleExceptionSetsExceptionPropertyAndResetsIt(): void { - $errorHandler = new ErrorHandler(); - - $errorHandler->discardExistingOutput = false; + $errorHandler = new class () extends ErrorHandler { + public ?Throwable $capturedException = null; + protected function renderException($exception): Response + { + // Assert property is set during handling. + $this->capturedException = $this->exception; + return parent::renderException($exception); + } + }; + $errorHandler->discardExistingOutput = false; $initialException = self::inaccessibleProperty($errorHandler, 'exception'); self::assertNull( $initialException, "Exception property should be 'null' initially.", ); $exception = new Exception('Test exception for property verification'); $response = $errorHandler->handleException($exception); + self::assertSame( + $exception, + $errorHandler->capturedException, + 'Exception property should be set while handling exception.', + ); + $finalException = self::inaccessibleProperty($errorHandler, 'exception'); self::assertNull( $finalException, "Exception property should be reset to 'null' after handling exception.", ); self::assertSame( 500, $response->getStatusCode(), 'Should set correct status code and reset exception property.', ); }If you prefer not to use a test double, consider renaming to
testHandleExceptionResetsExceptionProperty()to match the current assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/http/ErrorHandlerTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/http/ErrorHandlerTest.php (1)
src/http/ErrorHandler.php (2)
ErrorHandler(39-263)handleException(113-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (1)
tests/http/ErrorHandlerTest.php (1)
156-184: Good addition: verifies exception state is null before/after and status code 500.This test strengthens the contract around ErrorHandler::handleException resetting its internal state and returning a 500 for generic exceptions. Looks correct and consistent with the implementation.
…corresponding test for exception logging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/http/StatelessApplication.php (1)
463-465: Conditional flush via instance logger is correct; minor optimization possible.Using the instance logger path (
$this->getLog()->getLogger()) instead of the global is the right move here. If you want to shave a tiny bit of overhead, you could skip flushing when there are no buffered messages:- if ($this->flushLogger) { - $this->getLog()->getLogger()->flush(true); - } + if ($this->flushLogger) { + $logger = $this->getLog()->getLogger(); + if (!empty($logger->messages)) { + $logger->flush(true); + } + }Also consider updating the method doc above to say it “conditionally flushes the logger” to reflect the new behavior.
tests/http/StatelessApplicationTest.php (1)
583-652: Solid test of exception logging; tighten a couple of details for robustness.
- Prefer using the class constant for the expected category to avoid string drift.
- Make the test’s intent explicit by asserting the app is configured not to flush.
Proposed changes:
- $expectedCategory = 'yii\base\Exception'; + $expectedCategory = Exception::class;$app = $this->statelessApplication( [ 'flushLogger' => false, 'components' => [ 'errorHandler' => [ 'errorAction' => null, ], 'log' => [ 'traceLevel' => YII_DEBUG ? 1 : 0, 'targets' => [ [ 'class' => FileTarget::class, 'levels' => [ 'error', ], ], ], ], ], ], ); + + self::assertFalse( + $app->flushLogger, + "Test must keep logger messages in memory to assert on them; 'flushLogger' should be false." + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/http/StatelessApplication.php(2 hunks)tests/http/StatelessApplicationTest.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T16:24:09.241Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#53
File: src/http/ErrorHandler.php:258-272
Timestamp: 2025-08-03T16:24:09.241Z
Learning: In yii2-extensions/psr-bridge, the StatelessApplication creates a new Response instance for each request in the reset() method, then passes it to ErrorHandler::setResponse(). This means the template response is not shared across requests, so calling clear() on it in createErrorResponse() is safe and doesn't cause side effects.
Applied to files:
src/http/StatelessApplication.php
🧬 Code Graph Analysis (2)
src/http/StatelessApplication.php (1)
tests/support/stub/MockerFunctions.php (1)
flush(93-96)
tests/http/StatelessApplicationTest.php (3)
tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createServerRequestCreator(154-161)tests/TestCase.php (1)
statelessApplication(124-187)src/http/StatelessApplication.php (1)
handle(269-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (2)
src/http/StatelessApplication.php (1)
51-65: Configurable logger flush flag is a good addition (sensible default and clear docs).Public bool with default true fits the worker use-case and stays BC. The docblock sets expectations and warns appropriately. This integrates cleanly with your delayed parent::__construct($config) call in reset(), so
'flushLogger' => falsein app config is honored at runtime.tests/http/StatelessApplicationTest.php (1)
16-16: Importing FileTarget is necessary for the new logging test.This aligns the test with the configured log target.
Summary by CodeRabbit
New Features
Tests