test(http): Extract error handler from ApplicationTest to ApplicationErrorHandlerTest class.
#138
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ionErrorHandlerTest` class.
WalkthroughAdds a new dedicated ApplicationErrorHandlerTest with extensive error-handling tests, removes corresponding error-path tests from ApplicationTest, and extends StatelessApplicationProvider with test data for exception rendering formats. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StatelessApp
participant ErrorHandler
participant Logger
participant View
Client->>StatelessApp: HTTP request
alt route resolves and handler succeeds
StatelessApp-->>Client: 200 OK Response
else exception thrown during handling
StatelessApp->>ErrorHandler: handle(exception)
ErrorHandler->>Logger: log(exception)
ErrorHandler->>View: render(format: HTML/JSON/RAW)
View-->>ErrorHandler: rendered body
ErrorHandler-->>Client: error Response (status, headers, body)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ 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 #138 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 318 318
===========================================
Files 12 12
Lines 808 808
===========================================
Hits 808 808 ☔ 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: 4
🧹 Nitpick comments (3)
tests/http/stateless/ApplicationTest.php (1)
120-132: Prefer structural JSON assertions over exact-string equality to reduce brittleness.Comparing decoded payloads is more resilient to insignificant formatting changes (whitespace, key order) while still asserting correctness.
Apply this diff:
- self::assertSame( - <<<JSON - {"foo":"bar","a":{"b":"c"}} - JSON, - $response->getBody()->getContents(), - "Response 'body' should match expected JSON string '{\"foo\":\"bar\",\"a\":{\"b\":\"c\"}}' for " . - "'site/post' route in 'StatelessApplication'.", - ); + $body = $response->getBody()->getContents(); + self::assertSame( + ['foo' => 'bar', 'a' => ['b' => 'c']], + json_decode($body, true), + "Response 'body' should decode to expected structure for 'site/post' route in 'StatelessApplication'.", + );tests/http/stateless/ApplicationErrorHandlerTest.php (2)
7-16: Add PHPUnit Group attribute to keep parity with ApplicationTest and preserve group-based runs.ApplicationTest is grouped under 'http'; mirroring that here maintains filtering consistency in CI and local runs.
Apply this diff:
-use PHPUnit\Framework\Attributes\RequiresPhpExtension; +use PHPUnit\Framework\Attributes\{Group, RequiresPhpExtension}; @@ -final class ApplicationErrorHandlerTest extends TestCase +#[Group('http')] +final class ApplicationErrorHandlerTest extends TestCaseAlso applies to: 27-28
144-168: Remove duplicated assertions to reduce noise.The status code and Content-Type assertions appear twice back-to-back; keep the first set only.
Apply this diff:
- self::assertSame( - 500, - $response->getStatusCode(), - "Expected HTTP '500' for route 'site/trigger-exception'.", - ); - self::assertSame( - 'text/html; charset=UTF-8', - $response->getHeaderLine('Content-Type'), - "Expected Content-Type 'text/html; charset=UTF-8' for route 'site/trigger-exception'.", - );
📜 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)
tests/http/stateless/ApplicationErrorHandlerTest.php(1 hunks)tests/http/stateless/ApplicationTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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:
tests/http/stateless/ApplicationErrorHandlerTest.phptests/http/stateless/ApplicationTest.php
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods, so individual test methods that extend TestCase don't need manual $_SERVER restoration.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.phptests/http/stateless/ApplicationTest.php
📚 Learning: 2025-07-20T16:35:15.341Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1536-1552
Timestamp: 2025-07-20T16:35:15.341Z
Learning: In the yii2-extensions/psr-bridge project, the base TestCase class already handles $_SERVER cleanup in setUp() and tearDown() methods (lines 28 and 32), so individual test methods that extend TestCase don't need manual $_SERVER restoration.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.phptests/http/stateless/ApplicationTest.php
📚 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 (line 408: `$this->response = new Response($this->components['response'] ?? []);`), 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:
tests/http/stateless/ApplicationTest.php
📚 Learning: 2025-07-20T16:33:57.495Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#6
File: tests/http/RequestTest.php:1564-1578
Timestamp: 2025-07-20T16:33:57.495Z
Learning: The TestCase class in yii2-extensions/psr-bridge automatically handles $_SERVER superglobal cleanup by saving its original state before each test and restoring it afterward in setUp() and tearDown() methods. Manual $_SERVER cleanup in individual test methods is unnecessary when extending this TestCase.
Applied to files:
tests/http/stateless/ApplicationTest.php
📚 Learning: 2025-08-08T15:24:06.085Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/TestCase.php:23-27
Timestamp: 2025-08-08T15:24:06.085Z
Learning: In yii2-extensions/psr-bridge (tests/TestCase.php), maintainer preference: it’s acceptable to use random-looking strings for test-only constants like COOKIE_VALIDATION_KEY; no need to replace with an obviously non-secret value unless CI/secret scanners become problematic.
Applied to files:
tests/http/stateless/ApplicationTest.php
📚 Learning: 2025-08-06T22:52:05.608Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#64
File: tests/http/StatelessApplicationTest.php:1939-1967
Timestamp: 2025-08-06T22:52:05.608Z
Learning: In yii2-extensions/psr-bridge tests, when testing specific component methods like Request::resolve(), it's necessary to call $app->handle($request) first to initialize all application components before testing the method in isolation. This ensures proper component lifecycle initialization.
Applied to files:
tests/http/stateless/ApplicationTest.php
🧬 Code graph analysis (2)
tests/http/stateless/ApplicationErrorHandlerTest.php (6)
src/http/Response.php (1)
Response(38-150)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createServerRequestCreator(154-161)tests/TestCase.php (2)
TestCase(23-227)statelessApplication(134-197)src/http/StatelessApplication.php (1)
handle(269-295)src/exception/Message.php (1)
getMessage(223-226)src/http/Request.php (1)
resolve(767-787)
tests/http/stateless/ApplicationTest.php (1)
src/http/StatelessApplication.php (1)
StatelessApplication(49-553)
⏰ 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.4-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (5)
tests/http/stateless/ApplicationTest.php (2)
7-9: Imports and scope narrowing look good.The import set reflects the class’s reduced responsibility post-extraction and aligns with the PR objective to keep this test focused on non-error scenarios. LGTM.
47-97: Cookie assertions: correct handling of session cookie and flags.Skipping the session cookie via a prefix check avoids false positives created by Response injecting the session cookie during getPsr7Response(), while still validating attributes of application-set cookies. This matches project behavior noted in prior learnings about fresh Response per request and is appropriate here.
tests/http/stateless/ApplicationErrorHandlerTest.php (3)
419-459: RAW format assertions are precise and valuable.Validating absence of HTML tags and presence of exception class/message ensures the ErrorHandler respects Response::FORMAT_RAW without leaking markup. LGTM.
571-592: Good use of handle() before resolve() to initialize components.Calling handle() before Request::resolve() follows the project’s lifecycle rules and avoids false negatives. Matches prior learnings.
481-502: Fallback HTML error response checks are solid.Verifying both status code and specific InvalidRouteException text (including entity-encoded quotes) will catch regressions in fallback rendering. LGTM.
…er` for exception rendering formats.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/provider/StatelessApplicationProvider.php (3)
8-14: Imports are correct; theuse const PHP_INT_SIZEis optional
yii\base\Exceptionandyii2\extensions\psrbridge\http\Responseimports are appropriate for the new data provider.- Importing the global
PHP_INT_SIZEviause constisn’t necessary and slightly increases noise. If the codebase doesn’t mandate this style, consider dropping it.Apply this diff to remove the unnecessary const import:
-use const PHP_INT_SIZE;
163-166: Fix message wording: “sanitized” instead of “sanitize”Minor grammar tweak in the assertion message.
- 'Response body should contain valid sanitize object cookie with its properties when validation is enabled.', + 'Response body should contain a valid, sanitized object cookie with its properties when validation is enabled.',
193-229: Solid provider for exception rendering; consider tightening types and header expectationsOverall, this provider reads well and aligns with the new ApplicationErrorHandler tests. A couple of small robustness improvements:
- The tuple’s last element is a numerically indexed list of substrings. Prefer
list<string>overarray<string>in the phpstan annotation.- For RAW responses, the expected “content type” is set to an empty string. Depending on Yii’s defaults, RAW may still emit a default
text/html; charset=UTF-8header via the base Response. If the test asserts equality, this could be fragile.Actions:
- Update the phpstan return type to use
list<string>.- Verify what header is actually produced for RAW in this bridge; if it’s not empty, either:
- make the provider’s second field
nullto signal “don’t assert content type” (and adjust the test to skip assertion whennull), or- change it to the concrete header that is emitted in RAW mode.
Suggested phpstan docblock tweak:
- /** - * @phpstan-return array<string, array{string, string, int, string, array<string>}> - */ + /** + * @phpstan-return array<string, array{string, string, int, string, list<string>}> + */If you decide to use the “nullable means skip” approach for content type, adjust RAW to
null:'RAW format with exception' => [ Response::FORMAT_RAW, - '', + null, 500, 'site/trigger-exception', [ Exception::class, 'Exception error message.', ], ],Also, please confirm:
- The route
site/trigger-exceptionthrowsyii\base\Exception(not\RuntimeExceptionetc.), otherwise the “class name” substring check will fail.- “Stack trace:” presence is debug-dependent. Ensure tests run with debug enabled when asserting that substring.
📜 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)
tests/http/stateless/ApplicationErrorHandlerTest.php(1 hunks)tests/provider/StatelessApplicationProvider.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/http/stateless/ApplicationErrorHandlerTest.php
🧰 Additional context used
🧠 Learnings (3)
📚 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:
tests/provider/StatelessApplicationProvider.php
📚 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 (line 408: `$this->response = new Response($this->components['response'] ?? []);`), 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:
tests/provider/StatelessApplicationProvider.php
📚 Learning: 2025-08-08T15:24:06.085Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/TestCase.php:23-27
Timestamp: 2025-08-08T15:24:06.085Z
Learning: In yii2-extensions/psr-bridge (tests/TestCase.php), maintainer preference: it’s acceptable to use random-looking strings for test-only constants like COOKIE_VALIDATION_KEY; no need to replace with an obviously non-secret value unless CI/secret scanners become problematic.
Applied to files:
tests/provider/StatelessApplicationProvider.php
🧬 Code graph analysis (1)
tests/provider/StatelessApplicationProvider.php (1)
src/http/Response.php (1)
Response(38-150)
⏰ 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). (5)
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (1)
tests/provider/StatelessApplicationProvider.php (1)
114-117: Good addition: richer object payload for cookie testsInitializing an object value and adding a property makes the “object cookie” case more realistic and exercises the sanitizer/unserialize path. Looks good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit