Exclude tests/phpunit from DeepSource PHP analysis - #3269
Exclude tests/phpunit from DeepSource PHP analysis#3269NathanaelJonesIreland wants to merge 1 commit into
Conversation
The PHP analyzer cannot resolve PHPUnit\Framework\TestCase, because it lives under the already-excluded vendor/**. Every assert*() call in a PHPUnit test file is therefore reported as "Call to an undefined method", so any PR that adds or edits a test file goes red on DeepSource: PHP regardless of its contents. Measured on PR #3242: 56 findings, 55 of them of this shape (33 in test_FrmStyle.php, 15 in test_FrmCreateFile.php, 5 in test_FrmStylesController.php, 2 in test_FrmMigrate.php). PR #3256 draws the same class of finding on the one test file it edits, a file that already carried assertions on master. test_patterns already lists these paths and is not sufficient: it relaxes the style and security categories but still reports bug-risk issues such as undefined methods. exclude_patterns is the only setting that stops them. PHPStan and Psalm are green on the same files because both load stubs.php explicitly (which declares WP_UnitTestCase -> WP_UnitTestCase_Base -> PHPUnit\Framework\TestCase); DeepSource has no equivalent stub-path setting. The green PHPUnit matrix is the other evidence these are artifacts: a suite cannot pass while calling undefined methods. Mirrors how every other analyzer in this repo already treats these files - phpstan.neon excludes */tests/*, phpcs.xml excludes a list of rules for tests/phpunit/*. Scoped to tests/phpunit/** rather than **/tests/**, so the JavaScript analyzer keeps covering tests/cypress (it currently passes there). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe DeepSource configuration now excludes ChangesDeepSource analysis configuration
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The scoped PHPUnit analysis exclusion is merge-ready after normal checks and review; the rationale in the configuration should be corrected or clarified, but no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Aug 21, 2026 8:04p.m. | Review ↗ |
| JavaScript | |
Aug 21, 2026 8:04p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
DeepSource: PHP fails this PR on PHP-A1004 ("Use of insecure md5() function")
at the new update_css_version() call. It is a false positive: the value is a
12-character content-derived cache-busting token written to
frm_last_style_update, not a password. The check's suggested remedy,
password_hash(), is salted and non-deterministic, so it would defeat the entire
mechanism -- the whole point is that identical stylesheet bytes must reproduce
an identical version string.
Suppressed at the line rather than repo-wide, following the existing skipcq
convention in this codebase (classes/views/frm-entries/show.php,
classes/views/frm-fields/back-end/field-options.php, and the phpcs-sniffs
CommentSpacingSniff which explicitly whitelists skipcq: comments). Note that
classes/models/ already contains seven md5()-for-cache-key calls of exactly this
kind (FrmAddon, FrmAntiSpam, FrmApplicationApi, FrmFormApi, FrmFormTemplateApi,
FrmStyleApi, FrmUsage), none annotated -- DeepSource only reports on lines the
diff touches, which is why this one surfaced and those did not.
The remaining DeepSource findings on this PR are all in tests/phpunit/, which is
what #3269 addresses; this commit does not duplicate that change.
Verified: php -l clean; the custom Formidable.Commenting.CommentSpacing sniff
passes on the file; php-cs-fixer --dry-run --allow-risky=yes exits 0 against the
LF form CI analyses (the working tree is CRLF via autocrlf, which cs-fixer
reports on its own and is unrelated to this change).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Babysit note — labels added, no code touched. This PR had no labels and no reviews since it was opened 3h ago, which made its check status misleading rather than green. Every gate that would actually exercise the change read Added The two bot comments from this run are both clean and need no action: CodeRabbit reports "no actionable comments were generated", DeepSource reports grade A on Left entirely alone otherwise: this is a core product repo, so no fix, push or merge from the automation — it needs a human approval, which a bot pass does not substitute for. |
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
There was a problem hiding this comment.
Config-only change, well-reasoned. .deepsource.toml gains one exclude entry (tests/phpunit/**) plus an inline comment explaining why: DeepSource's PHP analyzer can't resolve PHPUnit\Framework\TestCase (declared via vendor/**, which is already excluded), so every assert*() call in a test file reads as an undefined-method call — a bug-risk finding that test_patterns doesn't suppress (it only relaxes style/security).
Checked:
- Mirrors existing precedent:
phpstan.neon'sexcludePaths: */tests/*andphpcs.xml'stests/phpunit/*exclusions already treat this directory the same way. - Scoped to
tests/phpunit/**, not**/tests/**—tests/cypress(JS analyzer) is untouched, confirmed still passing in this PR's own checks. - DeepSource: PHP is green on this PR's head commit with the new config applied, and the PHP 7.4/8 test matrix passes — consistent with "these were false positives, not real failures."
- No linked issue to route a reviewer request to.
Nothing blocking. Approving.
2 participants
The problem
DeepSource: PHPgoes red on any PR that adds or edits a PHPUnit test file, regardless of what the PR does. The analyzer cannot resolvePHPUnit\Framework\TestCase— it lives under the already-excludedvendor/**— so everyassert*()call in a test file is reported as "Call to an undefined method".Measured on #3242, which is red right now: 56 findings, 55 of them of exactly this shape.
tests/phpunit/styles/test_FrmStyle.phptests/phpunit/misc/test_FrmCreateFile.phptests/phpunit/styles/test_FrmStylesController.phptests/phpunit/database/test_FrmMigrate.phpclasses/models/FrmStyle.phpSample:
Call to an undefined method test_FrmStyle::assertNotEmpty(),…::assertSame(),…::assertTrue(),…::assertCount(), andCall to an undefined static method FrmUnitTest::tearDown().It is not limited to new test files. #3256 merely edits
tests/phpunit/fields/test_FrmFieldValidate.php— a file that already carried assertions onmaster— and draws the same class of finding.Why
test_patternsdoesn't already handle it.deepsource.tomlalready lists these paths undertest_patterns, so this looks like it should be solved. It isn't:test_patternsrelaxes the style and security categories but still reports bug-risk issues, and "undefined method" is bug-risk.exclude_patternsis the only setting that stops them.Why these are definitely artifacts, not defects
stubs.phpdeclaresWP_UnitTestCase extends WP_UnitTestCase_Base extends PHPUnit\Framework\TestCase, and bothphpstan.neonandpsalm.xmlloadstubs.phpexplicitly. That is precisely why PHPStan and Psalm are green on the same files DeepSource calls broken. DeepSource has no equivalent stub-path setting.Why exclusion is the right fix rather than a workaround
Every other analyzer in this repo already treats these files this way:
phpstan.neon→excludePaths: */tests/*phpcs.xml→ a list of rule exclusions scoped totests/phpunit/*This change brings DeepSource in line with them. Since the analyzer cannot resolve the base class, its findings on this directory are 100% false positives, so nothing of value is being given up.
Scoped deliberately to
tests/phpunit/**rather than**/tests/**, so the JavaScript analyzer keeps coveringtests/cypress— it currently passes there and this shouldn't touch it.test_patternsand both[[analyzers]]blocks are unchanged; TOML validated.What this does and does not fix
It removes the recurring red. It is not cosmetic: the noise has real cost — this failure class has been re-diagnosed on #3242 across roughly a dozen separate triage passes, and it has trained everyone to read
DeepSource: PHPas "probably nothing," which is exactly how a genuine finding gets waved through. #3234 merged with 6 open DeepSource threads and #3235 with 9, both on test files.The one remaining finding on #3242 is a real (if arguable) one in product code —
Use of insecure md5() function foundinclasses/models/FrmStyle.php, where the hash is an opaque cache-busting key rather than a credential. That belongs to #3242 and is deliberately left out of this PR.Requesting a look from @Crabcyborg since this is a repo-wide CI policy change rather than a fix to my own branch.
🤖 Generated with Claude Code
Summary by CodeRabbit