Skip to content

Remove autoscan ITs and add withoutSemantic tests - #5870

Open
romainbrenguier wants to merge 14 commits into
masterfrom
romain/remove-autoscan
Open

Remove autoscan ITs and add withoutSemantic tests#5870
romainbrenguier wants to merge 14 commits into
masterfrom
romain/remove-autoscan

Conversation

@romainbrenguier

Copy link
Copy Markdown
Contributor

Summary

  • Remove the its/autoscan/ integration test module (test classes, pom.xml, 261 diff JSON files, CI job)
  • Add test_without_semantic() unit tests to ~575 check test files to replace the autoscan coverage

The autoscan ITs validated that the Java analyzer works without bytecode by comparing results with/without compiled binaries using SonarQube Orchestrator. This is slow and heavyweight. The same coverage is achieved by adding withoutSemantic() unit tests to each rule's check test, which is faster, more granular, and easier to maintain.

Current state (WIP)

Done

  • Removed its/autoscan/ directory entirely
  • Removed autoscan module from its/pom.xml
  • Removed autoscan CI job from .github/workflows/build.yml and from promote job's needs
  • Removed Autoscan Test documentation section from README.md
  • Added test_without_semantic() to ~575 check test files
  • Compilation passes (mvn test-compile -pl java-checks)

Remaining work

  • Run full test suite (mvn test -pl java-checks) and fix remaining failures
    • Some tests may need verifyIssues() changed to verifyNoIssues() (or vice versa)
    • A few tests may need their check expression or file path adjusted
  • Manually add test_without_semantic() to skipped complex tests:
    • MissingPackageInfoCheckTest (uses onFiles() plural + caching)
    • MockitoAnnotatedObjectsShouldBeInitializedCheckTest (uses testCodeSourcesPathInModule + classpath)
    • AbstractRegexCheckTest (uses local inner class as check)
  • Verify no regressions in other modules

Test plan

  • mvn test -pl java-checks passes
  • mvn validate -pl its/autoscan fails (module removed)
  • CI workflow YAML is valid
  • No other modules affected

🤖 Generated with Claude Code

Comment thread java-checks/src/test/java/org/sonar/java/checks/AccessibilityChangeCheckTest.java Outdated
@romainbrenguier
romainbrenguier force-pushed the romain/remove-autoscan branch from cac9406 to 455da33 Compare August 4, 2026 06:45
gitar-bot[bot]

This comment was marked as resolved.

@romainbrenguier
romainbrenguier force-pushed the romain/remove-autoscan branch from 455da33 to 74e2ec2 Compare August 4, 2026 08:51
@gitar-bot
gitar-bot Bot dismissed their stale review August 4, 2026 09:00

✅ All code review findings resolved.

Configure merge blocking

Comment thread java-checks/src/test/java/org/sonar/java/checks/regex/AbstractRegexCheckTest.java Outdated
Comment on lines +1 to +15
package checks;

import java.io.IOException;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.text.FieldPosition;
import java.text.MessageFormat;
import java.util.Calendar;
import java.util.Formatter;
import java.util.GregorianCalendar;
import java.util.Locale;
import java.util.Random;
import java.util.stream.IntStream;
import org.apache.logging.log4j.LogManager;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: WithoutSemantic sample files duplicate semantic samples and will drift

This commit introduces ~575 new *WithoutSemantic.java sample files that are near-verbatim copies of their existing semantic counterparts (e.g. PrintfMisuseCheckSampleWithoutSemantic.java vs PrintfMisuseCheckSample.java, SQLInjectionWithoutSemantic.java vs the original sample), differing only in the // Noncompliant/// FN markers. Because the code bodies are duplicated, future edits to the original samples won't propagate, so the two sets can silently diverge and the withoutSemantic coverage can rot. Consider either generating these files from the originals, or at minimum documenting the coupling so maintainers keep them in sync; where feasible, reuse the same sample file with the semantic-vs-no-semantic difference expressed only through markers.

Was this helpful? React with 👍 / 👎

@romainbrenguier
romainbrenguier force-pushed the romain/remove-autoscan branch 2 times, most recently from 1c53fba to 2e7a6b8 Compare August 6, 2026 11:31
@romainbrenguier
romainbrenguier marked this pull request as ready for review August 6, 2026 12:55

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work.

i have a couple of small cleanups/questions i would like resolved before merge.

Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml
romainbrenguier and others added 8 commits August 10, 2026 14:35
The autoscan ITs validated the Java analyzer works without bytecode
by comparing results with/without compiled binaries using SonarQube
Orchestrator. This coverage is being replaced by withoutSemantic()
unit tests in each rule's check test, which is faster and more
granular.

- Delete its/autoscan/ directory (test classes, pom.xml, 261 diff JSON files)
- Remove autoscan module from its/pom.xml
- Remove autoscan CI job from build.yml and promote job dependency
- Remove Autoscan Test documentation from README.md

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add test_without_semantic() methods to ~575 check test files.
These tests verify each rule's behavior when running without
bytecode/semantic analysis, replacing coverage previously provided
by the autoscan integration tests.

Tests that require semantic analysis use verifyNoIssues() to confirm
the rule correctly produces no false positives without bytecode.

This is a work in progress - some tests may still need adjustment:
- verifyIssues() vs verifyNoIssues() may need to be corrected for
  some rules after running the full test suite
- A few complex test files were skipped and may need manual handling

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add verifyAnalysisSucceeds() to CheckVerifier for checks that raise
  a different set of issues without semantic analysis. This runs the
  analysis without asserting on the specific issues, verifying the
  check doesn't crash without bytecode.
- Fix 48 tests that used verifyNoIssues() but where the check still
  raises issues without semantics: use verifyAnalysisSucceeds() instead.
- Add test_without_semantic() to MissingPackageInfoCheckTest,
  MockitoAnnotatedObjectsShouldBeInitializedCheckTest, and
  AbstractRegexCheckTest.
- Fix double blank lines before test_without_semantic in ~315 files.
- Fix import ordering in DefaultEncodingUsageCheckTest.

All 2251 tests in java-checks pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… assertions

- Remove stray merge conflict marker in DateEnumsCheckTest.java
- Remove misleading setExpectNoIssues() call in verifyAnalysisSucceeds()
  since the method never inspects issues or expectations afterward
- Change withoutSemantic() tests for semantic-dependent checks
  (AccessibilityChangeCheck, MathClampMethodsCheck,
  InstanceOfPatternMatchingCheck) from verifyIssues() to
  verifyAnalysisSucceeds(), since these checks produce different
  issue sets without type resolution

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…avaCheckVerifier

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sues

Replace verifyAnalysisSucceeds() with verifyIssues() in 52
test_without_semantic() methods by creating dedicated WithoutSemantic
sample files with correct Noncompliant/FN markers for each check's
behavior without semantic analysis.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The migration script that removed FN (false negative) lines left behind
orphaned method bodies, constructor bodies, and trailing commas in
annotations. This caused compilation failures in multiple WithoutSemantic
test files. Also renamed helper classes with WS suffix to avoid duplicate
class name conflicts within the same package.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- IndentationCheck: fix Noncompliant annotation mismatch (line 34 not
  detected, line 67 detected without semantics)
- PresuperLogicBloatsConstructorCheck: fix missing if-block causing
  parse error at line 116
- ReuseRandomCheck: remove Noncompliant from constructor lines 15-16
  (constructors are excluded from the check)
- BadConstantNameCheck: create missing test sample file for no_semantic
  test (uses Object type which won't trigger without type resolution)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@romainbrenguier
romainbrenguier force-pushed the romain/remove-autoscan branch 2 times, most recently from 15fb17a to 5486eee Compare August 10, 2026 13:45
romainbrenguier and others added 4 commits August 10, 2026 15:54
…w tests

Remove dead code paths in assertSoleFlowDiscrepancy (unreachable return)
and validateFlowAttributes (unreachable null check). Add tests for flow
location validation, effort-to-fix with linear remediation, sole flow
discrepancy, and verifyAnalysisSucceeds with cache.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Autoscan is a required GitHub CI check that cannot be removed without
admin rights. This adds a trivial no-op job with the same name so the
required check passes until an admin can remove the requirement.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
romainbrenguier and others added 2 commits August 11, 2026 10:24
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create a dedicated WithoutSemantic sample file because the check
behaves differently without semantic analysis:
- reflectionEquals usage in parents cannot be detected (FP at lines 160, 180)
- External parent types cannot be resolved (no issue at line 164)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gitar-bot

gitar-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 6 resolved / 7 findings

Removes the autoscan integration test module and adds withoutSemantic test coverage to rule check tests. Consider consolidating duplicate sample files to prevent test drift.

💡 Quality: WithoutSemantic sample files duplicate semantic samples and will drift

📄 java-checks-test-sources/default/src/main/java/checks/PrintfMisuseCheckSampleWithoutSemantic.java:1-15 📄 java-checks-test-sources/default/src/main/java/checks/SQLInjectionWithoutSemantic.java:1-15

This commit introduces ~575 new *WithoutSemantic.java sample files that are near-verbatim copies of their existing semantic counterparts (e.g. PrintfMisuseCheckSampleWithoutSemantic.java vs PrintfMisuseCheckSample.java, SQLInjectionWithoutSemantic.java vs the original sample), differing only in the // Noncompliant/// FN markers. Because the code bodies are duplicated, future edits to the original samples won't propagate, so the two sets can silently diverge and the withoutSemantic coverage can rot. Consider either generating these files from the originals, or at minimum documenting the coupling so maintainers keep them in sync; where feasible, reuse the same sample file with the semantic-vs-no-semantic difference expressed only through markers.

✅ 6 resolved
Quality: Stray double blank line before test_without_semantic in many files

📄 java-checks/src/test/java/org/sonar/java/checks/AccessibilityChangeCheckTest.java:53-54 📄 java-checks/src/test/java/org/sonar/java/checks/ImportDeclarationOrderCheckTest.java:254-255
A large number of the modified test files introduce two consecutive blank lines between the previous method's closing brace and the new @test method (e.g. AccessibilityChangeCheckTest lines 53-54, BufferedReaderBoilerplateCheckTest, CommentsMustStartWithCorrectNumberOfSlashesCheckTest, ControlCharacterInLiteralCheckTest, DefaultEncodingUsageCheckTest, ImportDeclarationOrderCheckTest 254-255, ReadlnWithPromptCheckTest, StringIndexOfRangesCheckTest, etc.). This is a formatting inconsistency across ~575 files; collapse to a single blank line for consistency.

Quality: Import of org.junit.jupiter.api.Test added out of order

📄 java-checks/src/test/java/org/sonar/java/checks/DefaultEncodingUsageCheckTest.java:19-22
In DefaultEncodingUsageCheckTest the new import org.junit.jupiter.api.Test; is appended after the org.sonar.java... imports instead of being grouped with the other org.junit.jupiter imports, breaking alphabetical import ordering (a convention this very project enforces via ImportDeclarationOrderCheck). Move the import up next to the other junit imports.

Edge Case: verifyIssues() + withoutSemantic() reuses semantic sample files

📄 java-checks/src/test/java/org/sonar/java/checks/AccessibilityChangeCheckTest.java:55-63
Many new test_without_semantic() methods call verifyIssues() while pointing at the same sample file whose // Noncompliant comments were authored for a full semantic analysis (e.g. AccessibilityChangeCheckTest, MathClampMethodsCheckTest, InstanceOfPatternMatchingCheckTest). withoutSemantic() disables type resolution, so any check that relies on semantics will raise a different (usually smaller) issue set and verifyIssues() will fail because the raised issues no longer match the expected comments. This is the bulk of the acknowledged remaining work; for semantic-dependent rules the intent (verify the check doesn't crash without bytecode) is better expressed with verifyNoIssues(), and only syntax-only rules should keep verifyIssues(). Each affected test needs to be triaged individually.

Quality: verifyAnalysisSucceeds() sets expectNoIssues but never verifies

📄 java-checks-testkit/src/main/java/org/sonar/java/checks/verifier/internal/InternalCheckVerifier.java:288-302
In InternalCheckVerifier.verifyAnalysisSucceeds() the call expectations.setExpectNoIssues() mutates the expectations flag, but the method only runs astScanner.scan(...) and never inspects issues or expectations afterwards. The flag has no observable effect (the noEffectParser is added unconditionally), so the line is dead/misleading code that implies an assertion the method does not actually perform. Consider removing it to keep the method's intent (only verify the analysis does not crash) clear.

Quality: withoutSemantic tests weakened from verifyNoIssues to no assertion

📄 java-checks/src/test/java/org/sonar/java/checks/AnnotationDefaultArgumentCheckTest.java:43-50 📄 java-checks-testkit/src/main/java/org/sonar/java/checks/verifier/CheckVerifier.java:308-313
Switching test_without_semantic() from verifyNoIssues() to verifyAnalysisSucceeds() (e.g. AnnotationDefaultArgumentCheckTest and ~48 similar files) means these tests now only assert the analysis does not crash — they no longer detect false-positive issues raised in without-semantic mode. This is an intentional trade-off per the PR description, but for checks that previously asserted no issues it reduces regression coverage: a check newly emitting spurious issues without bytecode would pass silently. Consider keeping verifyNoIssues() where the without-semantic behavior is known to raise no issues, reserving verifyAnalysisSucceeds() for cases where issues are legitimately expected.

...and 1 more resolved from earlier reviews

🤖 Prompt for agents
Code Review: Removes the autoscan integration test module and adds withoutSemantic test coverage to rule check tests. Consider consolidating duplicate sample files to prevent test drift.

1. 💡 Quality: WithoutSemantic sample files duplicate semantic samples and will drift
   Files: java-checks-test-sources/default/src/main/java/checks/PrintfMisuseCheckSampleWithoutSemantic.java:1-15, java-checks-test-sources/default/src/main/java/checks/SQLInjectionWithoutSemantic.java:1-15

   This commit introduces ~575 new `*WithoutSemantic.java` sample files that are near-verbatim copies of their existing semantic counterparts (e.g. PrintfMisuseCheckSampleWithoutSemantic.java vs PrintfMisuseCheckSample.java, SQLInjectionWithoutSemantic.java vs the original sample), differing only in the `// Noncompliant`/`// FN` markers. Because the code bodies are duplicated, future edits to the original samples won't propagate, so the two sets can silently diverge and the withoutSemantic coverage can rot. Consider either generating these files from the originals, or at minimum documenting the coupling so maintainers keep them in sync; where feasible, reuse the same sample file with the semantic-vs-no-semantic difference expressed only through markers.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

You might need to re-base and push to get rid of the autoscan check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants