-
-
Notifications
You must be signed in to change notification settings - Fork 1
test(http): Move JSON response test from ApplicationTest to ApplicationCoreTest for better organization.
#146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ationCoreTest` for better organization.
WalkthroughAdds JSON body assertions to multiple stateless HTTP core tests and removes a duplicate JSON response test from another test class. No production code or public API 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 #146 +/- ##
===========================================
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/http/stateless/ApplicationCoreTest.php (4)
153-159: Same stream-safety nit: use (string)$response1->getBody() instead of getContents()Aligns with the previous suggestion for consistent, side-effect-free assertions.
self::assertJsonStringEqualsJsonString( <<<JSON {"hello":"world"} JSON, - $response1->getBody()->getContents(), + (string) $response1->getBody(), "Expected JSON Response body '{\"hello\":\"world\"}'.", );
357-363: Same improvement for body assertionReplace getContents() to prevent cursor-related flakes.
self::assertJsonStringEqualsJsonString( <<<JSON {"hello":"world"} JSON, - $response->getBody()->getContents(), + (string) $response->getBody(), "Expected JSON Response body '{\"hello\":\"world\"}'.", );
398-404: Same improvement for body assertionUse string cast for reliability and consistency with other tests.
self::assertJsonStringEqualsJsonString( <<<JSON {"hello":"world"} JSON, - $response->getBody()->getContents(), + (string) $response->getBody(), "Expected JSON Response body '{\"hello\":\"world\"}'.", );
451-457: Same improvement for body assertionAvoid getContents() to prevent stream position surprises in future edits.
self::assertJsonStringEqualsJsonString( <<<JSON {"hello":"world"} JSON, - $response->getBody()->getContents(), + (string) $response->getBody(), "Expected JSON Response body '{\"hello\":\"world\"}'.", );
🧹 Nitpick comments (1)
tests/http/stateless/ApplicationCoreTest.php (1)
118-124: Prefer casting the PSR-7 stream to string over getContents() to avoid pointer-side effectsgetContents() reads from the current cursor; if anyone reads the body earlier (or later) the assertion can become flaky. Casting to string is safer for tests.
Apply this diff:
self::assertJsonStringEqualsJsonString( <<<JSON {"hello":"world"} JSON, - $response->getBody()->getContents(), + (string) $response->getBody(), "Expected JSON Response body '{\"hello\":\"world\"}'.", );Optional: to DRY repeated JSON body checks across tests, consider a small helper.
private static function assertJsonBody(string $expectedJson, \Psr\Http\Message\ResponseInterface $response): void { self::assertJsonStringEqualsJsonString($expectedJson, (string) $response->getBody()); }
📜 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/ApplicationCoreTest.php(5 hunks)tests/http/stateless/ApplicationTest.php(0 hunks)
💤 Files with no reviewable changes (1)
- tests/http/stateless/ApplicationTest.php
Summary by CodeRabbit