Skip to content

Conversation

@Meldiron
Copy link
Contributor

@Meldiron Meldiron commented Nov 26, 2025

Summary by CodeRabbit

  • Tests
    • Tightened many test assertions from loose to strict equality, ensuring value and type match across unit and end-to-end suites.
    • Minor test refactor to improve iteration clarity in a parameterized test.
    • No runtime behavior or public API changes; this increases test precision and robustness.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 26, 2025

Walkthrough

This PR systematically replaces assertEquals() assertions with assertSame() across eight test files: HookTest.php, HttpTest.php, RequestTest.php, ResponseTest.php, RouteTest.php, RouterTest.php, BaseTest.php, and ResponseFPMTest.php. The change enforces strict type and value equality comparisons instead of loose equality in all affected test assertions. No production code, logic, control flow, or public API signatures are modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Rationale: Highly homogeneous, repetitive changes applied consistently across multiple test files following an identical pattern (assertEqualsassertSame). No logic density, control-flow changes, or new complexity introduced. The primary verification task is confirming the pattern was applied uniformly and that strict equality checks align with intended test expectations.

Areas requiring attention:

  • Verify that expected values match types (e.g., strings vs integers) so tests still pass under strict comparison.
  • Spot-check a representative test or two in each file to confirm the assertion swap preserves correctness.
  • Confirm there are no remaining assertEquals usages that should have been updated.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.20% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately and directly describes the main change: systematically replacing assertEquals with assertSame across all test files for strict equality checks.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore-assert-same

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e9c018 and d83d86a.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • tests/HookTest.php (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/HookTest.php (3)
src/Http/Route.php (1)
  • hook (111-116)
src/Http/Request.php (1)
  • getParams (72-75)
tests/MockRequest.php (1)
  • getParams (46-51)
🪛 PHPMD (2.15.0)
tests/HookTest.php

113-113: Avoid unused local variables such as '$param'. (undefined)

(UnusedLocalVariable)

🔇 Additional comments (4)
tests/HookTest.php (4)

23-39: Strict initial-state assertions look good

Using assertSame('', ...) and assertSame([], ...) for getDesc() and getGroups() tightens the contracts and matches the PR goal of strict comparisons. No issues spotted.


41-46: Strict comparison on action result is appropriate

assertSame('hello world', $this->hook->getAction()()) is a precise check on both type and value of the callback result; this is a solid upgrade from loose equality.


48-57: Param list initialization assertion aligns with API

assertSame([], $this->hook->getParams()) clearly enforces that an empty parameter list is represented as an empty array, which is consistent with other getParams() usages in the codebase.


59-93: Strict result check for injected resources

Asserting 'user:00:00:00' with assertSame after DI resolution is correct and keeps this integration test precise without any type juggling.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
tests/RequestTest.php (1)

22-35: AssertSame usage is type-safe; consider normalizing expected/actual order

All of the new assertSame calls here look semantically correct:

  • Headers, query params, payload, server values, cookies, protocol/method/URI/query string, port/hostname, and the various HTTP_*-backed accessors are compared against string literals or provided defaults, matching the underlying return types.
  • Content-range and range parsing tests correctly assert either specific ints/strings ('bytes', 0, 499, 2000) or null on invalid inputs, which works well with strict comparison and reinforces that these methods do not fall back to falsy values like false or 0.
  • There are no obvious cases where a numeric string is being compared to an int, or vice versa; where ports/status codes are involved, the expectations are typed consistently.

One small style point: in a few places (for example lines around testCanGetQueryParameter, testCanSetQuery, testCanSetPayload, etc.), you use:

$this->assertSame($this->request->getQuery('key'), 'value');

where PHPUnit’s convention is assertSame($expected, $actual). Because identity comparison is symmetric the result is the same, but swapping to:

$this->assertSame('value', $this->request->getQuery('key'));

would make failure messages more conventional (Failed asserting that actual matches expected) and slightly easier to read when debugging. This is strictly optional and not a functional concern.

Also applies to: 37-58, 60-87, 89-92, 94-116, 118-331

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb01940 and 1e9c018.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • tests/HookTest.php (3 hunks)
  • tests/HttpTest.php (21 hunks)
  • tests/RequestTest.php (1 hunks)
  • tests/ResponseTest.php (9 hunks)
  • tests/RouteTest.php (2 hunks)
  • tests/RouterTest.php (7 hunks)
  • tests/e2e/BaseTest.php (3 hunks)
  • tests/e2e/ResponseFPMTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/HookTest.php (3)
src/Http/Route.php (1)
  • hook (111-116)
src/Http/Request.php (1)
  • getParams (72-75)
tests/MockRequest.php (1)
  • getParams (46-51)
tests/ResponseTest.php (1)
src/Http/Response.php (6)
  • Response (7-928)
  • getStatusCode (443-446)
  • addHeader (489-507)
  • addCookie (551-567)
  • html (830-835)
  • getContentType (401-404)
🪛 GitHub Actions: CodeQL
tests/HookTest.php

[error] 108-108: Empty array passed to foreach.

🔇 Additional comments (7)
tests/HookTest.php (1)

23-39: AssertSame conversions look correct; be aware of the CodeQL foreach warning

  • In testDescriptionCanBeSet, testGroupsCanBeSet, testActionCanBeSet, and testParamCanBeSet, switching to assertSame is safe: the expectations are literals ('', arrays, and 'hello world') and the getters return the same scalar/array types, so stricter comparison is appropriate.
  • In testResourcesCanBeInjected, assertSame('user:00:00:00', $result) is also a good strict check since the hook callback returns a pure string.
  • In testParamValuesCanBeSet, the new assertSame calls against [] and the string values 'hello' / 'world' align with the expected array/param structure and keep the intent of the test intact.

The CodeQL pipeline warning (108-108: Empty array passed to foreach) is pointing at the foreach ($this->hook->getParams() as $key => $param) inside testParamValuesCanBeSet. Given that this foreach is preceded by two ->param() calls that should populate the params, this looks like a static-analysis false positive rather than a behavioural change introduced by this PR. It’s still worth re-running the analysis and confirming there’s no path where getParams() is guaranteed to stay empty before that foreach (for example, if Hook::param()’s contract changes in the future).

Also applies to: 41-57, 59-93, 95-115

tests/e2e/ResponseFPMTest.php (1)

26-57: Strict cookie and status-code assertions are consistent with test intent

All replacements of assertEquals with assertSame in testCookie() look correct:

  • Status codes are compared as 200 (int) against $response['headers']['status-code'], which should already be an int from the client helper.
  • Cookie echoes are compared as exact strings ($cookie), which is what this test is explicitly verifying for FPM (preserving original header formatting).

No issues spotted with these stricter comparisons.

tests/e2e/BaseTest.php (1)

9-128: E2E BaseTest strict equality changes align with response types

Across testResponse, testResponseValue, testHeaders, testHead, testNoContent, testChunkResponse, testRedirect, testHumans, testParamInjection, testNotFound, testCookie, and testSetCookie, the switch to assertSame:

  • Compares string bodies to string literals (e.g. 'Hello World!', 'humans.txt').
  • Compares numeric status codes as ints (e.g. 204, 400, 404, 200) to the status-code header, which should already be an int.
  • Compares header and cookie values to exact string literals.

Given those types, the stricter identity checks are appropriate and should not introduce regressions, while adding useful type-sensitivity to the tests.

tests/RouterTest.php (1)

14-27: Router match tests correctly enforce Route instance identity

The new assertSame checks in the various router tests (testCanMatchUrl, testCanMatchUrlWithPlaceholder, testCanMatchUrlWithWildcard, testCanMatchHttpMethod, testCanMatchAlias, testCanMatchMix, testCanMatchFilename, testCannotFindUnknownRouteByMethod) now verify that:

  • Router::match(...) returns the exact same Route object that was added (object identity), not just an equivalent route.
  • Method/path mismatch tests still rely on assertNotEquals / assertNull, which are unchanged.

Assuming the intended contract is that the router keeps and returns the original Route instances, these stricter assertions are desirable and look correct.

Also applies to: 29-52, 54-69, 71-81, 86-98, 100-121, 123-129, 136-145

tests/RouteTest.php (1)

17-47: RouteTest strict comparisons match Route’s default values and behavior

The switch to assertSame in:

  • testCanGetMethod, testCanGetAndSetPath, testCanSetAndGetDescription, testCanSetAndGetGroups — compares strings/arrays to literals and is type-correct.
  • testCanSetAndGetAction — explicitly checks null before an action is set and 'hello world' after invoking the stored callable; assertSame is appropriate here.
  • testCanGetAndSetParam — verifies getParams() is an empty array by default, using assertSame([], ...) which is fine.
  • testCanSetAndGetLabels — uses string defaults/values and remains correct with strict comparison.

All updated assertions align with the Route API and should behave as intended.

Also applies to: 49-61, 69-76

tests/ResponseTest.php (1)

49-56: FPMResponseTest strict assertions correctly validate chaining, status, and content types

The updated assertSame usages here look solid:

  • testCanGetStatus now strictly checks getStatusCode() against Response::STATUS_CODE_OK (int → int).
  • testCanAddHeader and testCanAddCookie confirm that addHeader() / addCookie() return the same Response instance ($this), which is exactly what fluent APIs should guarantee.
  • All send* tests (testCanSend, testCanSendRedirect, testCanSendText, testCanSendHtml, testCanSendJson, testCanSendJsonp, testCanSendIframe) now strictly compare the captured output buffer and the getContentType() string against exact literals, which tightens guarantees without changing behavior.

No issues identified with these stricter comparisons.

Also applies to: 58-68, 75-88, 90-109, 111-122, 124-135, 137-148, 150-161, 163-174

tests/HttpTest.php (1)

71-98: HttpTest’s stricter assertions correctly enforce modes, outputs, and route identity

Across the updated sections of HttpTest:

  • Mode tests (testCanGetDifferentModes) now strictly compare Http::getMode() to the relevant MODE_TYPE_* constants, which are strings, so assertSame is appropriate.
  • Route execution tests (testCanExecuteRoute*, hook-related tests, and callable-string tests) compare captured output buffers to exact string compositions; all expectations are strings, so strict comparison simply tightens these checks without changing intent.
  • Error-handling tests (e.g. testCanExecuteRouteWithParamsWithError, testAllowRouteOverrides, testCanHookThrowExceptions) now assert on exact error/exception messages as strings, which aligns with how these messages are generated.
  • In route matching tests (testCanMatchRoute and the various edge cases around null/empty/malformed URLs), assertSame($expected, $route) ensures that the router returns the very same Route instance that was registered, not just an equivalent one, which is a sensible strengthening of the tests.
  • testNoMismatchRoute’s stricter checks on $_SERVER['REQUEST_METHOD'] vs getMethod() and $_SERVER['REQUEST_URI'] vs getPath() compare strings to strings, so identity comparison is fine.
  • Wildcard and callable-parameter tests similarly use strict string comparisons for outputs like 'HELLO', 'func-value: system', and 'generated: generated-value'.

All of these assertSame conversions are consistent with the existing behavior and serve to catch potential type or identity regressions more reliably.

Also applies to: 100-140, 142-195, 197-244, 246-380, 382-454, 456-484, 486-559, 582-610, 612-662, 664-702, 737-790, 795-853

@lohanidamodar lohanidamodar merged commit 85b6c3b into master Nov 26, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants