test(http): Move testRenderExceptionSetsDisplayErrorsInDebugMode from ApplicationTest to ApplicationErrorHandlerTest for better organization.
#143
Conversation
…om `ApplicationTest` to `ApplicationErrorHandlerTest` for better organization.
|
Warning Rate limit exceeded@terabytesoftw has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughMoves a RunKit7-based test for renderException/display_errors from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 #143 +/- ##
=============================================
- Coverage 100.00% 99.87% -0.13%
Complexity 318 318
=============================================
Files 12 12
Lines 808 809 +1
=============================================
Hits 808 808
- Misses 0 1 +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 (3)
tests/http/stateless/ApplicationErrorHandlerTest.php (3)
390-394: Make the display_errors assertion resilient across SAPIs.ini_get('display_errors') may return "1"/"0" or "On"/"Off" depending on environment. Asserting strictly against '1' can be brittle on some setups. Consider normalizing to a boolean.
Apply this diff within the assertion block:
- self::assertSame( - '1', - ini_get('display_errors'), - "'display_errors' should be set to '1' when YII_DEBUG mode is enabled and rendering exception view.", - ); + $displayErrors = (string) ini_get('display_errors'); + $isEnabled = filter_var($displayErrors, FILTER_VALIDATE_BOOLEAN); + self::assertTrue( + $isEnabled, + "'display_errors' should be enabled when YII_DEBUG mode is enabled and rendering exception view. Actual: '{$displayErrors}'", + );
355-356: Optional: make the test self-contained by explicitly forcing YII_DEBUG=true.This test currently relies on the suite’s bootstrap defining YII_DEBUG=true. To avoid environment coupling and potential flakiness, explicitly set YII_DEBUG to true and restore it in finally.
Apply this diff to the method:
@\runkit_constant_redefine('YII_ENV_TEST', false); + $originalYiiDebug = YII_DEBUG; + @\runkit_constant_redefine('YII_DEBUG', true); @@ - @\runkit_constant_redefine('YII_ENV_TEST', true); + @\runkit_constant_redefine('YII_ENV_TEST', true); + @\runkit_constant_redefine('YII_DEBUG', $originalYiiDebug);Also applies to: 407-408
364-365: Minor: import ini_get/ini_set for consistency with other function imports.You already import ob_get_level/ob_start at the top via use function. For consistency (and a tiny perf win on namespaced calls), import ini_get and ini_set similarly.
Add near the other function imports at the top of this file:
use function ini_get; use function ini_set;
📜 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 (10)
📚 Learning: 2025-08-24T11:52:50.524Z
Learnt from: terabytesoftw
PR: yii2-extensions/psr-bridge#141
File: tests/http/stateless/ApplicationRoutingTest.php:1-164
Timestamp: 2025-08-24T11:52:50.524Z
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/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/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/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-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/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
📚 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/ApplicationTest.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/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, 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
🧬 Code graph analysis (1)
tests/http/stateless/ApplicationErrorHandlerTest.php (3)
tests/TestCase.php (1)
statelessApplication(134-197)src/http/StatelessApplication.php (1)
handle(269-295)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createServerRequestCreator(154-161)
⏰ 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 (2)
tests/http/stateless/ApplicationTest.php (1)
7-7: Import cleanup aligns with test relocation.Trimming the attribute imports down to Group after moving the runkit-dependent test out of this class is correct. No unused attributes remain here; #[Group('http')] is still used below.
tests/http/stateless/ApplicationErrorHandlerTest.php (1)
349-409: Relocated test is well-scoped and restores global state.Good move placing this display_errors behavior under ApplicationErrorHandlerTest. The test properly:
- toggles YII_ENV_TEST,
- preserves/ restores output buffer depth,
- restores ini setting for display_errors,
- asserts status, content-type, and body contents.
This improves cohesion without altering production code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/http/stateless/ApplicationErrorHandlerTest.php (5)
355-356: Make the test order-independent by explicitly forcing debug mode and restoring it.Currently the test assumes YII_DEBUG is true. If run after a test that sets YII_DEBUG=false and forgets to restore it, this test could falsely fail. Force YII_DEBUG=true at the start and restore the original value in finally.
@@ - @\runkit_constant_redefine('YII_ENV_TEST', false); + @\runkit_constant_redefine('YII_ENV_TEST', false); + $originalYiiDebug = \defined('YII_DEBUG') ? \constant('YII_DEBUG') : null; + @\runkit_constant_redefine('YII_DEBUG', true); @@ - ini_set('display_errors', $originalDisplayErrors); - - @\runkit_constant_redefine('YII_ENV_TEST', true); + ini_set('display_errors', $originalDisplayErrors); + if ($originalYiiDebug !== null) { + @\runkit_constant_redefine('YII_DEBUG', $originalYiiDebug); + } + @\runkit_constant_redefine('YII_ENV_TEST', true);Also applies to: 411-414
393-397: Harden the assertion on display_errors to be robust across PHP configurations.Comparing to the string '1' can be brittle; some environments may yield "On"/"off". Use FILTER_VALIDATE_BOOLEAN for a resilient truthy check.
- self::assertSame( - '1', - ini_get('display_errors'), - "'display_errors' should be set to '1' in debug mode when rendering exception.", - ); + self::assertTrue( + filter_var(ini_get('display_errors'), FILTER_VALIDATE_BOOLEAN), + "'display_errors' should be enabled (truthy) in debug mode when rendering exception.", + );
401-405: Fix typo and clarify assertion message for output buffers.There is an extra apostrophe after clearOutput(). Also, the message can be clearer.
- self::assertLessThanOrEqual( + self::assertLessThanOrEqual( $initialBufferLevel, $buffersAfterTest, - "'clearOutput()'' should properly clean output buffers", + "'clearOutput()' should clean pre-existing output buffers created before handling the request.", );
407-409: Symmetrically restore output buffering level to its initial value.Only increasing the buffer level back (via ob_start) handles the “less than” case. Also handle the “greater than” case to avoid leaking extra buffers if the assertion ever gets relaxed or in future refactors.
- while (ob_get_level() < $initialBufferLevel) { - ob_start(); - } + while (ob_get_level() > $initialBufferLevel) { + ob_end_clean(); + } + while (ob_get_level() < $initialBufferLevel) { + ob_start(); + }If you prefer consistent style with the existing imports, you may also add the following near the other function imports at the top of the file:
use function ob_end_clean;
381-391: Strengthen the test by asserting that pre-buffered content is not leaked into the response body.Since discardExistingOutput=true, it’s useful to assert that previously buffered strings don’t appear in the response.
$response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); 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'.", ); + $body = $response->getBody()->getContents(); + self::assertStringNotContainsString( + 'buffer content that should be cleared', + $body, + 'Response body must not contain content that was present in a pre-existing output buffer.', + ); + self::assertStringNotContainsString( + 'nested buffer content', + $body, + 'Response body must not contain content from nested pre-existing output buffers.', + );
📜 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(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/http/stateless/ApplicationErrorHandlerTest.php (3)
tests/TestCase.php (1)
statelessApplication(134-197)src/http/StatelessApplication.php (1)
handle(269-295)tests/support/FactoryHelper.php (2)
FactoryHelper(46-268)createServerRequestCreator(154-161)
⏰ 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 (1)
tests/http/stateless/ApplicationErrorHandlerTest.php (1)
349-416: Verification complete: no duplicate test definitions foundThe grep search for
testRenderExceptionSetsDisplayErrorsInDebugModereturned a single match intests/http/stateless/ApplicationErrorHandlerTest.php(lines 351–355), confirming the method was moved and not left behind or duplicated elsewhere.
…ions in `testRenderExceptionPassesExceptionParameterToTemplateView`.
…` output cleaning.
Summary by CodeRabbit