Chore: Double slash tests #176
Conversation
WalkthroughAdds a new end-to-end test method testDoubleSlash() in tests/e2e/BaseTest.php to validate URL double-slash path normalization by issuing five GET requests and asserting status codes and response bodies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant TestSuite as BaseTest::testDoubleSlash()
participant Server as HTTP Server
Tester->>TestSuite: Run testDoubleSlash()
loop Double-slash paths
Note over TestSuite: Paths: //, //path-404,<br/>//value/123, /value//123, //value//123
TestSuite->>Server: GET {path}
Server-->>TestSuite: 200 OK + Body
TestSuite-->>Tester: Assert status/body per case
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 (
|
![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/e2e/BaseTest.php (4)
97-97: Make “empty body” assertions resilient to incidental whitespaceIn a few environments/middlewares, bodies may include a trailing newline. Trimming before asserting emptiness will reduce flakiness without weakening the check.
Apply this diff:
- $this->assertEmpty($response['body']); + $this->assertEmpty(trim($response['body']));Repeat the same replacement for all three empty-body assertions in this method.
Also applies to: 101-101, 105-105
86-106: Reduce duplication with a table-driven loopYou can make this test more compact and maintainable by iterating over cases. It also lets you attach per-path failure messages easily.
Apply this diff to the method body:
- { - $response = $this->client->call(Client::METHOD_GET, '//'); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals('Hello World!', $response['body']); - - $response = $this->client->call(Client::METHOD_GET, '//path-404'); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals('Hello World!', $response['body']); - - $response = $this->client->call(Client::METHOD_GET, '//value/123'); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEmpty($response['body']); - - $response = $this->client->call(Client::METHOD_GET, '/value//123'); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEmpty($response['body']); - - $response = $this->client->call(Client::METHOD_GET, '//value//123'); - $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEmpty($response['body']); - } + { + $cases = [ + ['//', 200, 'Hello World!'], + ['//path-404', 200, 'Hello World!'], + ['//value/123', 200, ''], + ['/value//123', 200, ''], + ['//value//123', 200, ''], + ]; + + foreach ($cases as [$path, $expectedStatus, $expectedBody]) { + $response = $this->client->call(Client::METHOD_GET, $path); + $this->assertEquals($expectedStatus, $response['headers']['status-code'], "Unexpected status for path {$path}"); + if ($expectedBody === '') { + $this->assertEmpty(trim($response['body']), "Expected empty body for path {$path}, got: {$response['body']}"); + } else { + $this->assertSame($expectedBody, $response['body'], "Unexpected body for path {$path}"); + } + } + }
91-94: Confirm expected 200 for path “//path-404”This intentionally diverges from testNotFound(), which asserts 404 for unknown paths. Given Client::call() enables CURLOPT_FOLLOWLOCATION, a 200 here could be the result of a redirect to “/” rather than router-level normalization. If the goal is specifically to validate normalization vs. redirect behavior, we may want to capture/inspect redirect info in the client in a follow-up (e.g., an option to disable following redirects or returning redirect count/effective URL).
Please confirm that returning 200 with “Hello World!” for “//path-404” is the desired behavior across supported runtimes/web servers.
If you want, I can draft a follow-up change adding an optional “followRedirects” flag (default true) to Client::call() so tests can assert redirect semantics explicitly.
85-85: Name nit: clarify intentOptional: rename to testDoubleSlashNormalization() to be more explicit about what’s being validated.
- public function testDoubleSlash() + public function testDoubleSlashNormalization()
📜 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/e2e/BaseTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/e2e/BaseTest.php (1)
tests/e2e/Client.php (2)
call(55-107)Client(7-108)
🔇 Additional comments (1)
tests/e2e/BaseTest.php (1)
85-106: LGTM: Solid addition of double-slash E2E coverageNice, focused test expanding coverage for path normalization edge cases. Matches the PR objective and keeps assertions clear.
|
Tests failing and master isnt used anyway right now, skipping for now |
Summary by CodeRabbit