milamberspace commented on PR #6718: URL: https://github.com/apache/jmeter/pull/6718#issuecomment-4928395298
Good point — worth tackling so this doesn't creep back in. A few concrete options: **1. Add SonarCloud PR analysis to the CI (best fit).** This exact class of issue is SonarQube rule `java:S3415`, which is how these 90 were found. JMeter already has the `org.sonarqube` Gradle plugin wired (org `apache`, project `JMeter`), but today it runs **out-of-band** — a nightly report to https://sonarcloud.io/dashboard?id=JMeter, not a per-PR check. That's why these accumulated unnoticed. Wiring a `sonar` step into the PR workflow (SonarCloud supports PR decoration + a "Clean as You Code" gate that fails a PR introducing a new violation) would catch this at review time. `S3415` is heuristic (name-based — it fires when a literal/constant sits in the "actual" slot), so it catches the common cases, not 100%. **2. Error Prone `ArgumentSelectionDefectChecker`.** We already run Error Prone (the "Error Prone (JDK 21)" CI job), so this is the cheapest to trial. Its argument-selection check can flag swapped **variables** whose names don't match the `expected`/`actual` parameters — though it's unreliable for literals. Worth evaluating whether promoting it to an error is net-positive vs. noise. **3. AssertJ migration (largest, most durable).** `assertThat(actual).isEqualTo(expected)` makes the order unambiguous by construction — there's no "which arg is expected?" left to get wrong. A big, separate effort, but it removes the whole class of bug instead of policing it. My take: (1) is the most direct — it targets exactly this pattern and JMeter is already on SonarCloud; the missing piece is running it on PRs rather than only nightly. Happy to open a follow-up issue to track it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
