refactor: Simplify request handling in StatelessApplicationTest class by removing redundant variable assignments.
#112
Conversation
…ss by removing redundant variable assignments.
WalkthroughRefactors tests in tests/http/StatelessApplicationTest.php to inline ServerRequest creation into app->handle() calls, remove intermediate request variables, consolidate response variables, and update assertions and cleanup. No production/public API or behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 #112 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 320 320
===========================================
Files 12 12
Lines 810 810
===========================================
Hits 810 810 ☔ 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 (5)
tests/http/StatelessApplicationTest.php (5)
327-327: Minor perf/readability nit: reuse a single ServerRequestCreator in the loopInside the 25-iteration loop, you recreate the ServerRequestCreator each time. Reusing one cuts object churn and clarifies intent without changing behavior.
Apply within this hunk:
- $response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); + $response = $app->handle($creator->createFromGlobals());Add just above the loop (outside this hunk):
$creator = FactoryHelper::createServerRequestCreator();Also applies to: 336-336
750-751: Same optional reuse in worker-mode multi-request testSame suggestion as above: hoist a single $creator and use $creator->createFromGlobals() inside both loops. Keeps the test succinct and a touch faster.
Also applies to: 782-783
570-576: Fix assertion message route name (“captcha” → “setflash”)The message references site/captcha but this test hits site/setflash. Update for clarity.
- "Response 'Content-Type' should be 'application/json; charset=UTF-8' for 'site/captcha' route in " . + "Response 'Content-Type' should be 'application/json; charset=UTF-8' for 'site/setflash' route in " .I can scan the file for similar message/route mismatches if you want a quick follow-up patch.
1768-1773: Intentional retention of explicit $request — makes sense hereKeeping a concrete $request to later assert $request->getUri()->getPath() adds value in this test; no need to inline here. If you prefer full consistency, you could assert against $app->request->getPsr7Request(), but this is fine as-is.
2465-2466: General DRY suggestion: tiny helper for “handle from globals”Optional: a small helper reduces repetition and focuses tests on assertions.
Add to the test class:
private function handleFromGlobals($app): \Psr\Http\Message\ResponseInterface { return $app->handle(\yii2\extensions\psrbridge\tests\support\FactoryHelper::createServerRequestCreator()->createFromGlobals()); }Then replace occurrences of:
$app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals());with:
$this->handleFromGlobals($app);Also applies to: 2499-2500
📜 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/StatelessApplicationTest.php(62 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/StatelessApplicationTest.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/StatelessApplicationTest.php
🧬 Code Graph Analysis (1)
tests/http/StatelessApplicationTest.php (1)
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). (4)
- GitHub Check: phpunit / PHP 8.4-windows-latest
- 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 (5)
tests/http/StatelessApplicationTest.php (5)
79-80: Inline ServerRequest creation in handle() — LGTMPassing FactoryHelper::createServerRequestCreator()->createFromGlobals() directly into $app->handle() keeps semantics intact while reducing noise. Works cleanly across multiple requests within the same test.
Also applies to: 132-133, 201-203
243-245: Good adherence to component initialization before assertionsCalling handle() to initialize app components before inspecting memory, resolving routes, or invoking Request::resolve() aligns with our prior learning about lifecycle requirements and prevents false negatives.
Also applies to: 278-279, 650-651, 3059-3060
384-386: clearOutput() precondition satisfiedNice: handle() is invoked before exercising errorHandler->clearOutput(), ensuring the handler exists and buffers are owned by the app.
424-426: Container bootstrapping via handle()Good call invoking handle() prior to container assertions. This ensures definitions are registered and usable.
2927-2928: Stateful assertions depend on inlining — correct orderingInvoking handle() before reading PSR-7 request headers (statelessAppStartTime) and before checking Yii aliases keeps the tests deterministic.
Also applies to: 2956-2957
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/http/StatelessApplicationTest.php (2)
1047-1072: Harden runkit constant redefinitions with try/finally to avoid leak-on-failure.If an assertion throws, YII_DEBUG may remain redefined and affect later tests. Wrap the body in try/finally to always restore.
Apply this pattern (example for testRenderExceptionWithErrorActionReturningResponseObject):
- @\runkit_constant_redefine('YII_DEBUG', false); + @\runkit_constant_redefine('YII_DEBUG', false); $_SERVER = [ 'REQUEST_METHOD' => 'GET', 'REQUEST_URI' => 'site/trigger-exception', ]; - $app = $this->statelessApplication( + try { + $app = $this->statelessApplication( [ 'components' => [ 'errorHandler' => [ 'errorAction' => 'site/error-with-response', ], ], ], ); - - $response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); + $response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); - self::assertSame( + self::assertSame( 500, $response->getStatusCode(), "Response 'status code' should be '500' when 'errorAction' returns Response object.", ); ... - @\runkit_constant_redefine('YII_DEBUG', true); + } finally { + @\runkit_constant_redefine('YII_DEBUG', true); + }Please replicate the same structure in the other tests that redefine YII_DEBUG.
Also applies to: 3112-3146, 3171-3205
2770-2787: Ensure filtered session cookie headers are reindexed and assertion message is updatedThe filtered session‐cookie array must be reindexed before using
[0]—and the assertion’s interpolated message must reference the new variable name to avoid an undefined-variable error.• Location: tests/http/StatelessApplicationTest.php lines 2770–2787
• Issue:array_filterpreserves original keys, so$cookie[0]can be undefined—even when the count is 1—and the error message still refers to$cookie.
• Fix: wrap the filter inarray_values(), rename the variable (e.g.$sessionCookies), update both the assertion and the embedded message.Apply this diff:
--- a/tests/http/StatelessApplicationTest.php +++ b/tests/http/StatelessApplicationTest.php @@ -2769,18 +2769,22 @@ class StatelessApplicationTest extends TestCase $response = $app->handle(FactoryHelper::createServerRequestCreator()->createFromGlobals()); $sessionName = $app->session->getName(); - $cookie = array_filter( + $sessionCookies = array_values(array_filter( $response->getHeader('Set-Cookie'), static fn(string $cookie): bool => str_starts_with($cookie, "{$sessionName}="), - ); + )); - self::assertCount( - 1, - $cookie, + self::assertCount( + 1, + $sessionCookies, "Response 'Set-Cookie' header should contain exactly one '{$sessionName}' cookie when no session cookie " . "is sent in 'StatelessApplication'.", - ); - self::assertMatchesRegularExpression( + ); + self::assertMatchesRegularExpression( '/^' . preg_quote($sessionName, '/') . '=[a-zA-Z0-9]+; Path=\/; HttpOnly; SameSite$/', - $cookie[0] ?? '', - "Response 'Set-Cookie' header should match the expected format for a new session 'ID' when no session " . - "cookie is sent in 'StatelessApplication'. Value received: '" . ($cookie[0] ?? '') . "'.", + $sessionCookies[0] ?? '', + "Response 'Set-Cookie' header should match the expected format for a new session 'ID' when no session " . + "cookie is sent in 'StatelessApplication'. Value received: '" . ($sessionCookies[0] ?? '') . "'.", );
🧹 Nitpick comments (1)
tests/http/StatelessApplicationTest.php (1)
2465-2465: Optional: consider a tiny helper to DRY request creation from globals.Not required for this PR, but a private helper like globalsRequest() in the base TestCase could shorten many calls without reintroducing intermediates.
Example:
private function globalsRequest(): \Psr\Http\Message\ServerRequestInterface { return \yii2\extensions\psrbridge\tests\support\FactoryHelper::createServerRequestCreator()->createFromGlobals(); }Then: $response = $app->handle($this->globalsRequest());
Also applies to: 2498-2498, 2984-2984, 3017-3017
📜 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/StatelessApplicationTest.php(62 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/StatelessApplicationTest.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/StatelessApplicationTest.php
🧬 Code Graph Analysis (1)
tests/http/StatelessApplicationTest.php (2)
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). (6)
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit / PHP 8.2-ubuntu-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: mutation / PHP 8.3-ubuntu-latest
🔇 Additional comments (29)
tests/http/StatelessApplicationTest.php (29)
79-79: Inlining ServerRequest creation directly into handle() is clean and preserves semantics.Using FactoryHelper::createServerRequestCreator()->createFromGlobals() inline avoids redundant locals without changing behavior. Pattern looks correct here for both users and the image fetch step.
Also applies to: 132-132, 201-201
243-243: Good call initializing the app via handle() before memory-limit assertions.Calling handle() first ensures components are initialized before invoking clean()/getMemoryLimit(), matching the test intent.
Also applies to: 279-279
327-337: Loop-scoped response usage and explicit unset keep the GC test deterministic.Creating $response per-iteration and unsetting it with other strong refs helps exercise gc_collect_cycles() predictably.
384-385: Correct lifecycle: invoke handle() before clearOutput().Ensures ErrorHandler is ready and has local buffers to clear; assertion logic remains intact.
424-425: Container resolution test stays accurate with inline handle().Triggering handle() once to bootstrap the container is sufficient; the subsequent container->has/->get assertions are valid.
564-588: Flash message tests read cleaner with single $response and inlined request.Assertions still target the same PSR-7 response while reducing noise; header and body checks remain precise.
Also applies to: 599-606, 612-612, 618-618
650-652: Unlimited memory path is still covered without extra locals.handle() before clean() validates the code path without side effects; good.
688-690: Logging test remains faithful; initialization via handle() + in-memory logger is correct.This keeps log messages available for assertions since flushLogger is false.
750-751: Worker-mode session isolation uses the simplified pattern correctly.The inline requests within the loop are fine and keep per-iteration state minimal.
Also applies to: 782-783
1288-1289: Cookie headers test is unchanged in behavior and reads simpler.Inline handle() is appropriate here.
1356-1357: Core components check after a single inline handle() is spot on.No functional changes; bootstrap remains explicit.
1534-1556: Fallback error path test remains correct with inlined request.The 500 + HTML content assertions are unaffected; simplification looks good.
1576-1589: Cookie collection JSON assertion unchanged; inline request keeps it concise.No issues spotted.
1606-1619: Basic auth happy path: good simplification, same checks.Reads cleaner; semantics preserved.
1636-1651: Malformed Authorization header case-insensitivity preserved.Inline request is fine; assertions unchanged.
1672-1686: POST body echo test is unchanged; inline handle() trims boilerplate.All good.
1707-1721: GET query echo test is similarly improved.No behavior change.
1737-1755: Route+query composition test remains explicit with inline call.Assertions unchanged; OK.
1919-1921: Invalid Basic prefix test simplified.Correctly retains case-insensitive expectations on ‘Basic ’ scheme.
2049-2070: Partial credentials via PHP_AUTH_USER: simplification is safe.Response/headers/body checks remain exact.
2085-2099: Unlimited memory re-check post-handle is clear and preserved.No concerns.
2114-2137: File/stream/redirect tests: inlining reduces noise without altering effects.All header/body assertions remain correct; good coverage.
Also applies to: 2152-2169, 2184-2196, 2211-2223
2465-2483: Sanity smoke tests read better with less setup.Return JSON and status 201 checks are intact.
Also applies to: 2498-2505
2523-2545: Cookie validation tests keep the same semantics with explicit FactoryHelper::createRequest() where needed.Nice balance between createFromGlobals() and createRequest() usage.
Also applies to: 2565-2584
2604-2621: Session persistence test: simplified request handling, assertions unchanged.Multiple steps stay readable with one $response variable; no behavior drift.
Also applies to: 2632-2650, 2652-2671
2690-2707: Session isolation test: same robustness with leaner setup.LGTM.
Also applies to: 2716-2734, 2736-2755
3059-3065: Correct order: handle() before request->resolve() (matches prior learning).This preserves the requirement that components are initialized before invoking Request::resolve(). Good alignment with our earlier repository learning.
If you see intermittent failures around resolve() in other tests, ensure handle() is called first as done here.
2927-2947: Nice touch validating statelessAppStartTime header via MockerFunctions.The inline handle() keeps the setup short while asserting adapter header propagation precisely.
3085-3086: Event triggering test stays lean and accurate with inline handle().Less boilerplate; same coverage.
Summary by CodeRabbit