CI: run SonarCloud analysis on PRs (Clean-as-You-Code) to catch new S3415 and similar regressions
- Dominant language
- Java
- Stars
- 9.5k
- Forks
- 2.3k
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 5
Description
## Context
#6718 fixed ~90 occurrences of SonarQube rule `java:S3415` ("assertion arguments passed in the wrong order" — `assertEquals(actual, expected)` instead of `assertEquals(expected, actual)`) across the test suite.
These accumulated because **SonarCloud analysis currently runs out-of-band** — a nightly report to https://sonarcloud.io/dashboard?id=JMeter via the `org.sonarqube` Gradle plugin — **not as a per-PR check**. So nothing flags a new violation when a PR introduces one.
Opening this per the discussion on #6718 (@vlsi asked whether we can add something to avoid re-introducing this type of error).
## Options (rough order of effort)
**1. Add SonarCloud PR analysis to the GitHub Actions CI (preferred).**
The `org.sonarqube` plugin is already wired (org `apache`, project `JMeter`); the missing piece is a `sonar` step on the PR workflow plus SonarCloud's "Clean as You Code" quality gate, which fails a PR that introduces a new violation. This directly targets `S3415` (and every other Sonar rule) 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`.**
Error Prone already runs in CI (the "Error Prone (JDK 21)" job), so this is the cheapest to trial. It can flag swapped *variables* whose names don't match the `expected` / `actual` parameters, but is unreliable for literals. Evaluate signal vs. noise before promoting it to an error.
**3. AssertJ migration (largest, most durable).**
`assertThat(actual).isEqualTo(expected)` removes the ambiguity by construction — there's no "which arg is expected?" left to get wrong. A big, separate effort, but it eliminates the whole class of bug instead of policing it.
## Notes
- SonarCloud PR analysis needs a `SONAR_TOKEN` (org `apache`) available to the workflow — coordinate with ASF Infra / the existing nightly setup, and mind that PR analysis from forks needs a `pull_request_target`-style flow or the token exposed safely.
- Related PR: #6718 (the fix that surfaced this).
Contributor guide
Assessment
This issue has not been assessed yet.