New sniff to prefer specialized phpunit assert functions to improve r…#2811
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughA new PHP CodeSniffer sniff detects and auto-refactors PHPUnit assertions from assertTrue/assertFalse with type-checking functions (is_array, is_object, is_string, is_numeric) to their corresponding assertIs* counterparts. The sniff is registered in the ruleset, test files are updated to use the recommended assertions, and a Stripe Lite test class is removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/phpunit/misc/test_FrmCSVExportHelper.php`:
- Line 167: The assertion uses assertIsString( $csv_path && $csv_path &&
file_exists( $csv_path ) ) which evaluates to a boolean and thus is wrong;
replace it by two separate assertions: verify that $csv_path is a string using
assertIsString($csv_path) (and optionally assertNotEmpty($csv_path)) and verify
the file exists using assertTrue(file_exists($csv_path)) (or
assertFileExists($csv_path)), updating the test around the $csv_path variable
and removing the combined boolean expression.
🧹 Nitpick comments (2)
phpcs-sniffs/Formidable/Sniffs/PHPUnit/PreferAssertIsArraySniff.php (2)
29-41: Class name doesn't reflect full scope.The class is named
PreferAssertIsArraySniffbut it handlesis_array,is_object,is_string, andis_numeric. Consider renaming to something likePreferAssertIsTypeSnifforPreferSpecializedAssertSniffto accurately reflect its broader scope.Additionally, the class docblock (lines 16-28) only documents
is_array,is_object, andis_stringbut omitsis_numericwhich is present inFUNCTION_MAP.
150-150: Remove unused parameter$isFuncPtr.The parameter
$isFuncPtris passed toapplyFix()but never used within the method body. This was also flagged by static analysis.♻️ Proposed fix
Update the method signature at line 150:
- private function applyFix( File $phpcsFile, $methodNamePtr, $openParen, $isFuncPtr, $isFuncOpenParen, $isFuncCloseParen, $assertCloseParen, $newMethodName ) { + private function applyFix( File $phpcsFile, $methodNamePtr, $openParen, $isFuncOpenParen, $isFuncCloseParen, $assertCloseParen, $newMethodName ) {Update the docblock (remove lines 142-143):
* `@param` int $methodNamePtr The assertTrue/assertFalse token position. * `@param` int $openParen The opening parenthesis of assertTrue. - * `@param` int $isFuncPtr The is_* function token position. * `@param` int $isFuncOpenParen The opening parenthesis of is_*.Update the call site at line 132:
- $this->applyFix( $phpcsFile, $stackPtr, $openParen, $firstArg, $isFuncOpenParen, $isFuncCloseParen, $assertCloseParen, $newMethodName ); + $this->applyFix( $phpcsFile, $stackPtr, $openParen, $isFuncOpenParen, $isFuncCloseParen, $assertCloseParen, $newMethodName );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
phpcs-sniffs/Formidable/Sniffs/PHPUnit/PreferAssertIsArraySniff.phpphpcs-sniffs/Formidable/ruleset.xmltests/phpunit/applications/test_FrmApplicationApi.phptests/phpunit/entries/test_FrmEntry.phptests/phpunit/fields/test_FrmField.phptests/phpunit/fields/test_FrmFieldsAjax.phptests/phpunit/forms/test_FrmForm.phptests/phpunit/misc/test_FrmAntiSpam.phptests/phpunit/misc/test_FrmAppHelper.phptests/phpunit/misc/test_FrmCSVExportHelper.phptests/phpunit/stripe/FrmStrpLiteUnitTest.phptests/phpunit/xml/test_FrmXMLHelper.php
💤 Files with no reviewable changes (1)
- tests/phpunit/stripe/FrmStrpLiteUnitTest.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
tests/phpunit/misc/test_FrmAntiSpam.phptests/phpunit/applications/test_FrmApplicationApi.phptests/phpunit/entries/test_FrmEntry.phptests/phpunit/misc/test_FrmAppHelper.phptests/phpunit/fields/test_FrmField.phptests/phpunit/xml/test_FrmXMLHelper.phptests/phpunit/misc/test_FrmCSVExportHelper.phpphpcs-sniffs/Formidable/Sniffs/PHPUnit/PreferAssertIsArraySniff.phptests/phpunit/fields/test_FrmFieldsAjax.phptests/phpunit/forms/test_FrmForm.php
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/PHPUnit/PreferAssertIsArraySniff.php
150-150: Avoid unused parameters such as '$isFuncPtr'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (18)
phpcs-sniffs/Formidable/Sniffs/PHPUnit/PreferAssertIsArraySniff.php (1)
60-134: LGTM!The detection and transformation logic is well-implemented. The sniff correctly:
- Validates method call context (
$this->,self::,static::)- Uses built-in parenthesis matching for reliability
- Preserves additional arguments (like custom messages) by stopping at non-whitespace tokens (lines 170-176)
phpcs-sniffs/Formidable/ruleset.xml (1)
39-40: LGTM!The new PHPUnit sniff rule is correctly registered and follows the existing ruleset structure.
tests/phpunit/xml/test_FrmXMLHelper.php (1)
103-103: LGTM!The assertion change from
assertTrue(is_array(...))toassertIsArray(...)improves readability and provides better failure messages when the assertion fails.tests/phpunit/entries/test_FrmEntry.php (1)
25-25: LGTM!The change to
assertIsNumeric()is consistent with the other type assertions in this file and aligns with the new sniff's guidelines.tests/phpunit/fields/test_FrmField.php (1)
25-25: LGTM!The change to
assertIsNumeric()improves assertion clarity and will provide more descriptive failure messages.tests/phpunit/misc/test_FrmAppHelper.php (2)
99-99: LGTM!Correct use of
assertIsObject()to validate that$settingsis an object instance.
539-539: LGTM!Correct use of
assertIsNotNumeric()with the failure message preserved.tests/phpunit/misc/test_FrmAntiSpam.php (3)
21-21: LGTM!Correct use of
assertIsString()for token validation.
30-30: LGTM!Correct use of
assertIsString()for secret key validation.
39-39: LGTM!Correct use of
assertIsArray()for tokens validation.tests/phpunit/applications/test_FrmApplicationApi.php (2)
15-15: LGTM!Correct use of
assertIsArray()for API response validation.
25-25: LGTM!Correct use of
assertIsArray()for application entry validation.tests/phpunit/fields/test_FrmFieldsAjax.php (4)
24-24: LGTM!Correct use of
assertIsNumeric()for form ID validation.
47-52: LGTM!Correct use of
assertIsNumeric()for field ID andassertIsObject()for field object validation.
95-95: LGTM!Correct use of
assertIsObject()with the failure message preserved.
135-135: LGTM!Correct use of
assertIsNumeric()for field ID validation.tests/phpunit/forms/test_FrmForm.php (2)
16-21: LGTM!The change from
assertTrue(is_numeric(...))toassertIsNumeric()improves test readability and provides more informative failure messages. Good refactor.
26-36: LGTM!Consistent application of the
assertIsNumeric()assertion intest_duplicate(), matching the pattern used intest_create().
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Crabcyborg
deleted the
new_sniff_to_prefer_specialized_phpunit_assert_functions
branch
1 participant
…eadability
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.