Conversation
hashicorp-vault-sonar-prod
Bot
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
|
| @Test | ||
| void rule_metadata_is_available_on_test_classpath() throws IOException { | ||
| var rules = ClassPath.from(getClass().getClassLoader()).getTopLevelClassesRecursive("org.sonar.java.checks").stream() | ||
| .map(ClassPath.ClassInfo::load) | ||
| .filter(check -> check.isAnnotationPresent(Rule.class)) | ||
| .toList(); | ||
| assertThat(rules).isNotEmpty(); | ||
| for (Class<?> rule : rules) { | ||
| String key = rule.getAnnotation(Rule.class).key(); | ||
| assertThat(getClass().getResource("/org/sonar/l10n/java/rules/java/" + key + ".json")) | ||
| .as("Remediation metadata for %s (%s)", key, rule.getName()).isNotNull(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Quality: Repo-wide metadata guard duplicated inside two rule test classes
The new rule_metadata_is_available_on_test_classpath test is a module-wide invariant (it discovers every annotated top-level check under org.sonar.java.checks and asserts its <ruleKey>.json is on the test classpath), but it is copy-pasted verbatim into two unrelated single-rule test classes, so a future change to the invariant has to be made in two places and the guard is invisible to anyone looking for it. Since both modules already put ../sonar-java-plugin/src/main/resources on the test classpath (java-checks/pom.xml:104, java-checks-aws/pom.xml:90), the shared body belongs in one helper (e.g. in java-checks-testkit next to RuleMetadataValidator, or a dedicated RuleMetadataClasspathTest per module) that both modules call with their base package.
Extract the guard into a shared testkit helper and call it from one dedicated test class per check module.:
// java-checks-testkit: new public helper, e.g. org.sonar.java.checks.verifier.RuleMetadataAssert
public static void assertMetadataAvailable(ClassLoader loader, String basePackage) throws IOException {
var rules = ClassPath.from(loader).getTopLevelClassesRecursive(basePackage).stream()
.map(ClassPath.ClassInfo::load)
.filter(check -> check.isAnnotationPresent(Rule.class))
.toList();
assertThat(rules).isNotEmpty();
for (Class<?> rule : rules) {
String key = rule.getAnnotation(Rule.class).key();
assertThat(loader.getResource("org/sonar/l10n/java/rules/java/" + key + ".json"))
.as("Remediation metadata for %s (%s)", key, rule.getName()).isNotNull();
}
}
// then each module keeps a one-line dedicated test calling this helper
Was this helpful? React with 👍 / 👎
| @ParameterizedTest | ||
| @ValueSource(strings = { | ||
| "{\"func\":\"Constant/Issue\",\"constantCost\":\"5min\"}", | ||
| "{\"func\":\"Linear\",\"linearFactor\":\"2h\"}", | ||
| "{\"func\":\"Linear with offset\",\"linearFactor\":\"1min\",\"linearOffset\":\"0min\"}" | ||
| }) | ||
| void valid_remediation(String remediation) { | ||
| assertThat(RuleMetadataValidator.readFunction(new StringReader("{\"remediation\":" + remediation + "}"), "Test")) | ||
| .isIn("Constant/Issue", "Linear", "Linear with offset"); | ||
| } |
There was a problem hiding this comment.
💡 Quality: valid_remediation test cannot detect a wrong function mapping
valid_remediation asserts only isIn("Constant/Issue", "Linear", "Linear with offset"), so it passes for any of the three inputs even if readFunction returned the wrong function name (e.g. returning "Linear" for the Constant/Issue input), which is exactly the mapping the validate gap rejection depends on. Pair each input with its expected return value so the mapping itself is covered.
Assert the exact function returned for each metadata input.:
@ParameterizedTest
@CsvSource(delimiter = '|', value = {
"{"func":"Constant/Issue","constantCost":"5min"} | Constant/Issue",
"{"func":"Linear","linearFactor":"2h"} | Linear",
"{"func":"Linear with offset","linearFactor":"1min","linearOffset":"0min"} | Linear with offset"
})
void valid_remediation(String remediation, String expectedFunction) {
assertThat(RuleMetadataValidator.readFunction(new StringReader("{"remediation":" + remediation + "}"), "Test"))
.isEqualTo(expectedFunction);
}
Was this helpful? React with 👍 / 👎
CI failed: The CI build failed due to a SonarQube server endpoint returning HTTP 404 during scanner bootstrapping.OverviewOne job failure was encountered where the Maven build failed during the execution of the FailuresSonarQube Scanner Bootstrapping Failure (confidence: high)
Summary
Code Review 👍 Approved with suggestions 3 resolved / 5 findingsAdds comprehensive remediation metadata validation for issue gaps across both modern and deprecated check verifiers, with metadata caching and support for legacy resource naming. The implementation correctly handles linear gaps omitting cost properties and validates gaps through analyzer-commons expectations. Consider extracting the duplicate 💡 Quality: Repo-wide metadata guard duplicated inside two rule test classes📄 java-checks/src/test/java/org/sonar/java/checks/SunPackagesUsedCheckTest.java:30-42 📄 java-checks-aws/src/test/java/org/sonar/java/checks/aws/AwsLongTermAccessKeysCheckTest.java:30-42 The new Extract the guard into a shared testkit helper and call it from one dedicated test class per check module.💡 Quality: valid_remediation test cannot detect a wrong function mapping
Assert the exact function returned for each metadata input.✅ 3 resolved✅ Performance: Rule metadata re-read and re-parsed for every reported issue
✅ Quality: Validation silently no-ops if metadata copy stops resolving
✅ Quality: Test resources reach across modules into a sibling source tree
🤖 Prompt for agentsReview coverageFunctional validation No results Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Closing in favour of https://github.com/SonarSource/rspec/pull/8170 |
1 participant
Part of SONARJAVA-6960
Summary
Validate issues produced by both the modern and deprecated check verifiers against SonarJava's bundled remediation metadata.
Constant/Issueremediation (the S1191 failure mode rejected by SonarQube'sDebtCalculator).[[effortToFix=N]]expectations detect incorrect or missing reported values. The deprecated verifier now checks these expectations independently of metadata availability too.<ruleKey>.jsonand legacy<ruleKey>_java.jsonresource names.Linear and linear-with-offset remediation may omit the gap:
DebtCalculatorthen uses a multiplier of one. The previous deprecated-verifier assertion requiring an explicit linear gap was incorrect and has been removed. SonarJava's nonpositive internal cost sentinels become null gaps and remain valid.Coverage and limits
This validates emitted issues exercised by tests against the metadata bundled with the analyzer. It does not compare against the latest upstream RSPEC checkout or server-side remediation overrides, and it cannot exercise untested reporting branches. Metadata alone cannot establish whether a reported gap counts the correct things: rule fixtures should supply
[[effortToFix=N]]when the exact value matters.Verification
2.0, as intended; the metadata was restored.git diff --checkpasses.The stricter legacy gap comparison also exposed a fake check in the testkit that reported no gap despite its fixture expecting four; the fake check now reports the expected gap.