apache / apache/jmeter

CI: run SonarCloud analysis on PRs (Clean-as-You-Code) to catch new S3415 and similar regressions

Open
#6,724 0 comments 0 reactions 1 assignee Claimed by @milamberspace View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.