test(http): Refactor ApplicationErrorHandlerTest to use createRequest for improved clarity and remove redundant server variable assignments.
#152
Conversation
…messages and add test for `NotFoundHttpException` handling.
WalkthroughTests and one enum message were updated: routing tests' assertions were generalized; error-handler tests switch from SAPI globals to explicit PSR request construction and now assert JSON 404 payloads under strict parsing; one test gained a PHP-extension attribute; Message::PAGE_NOT_FOUND literal was changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Factory as FactoryHelper
participant App as StatelessApplication
participant ErrorHandler
participant Response
Test->>Factory: createRequest(method, path)
Factory-->>Test: Request
Test->>App: handle(Request)
App->>App: resolve route
alt route found
App-->>Response: normal Response
else route missing
App->>ErrorHandler: renderException / handle404
alt strict parsing enabled
ErrorHandler-->>Response: JSON 404 payload (application/json)
else
ErrorHandler-->>Response: HTML 404 or throw NotFoundHttpException
end
end
App-->>Test: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. ✨ 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 #152 +/- ##
===========================================
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: 0
🧹 Nitpick comments (4)
tests/http/stateless/ApplicationRoutingTest.php (4)
151-151: Normalize path format for consistency (leading slash).Elsewhere URIs include a leading slash; this test uses a bare path. Suggest aligning to avoid confusion when comparing paths across tests.
Apply this diff within the selected range to update the message:
- "Request path should be 'site/update/123'.", + "Request path should be '/site/update/123'.",Updates needed outside the selected range (for consistency with the message and other tests):
// Change request creation to include leading slash (Line 126) $request = FactoryHelper::createRequest(method: 'GET', uri: '/site/update/123'); // Adjust the expected path (Line 148) '/site/update/123',
176-180: Make Content-Type assertion resilient to charset/casing variations.Hard-equality on the full header is brittle. Prefer a regex or a prefix check to assert HTML content without coupling to exact parameter formatting.
Apply this diff:
- self::assertSame( - 'text/html; charset=UTF-8', - $response->getHeaderLine('Content-Type'), - "Expected Content-Type 'text/html; charset=UTF-8' for route '/nonexistent/route'.", - ); + self::assertMatchesRegularExpression( + '/^text\/html\b/i', + $response->getHeaderLine('Content-Type'), + "Expected Content-Type starting with 'text/html' for route '/nonexistent/route'.", + );
181-185: Avoid brittle full-body equality; assert meaningful substrings and keep single stream read.Exact HTML text can change (error handler, templates). Capture once and assert key markers.
Apply this diff:
- self::assertSame( - '<pre>Not Found: Page not found.</pre>', - $response->getBody()->getContents(), - "Response body should match expected HTML string '<pre>Not Found: Page not found.</pre>'.", - ); + $content = $response->getBody()->getContents(); + self::assertStringContainsString( + 'Page not found.', + $content, + 'Response body should contain the "Page not found." message.', + ); + self::assertStringContainsString( + '<pre>', + $content, + 'Response body should be HTML-wrapped.', + );
187-191: Future-proof the exception message assertion against localization.resolve() wraps the message in Yii::t(). If i18n is configured later, a strict string compare could become flaky. A regex around the Message constant is more tolerant.
Apply this diff:
- $this->expectException(NotFoundHttpException::class); - $this->expectExceptionMessage(Message::PAGE_NOT_FOUND->getMessage()); + $this->expectException(NotFoundHttpException::class); + $pattern = '/^' . preg_quote(Message::PAGE_NOT_FOUND->getMessage(), '/') . '$/'; + $this->expectExceptionMessageMatches($pattern);
📜 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/stateless/ApplicationRoutingTest.php(5 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 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/ApplicationRoutingTest.php
📚 Learning: 2025-08-24T11:52:50.563Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#141
File: tests/http/stateless/ApplicationRoutingTest.php:1-164
Timestamp: 2025-08-24T11:52:50.563Z
Learning: In yii2-extensions/psr-bridge, tests that manipulate PHP superglobals ($_POST, $_GET, $_SERVER) in the http group do not require process isolation and work fine with the current PHPUnit configuration.
Applied to files:
tests/http/stateless/ApplicationRoutingTest.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/ApplicationRoutingTest.php
📚 Learning: 2025-08-25T16:08:54.379Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#149
File: tests/http/stateless/ApplicationCoreTest.php:24-27
Timestamp: 2025-08-25T16:08:54.379Z
Learning: In the yii2-extensions/psr-bridge project, avoid importing functions that are not actually used in the code, even if they might seem logically related to the functionality being tested.
Applied to files:
tests/http/stateless/ApplicationRoutingTest.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/ApplicationRoutingTest.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/ApplicationRoutingTest.php
📚 Learning: 2025-08-10T20:39:09.333Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#82
File: tests/adapter/UploadedFilesPsr7Test.php:197-248
Timestamp: 2025-08-10T20:39:09.333Z
Learning: In the yii2-extensions/psr-bridge project, the TestCase base class configures the runtime path as `dirname(__DIR__) . '/runtime'` in the application configuration. Tests that need to save files to the runtime directory can rely on this pre-configured path.
Applied to files:
tests/http/stateless/ApplicationRoutingTest.php
📚 Learning: 2025-08-08T15:28:00.166Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/adapter/ServerRequestAdapterTest.php:2215-2215
Timestamp: 2025-08-08T15:28:00.166Z
Learning: In yii2-extensions/psr-bridge, tests extend tests/TestCase which defines a protected const COOKIE_VALIDATION_KEY. Test code should use self::COOKIE_VALIDATION_KEY instead of hardcoded cookieValidationKey literals.
Applied to files:
tests/http/stateless/ApplicationRoutingTest.php
📚 Learning: 2025-08-08T15:28:00.166Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#71
File: tests/adapter/ServerRequestAdapterTest.php:2215-2215
Timestamp: 2025-08-08T15:28:00.166Z
Learning: In yii2-extensions/psr-bridge tests, prefer using self::COOKIE_VALIDATION_KEY from tests/TestCase over hardcoded 'cookieValidationKey' strings to avoid secret scanners FP and improve maintainability.
Applied to files:
tests/http/stateless/ApplicationRoutingTest.php
🧬 Code graph analysis (1)
tests/http/stateless/ApplicationRoutingTest.php (4)
tests/TestCase.php (1)
statelessApplication(134-197)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createRequest(68-89)src/exception/Message.php (1)
getMessage(223-226)src/http/Request.php (1)
resolve(768-788)
⏰ 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). (2)
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (3)
tests/http/stateless/ApplicationRoutingTest.php (3)
9-10: Imports are accurate and necessary.Both NotFoundHttpException and Message are used below; good minimal surface.
51-52: Cleaner, route-agnostic assertion messages — good improvement.These updated messages better describe the expectation without overfitting to concrete routes.
Also applies to: 84-85, 117-118, 145-146
159-167: Correct approach to force 404 path by enabling strict URL parsing per-test.Overriding urlManager.enableStrictParsing locally is precise and avoids side effects on other tests via TestCase::statelessApplication merge.
terabytesoftw
changed the title
ApplicationRoutingTest with improved assertion messages and add test for NotFoundHttpException handling.ApplicationErrorHandlerTest to use createRequest for improved clarity and remove redundant server variable assignments.
…est` for improved clarity and remove redundant server variable assignments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/exception/Message.php (1)
130-133: Changing PAGE_NOT_FOUND text may have unintended BC ripple; consider adding a new enum case instead.Altering the string value of a public enum case can break consumers who assert on exact messages (your own tests now do) or who localize/ map by value. Given the PR is marked “Breaks BC? ❌”, please double‑check usages outside this repo.
Optionally, keep the original PAGE_NOT_FOUND value and introduce a new case specifically for StatelessApplication contexts. Example:
- /** - * Error when the page is not found. - * - * Format: "Page not found in StatelessApplication." - */ - case PAGE_NOT_FOUND = 'Page not found in StatelessApplication.'; + /** + * Error when the page is not found. + * + * Format: "Page not found." + */ + case PAGE_NOT_FOUND = 'Page not found.'; + + /** + * Error when the page is not found within StatelessApplication context. + * + * Format: "Page not found in StatelessApplication." + */ + case PAGE_NOT_FOUND_STATELESS = 'Page not found in StatelessApplication.';If you prefer to keep the change, please verify no external consumers rely on the previous literal.
I can follow up with a small patch to use PAGE_NOT_FOUND_STATELESS in the JSON renderer while leaving PAGE_NOT_FOUND unchanged. Do you want me to draft it?
tests/http/stateless/ApplicationErrorHandlerTest.php (4)
229-229: Nit: assertion message can be terser (optional).The trailing period inside the quoted message can be confusing when reading failures. Suggest using a consistent style like “…message ‘Exception error message’” (without final dot inside quotes).
- "Logger should contain an error log entry with category '{$expectedCategory}' and message 'Exception error message.'.", + "Logger should contain an error log entry with category '{$expectedCategory}' and message 'Exception error message'.",
273-273: Solid coverage: exercising HTML error rendering and warning capture.
- Using FactoryHelper::createRequest() is consistent with the PR goal.
- Asserting absence of “Undefined variable” warnings and presence of stack trace/class/file checks the template contract well.
Optional: for long-term resilience, consider asserting on key fragments (e.g., “Exception error message”) and presence of a stack trace section, but avoid depending on specific filenames if paths change in stubs.
Also applies to: 293-293, 301-301, 307-307, 312-312, 317-317
515-515: Same note as above: confirm HTML “Page not found.” remains by design.If later you switch HTML path to the enum, this test will need updating. For forward-compatibility, you could assert on “Not Found:” and not the exact trailing text, but that’s optional.
Also applies to: 531-531
537-538: Strict parsing + JSON assertions look good; consider reducing brittleness.
- Nice use of RequiresPhpExtension('runkit7') to guard constant redefinition. The YII_ENV_TEST toggling and buffer balancing are careful.
- Configuring errorHandler=null, response->format JSON, and enableStrictParsing=true cleanly isolates the scenario.
Optional hardening:
- Assert JSON structure by decoding and checking fields to get clearer diffs on failure. assertJsonStringEqualsJsonString is OK but can be opaque.
- Use the enum (Message::PAGE_NOT_FOUND) in the expected payload if you keep the new literal, so tests evolve with the constant.
Example (alternative approach):
- self::assertJsonStringEqualsJsonString( - <<<JSON - {"name":"Not Found","message":"Page not found in StatelessApplication.","code":0,"status":404,"type":"yii\\\\web\\\\NotFoundHttpException"} - JSON, - $response->getBody()->getContents(), - 'Response body should contain JSON with NotFoundHttpException details.', - ); + $json = Json::decode($response->getBody()->getContents()); + self::assertSame('Not Found', $json['name'] ?? null); + self::assertSame('Page not found in StatelessApplication.', $json['message'] ?? null); + self::assertSame(0, $json['code'] ?? null); + self::assertSame(404, $json['status'] ?? null); + self::assertSame('yii\web\NotFoundHttpException', $json['type'] ?? null);If desired, I can swap in Message::PAGE_NOT_FOUND->value for the message assertion.
Also applies to: 540-542, 547-552, 557-575, 577-579, 581-581
📜 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 (3)
src/exception/Message.php(1 hunks)tests/http/stateless/ApplicationErrorHandlerTest.php(14 hunks)tests/http/stateless/ApplicationRoutingTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/http/stateless/ApplicationRoutingTest.php
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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.
📚 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/ApplicationErrorHandlerTest.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, 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.php
📚 Learning: 2025-08-25T16:08:54.379Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#149
File: tests/http/stateless/ApplicationCoreTest.php:24-27
Timestamp: 2025-08-25T16:08:54.379Z
Learning: In the yii2-extensions/psr-bridge project, avoid importing functions that are not actually used in the code, even if they might seem logically related to the functionality being tested.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.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/ApplicationErrorHandlerTest.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.php
📚 Learning: 2025-08-10T20:39:09.333Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#82
File: tests/adapter/UploadedFilesPsr7Test.php:197-248
Timestamp: 2025-08-10T20:39:09.333Z
Learning: In the yii2-extensions/psr-bridge project, the TestCase base class configures the runtime path as `dirname(__DIR__) . '/runtime'` in the application configuration. Tests that need to save files to the runtime directory can rely on this pre-configured path.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.php
📚 Learning: 2025-08-24T11:52:50.563Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#141
File: tests/http/stateless/ApplicationRoutingTest.php:1-164
Timestamp: 2025-08-24T11:52:50.563Z
Learning: In yii2-extensions/psr-bridge, tests that manipulate PHP superglobals ($_POST, $_GET, $_SERVER) in the http group do not require process isolation and work fine with the current PHPUnit configuration.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.php
🧬 Code graph analysis (1)
tests/http/stateless/ApplicationErrorHandlerTest.php (3)
src/http/StatelessApplication.php (1)
handle(269-295)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createRequest(68-89)tests/TestCase.php (1)
statelessApplication(134-197)
⏰ 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 (6)
tests/http/stateless/ApplicationErrorHandlerTest.php (6)
57-57: Good refactor: explicit request construction via FactoryHelper.Switching to FactoryHelper::createRequest() removes reliance on ambient server state and makes intent clear.
187-187: LGTM: route handled with createRequest().This aligns the test with the new pattern and keeps setup local to the test.
351-351: Good: format-parametrized test uses explicit request creation.Keeps the matrix readable and avoids hidden dependencies on globals.
413-413: Good end-to-end test for custom errorAction returning a Response.
- errorHandler configured inline
- explicit request
- asserting normalized HTML content
All consistent with the refactor.
Also applies to: 419-419, 440-440
458-458: Refactor to createRequest is correct; assertion message clarified.This keeps the test focused on fallback behavior when errorAction is invalid.
Also applies to: 479-479
489-489: Confirm HTML error message is intentionally decoupled from Message::PAGE_NOT_FOUNDI wasn’t able to locate the
ApplicationErrorHandlerimplementation undersrc/http/statelessto verify how the HTML response message is sourced. Given that:
- tests/http/stateless/ApplicationErrorHandlerTest.php (lines 489 & 505) still assert the HTML error page shows
"<pre>Not Found: Page not found.</pre>"- the JSON handler (
src/http/Request.phpline 784) now throwsNotFoundHttpExceptionwith
Message::PAGE_NOT_FOUND→"Page not found in StatelessApplication."Please confirm:
- Where the HTML “Not Found: Page not found.” string is defined in the
ApplicationErrorHandler(or equivalent) class.- That this HTML path is intentionally hard‐coded (or otherwise decoupled) from the
Message::PAGE_NOT_FOUNDenum, so future changes to the enum won’t affect the HTML output.Until the location and decoupling are verified, I recommend reviewing these tests and the handler implementation to ensure the intended behavior remains stable.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/http/stateless/ApplicationErrorHandlerTest.php (2)
47-76: Wrap YII_DEBUG overrides in try/finally for isolation.If an assertion fails, YII_DEBUG may remain altered for subsequent tests. Protect the override with try/finally.
Apply this diff:
- @\runkit_constant_redefine('YII_DEBUG', $debug); - - $app = $this->statelessApplication( + @\runkit_constant_redefine('YII_DEBUG', $debug); + try { + $app = $this->statelessApplication( [ 'components' => [ 'errorHandler' => ['errorAction' => $action], ], ], - ); + ); - $response = $app->handle(FactoryHelper::createRequest('GET', $route)); + $response = $app->handle(FactoryHelper::createRequest('GET', $route)); - self::assertSame( + self::assertSame( $expectedStatusCode, $response->getStatusCode(), "Expected HTTP '{$expectedStatusCode}' for route '{$route}'.", - ); - self::assertSame( + ); + self::assertSame( 'text/html; charset=UTF-8', $response->getHeaderLine('Content-Type'), "Expected Content-Type 'text/html; charset=UTF-8' for route '{$route}'.", - ); - self::assertSame( + ); + self::assertSame( self::normalizeLineEndings($expectedErrorViewContent), self::normalizeLineEndings($response->getBody()->getContents()), $expectedAssertMessage, - ); - - @\runkit_constant_redefine('YII_DEBUG', true); + ); + } finally { + @\runkit_constant_redefine('YII_DEBUG', true); + }
408-443: Also guard YII_DEBUG here with try/finally.Same isolation concern applies to this test.
Apply this diff:
- @\runkit_constant_redefine('YII_DEBUG', false); - - $app = $this->statelessApplication( + @\runkit_constant_redefine('YII_DEBUG', false); + try { + $app = $this->statelessApplication( [ 'components' => [ 'errorHandler' => ['errorAction' => 'site/error-with-response'], ], ], - ); + ); - $response = $app->handle(FactoryHelper::createRequest('GET', '/site/trigger-exception')); + $response = $app->handle(FactoryHelper::createRequest('GET', '/site/trigger-exception')); - self::assertSame( + self::assertSame( 500, $response->getStatusCode(), "Expected HTTP '500' for route 'site/trigger-exception'.", - ); - self::assertSame( + ); + self::assertSame( 'text/html; charset=UTF-8', $response->getHeaderLine('Content-Type'), "Expected Content-Type 'text/html; charset=UTF-8' for route 'site/trigger-exception'.", - ); - self::assertSame( + ); + self::assertSame( self::normalizeLineEndings( <<<HTML <div id="custom-response-error"> Custom Response object from error action: Exception error message. </div> HTML, ), self::normalizeLineEndings($response->getBody()->getContents()), 'Response body should contain content from Response object.', - ); - - @\runkit_constant_redefine('YII_DEBUG', true); + ); + } finally { + @\runkit_constant_redefine('YII_DEBUG', true); + }
🧹 Nitpick comments (4)
tests/http/stateless/ApplicationErrorHandlerTest.php (4)
229-229: Nit: assertion message punctuation consistency.Keep the trailing period inside the quotes to match the actual message text and other assertions.
Apply this diff:
- "'Exception error message'.", + "'Exception error message.'.",
515-531: Same concern for the not-found HTML message.Mirror the regex approach here to prevent brittleness if the message source remains "in StatelessApplication.".
Apply this diff:
- self::assertStringContainsString( - '<pre>Not Found: Page not found.</pre>', - $response->getBody()->getContents(), - "Response body should contain the default not found message '<pre>Not Found: Page not found.</pre>'.", - ); + $body = $response->getBody()->getContents(); + self::assertMatchesRegularExpression( + '#<pre>Not Found: Page not found(?: in StatelessApplication\.)?</pre>#', + $body, + 'Response body should contain the default not-found message.', + );
104-104: Optional: use FactoryHelper::createRequest here for consistency.Even though this test purposely manipulates $_SERVER to validate filtering (which is fine and base TestCase cleans up superglobals per prior learnings), routing can still use an explicit request for clarity.
Apply this diff:
- $response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); + $response = $app->handle(FactoryHelper::createRequest('GET', '/site/nonexistent-action'));Note: keeping the $_SERVER setup is still necessary since the fallback view prints actual $_SERVER contents; this change just removes the reliance on createFromGlobals for the request itself.
489-505: Verify not-found message consistency and harden HTML assertionsThe
PAGE_NOT_FOUNDconstant insrc/exception/Message.php:132readscase PAGE_NOT_FOUND = 'Page not found in StatelessApplication.';while the HTML tests in
tests/http/stateless/ApplicationErrorHandlerTest.phpstill assert the shorter<pre>Not Found: Page not found.</pre>. If the handler starts using the global constant for HTML output, these tests will break.Locations to update:
- tests/http/stateless/ApplicationErrorHandlerTest.php, around line 502
- tests/http/stateless/ApplicationErrorHandlerTest.php, around line 528
Two ways to address this:
- Align the HTML assertions with the constant:
self::assertStringContainsString( '<pre>Not Found: Page not found in StatelessApplication.</pre>', $response->getBody()->getContents(), "Response body should contain the full not-found message.", );- Or make the assertions tolerant of both variants via a regex:
- self::assertStringContainsString( - '<pre>Not Found: Page not found.</pre>', - $response->getBody()->getContents(), - "Response body should contain error message about 'Not Found: Page not found'.", - ); + $body = $response->getBody()->getContents(); + self::assertMatchesRegularExpression( + '#<pre>Not Found: Page not found(?: in StatelessApplication\.)?</pre>#', + $body, + "Response body should contain the not-found message.", + );
📜 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/stateless/ApplicationErrorHandlerTest.php(14 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
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.
📚 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/ApplicationErrorHandlerTest.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, 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.php
📚 Learning: 2025-08-25T16:08:54.379Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#149
File: tests/http/stateless/ApplicationCoreTest.php:24-27
Timestamp: 2025-08-25T16:08:54.379Z
Learning: In the yii2-extensions/psr-bridge project, avoid importing functions that are not actually used in the code, even if they might seem logically related to the functionality being tested.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.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/ApplicationErrorHandlerTest.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.php
📚 Learning: 2025-08-10T20:39:09.333Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#82
File: tests/adapter/UploadedFilesPsr7Test.php:197-248
Timestamp: 2025-08-10T20:39:09.333Z
Learning: In the yii2-extensions/psr-bridge project, the TestCase base class configures the runtime path as `dirname(__DIR__) . '/runtime'` in the application configuration. Tests that need to save files to the runtime directory can rely on this pre-configured path.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.php
📚 Learning: 2025-08-24T11:52:50.563Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#141
File: tests/http/stateless/ApplicationRoutingTest.php:1-164
Timestamp: 2025-08-24T11:52:50.563Z
Learning: In yii2-extensions/psr-bridge, tests that manipulate PHP superglobals ($_POST, $_GET, $_SERVER) in the http group do not require process isolation and work fine with the current PHPUnit configuration.
Applied to files:
tests/http/stateless/ApplicationErrorHandlerTest.php
🧬 Code graph analysis (1)
tests/http/stateless/ApplicationErrorHandlerTest.php (4)
src/http/StatelessApplication.php (1)
handle(269-295)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createRequest(68-89)tests/TestCase.php (1)
statelessApplication(134-197)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). (4)
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (10)
tests/http/stateless/ApplicationErrorHandlerTest.php (10)
57-57: Good switch to explicit PSR request creation.Using FactoryHelper::createRequest here aligns with the PR goal of avoiding globals and improves test clarity.
187-187: Consistent request construction improves readability.Nice replacement with FactoryHelper::createRequest for the trigger-exception route.
273-273: LGTM: clear and explicit request.The GET request via FactoryHelper keeps test intent obvious.
286-317: Assertions read well and avoid double-stream reads.Capturing the body once and reusing it across assertions is correct and prevents stream pointer pitfalls.
351-351: Good: standardized request creation here as well.
413-414: Explicit error action route clarifies behavior.Configuring 'site/error-with-response' in the errorHandler component makes the scenario unambiguous.
418-418: Consistent FactoryHelper usage.
440-440: Clearer assertion message.Message now precisely states the expectation about content coming from the Response object.
458-458: Good replacement with createRequest.
479-479: Assertion message tightened.This improves signal without losing meaning.
Summary by CodeRabbit