-
Notifications
You must be signed in to change notification settings - Fork 35
Replace assertEquals with assertSame #194
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
WalkthroughThis PR systematically replaces Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Areas requiring attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/HookTest.php (3)
🪛 PHPMD (2.15.0)tests/HookTest.php113-113: Avoid unused local variables such as '$param'. (undefined) (UnusedLocalVariable) 🔇 Additional comments (4)
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. Comment |
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
🧹 Nitpick comments (1)
tests/RequestTest.php (1)
22-35: AssertSame usage is type-safe; consider normalizing expected/actual orderAll of the new
assertSamecalls 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) ornullon invalid inputs, which works well with strict comparison and reinforces that these methods do not fall back to falsy values likefalseor0.- 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
⛔ Files ignored due to path filters (1)
composer.lockis 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, andtestParamCanBeSet, switching toassertSameis 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 newassertSamecalls 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 theforeach ($this->hook->getParams() as $key => $param)insidetestParamValuesCanBeSet. 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 wheregetParams()is guaranteed to stay empty before that foreach (for example, ifHook::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 intentAll replacements of
assertEqualswithassertSameintestCookie()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 typesAcross
testResponse,testResponseValue,testHeaders,testHead,testNoContent,testChunkResponse,testRedirect,testHumans,testParamInjection,testNotFound,testCookie, andtestSetCookie, the switch toassertSame:
- 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 thestatus-codeheader, 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 identityThe new
assertSamechecks in the various router tests (testCanMatchUrl,testCanMatchUrlWithPlaceholder,testCanMatchUrlWithWildcard,testCanMatchHttpMethod,testCanMatchAlias,testCanMatchMix,testCanMatchFilename,testCannotFindUnknownRouteByMethod) now verify that:
Router::match(...)returns the exact sameRouteobject 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
Routeinstances, 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 behaviorThe switch to
assertSamein:
testCanGetMethod,testCanGetAndSetPath,testCanSetAndGetDescription,testCanSetAndGetGroups— compares strings/arrays to literals and is type-correct.testCanSetAndGetAction— explicitly checksnullbefore an action is set and'hello world'after invoking the stored callable;assertSameis appropriate here.testCanGetAndSetParam— verifiesgetParams()is an empty array by default, usingassertSame([], ...)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 typesThe updated
assertSameusages here look solid:
testCanGetStatusnow strictly checksgetStatusCode()againstResponse::STATUS_CODE_OK(int → int).testCanAddHeaderandtestCanAddCookieconfirm thataddHeader()/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 thegetContentType()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 identityAcross the updated sections of
HttpTest:
- Mode tests (
testCanGetDifferentModes) now strictly compareHttp::getMode()to the relevantMODE_TYPE_*constants, which are strings, soassertSameis 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 (
testCanMatchRouteand the various edge cases around null/empty/malformed URLs),assertSame($expected, $route)ensures that the router returns the very sameRouteinstance that was registered, not just an equivalent one, which is a sensible strengthening of the tests.testNoMismatchRoute’s stricter checks on$_SERVER['REQUEST_METHOD']vsgetMethod()and$_SERVER['REQUEST_URI']vsgetPath()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
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.