Skip to content

re-enable the spotbug checks for TEST files, and add specific exclusions.#10998

Open
pragatiguptaaa wants to merge 1 commit intomasterfrom
KSQL-13446
Open

re-enable the spotbug checks for TEST files, and add specific exclusions.#10998
pragatiguptaaa wants to merge 1 commit intomasterfrom
KSQL-13446

Conversation

@pragatiguptaaa
Copy link
Copy Markdown
Member

Description

re-enable the spotbug checks for TEST files, and add specific exclusions.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")
  • Do these changes have compatibility implications for rollback? If so, ensure that the ksql command version is bumped.

Copilot AI review requested due to automatic review settings April 6, 2026 18:06
@pragatiguptaaa pragatiguptaaa requested a review from a team as a code owner April 6, 2026 18:06
@confluent-cla-assistant
Copy link
Copy Markdown

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR re-enables SpotBugs analysis for test classes and replaces the previous blanket test-class exclusion with more targeted SpotBugs suppression rules.

Changes:

  • Re-enable SpotBugs test analysis via includeTests and add a test-compile SpotBugs execution.
  • Replace the prior “exclude all *Test classes” rule with targeted bug-pattern exclusions for test-like classes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pom.xml Enables SpotBugs test scanning and adds a test-compile execution to run checks on test bytecode.
findbugs/findbugs-exclude.xml Narrows the prior blanket test exclusion into specific bug-pattern suppressions for classes matching *Test*.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1532 to 1537
<excludeFilterFile>${project.basedir}/../findbugs/findbugs-exclude.xml</excludeFilterFile>
<effort>Max</effort>
<threshold>Max</threshold>
<failOnError>true</failOnError>
<!-- <includeTests>true</includeTests>-->
<includeTests>true</includeTests>
</configuration>
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

<includeTests>true</includeTests> is configured at the plugin level, so it applies to both the compile and test-compile executions. This can cause the compile-phase SpotBugs run to attempt test analysis before target/test-classes exists and also makes the test-compile execution re-analyze main classes, duplicating work. Consider moving includeTests into the analyze-test-compile execution’s <configuration> (and explicitly disabling it for analyze-compile), or using a single execution after test-compile (e.g., verify) to analyze both main and test classes once.

Copilot uses AI. Check for mistakes.
Comment on lines +103 to +141
<Class name="~.*Test.*"/>
<Bug pattern="EI_EXPOSE_REP"/>
</Match>
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="EI_EXPOSE_REP2"/>
</Match>
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="MS_EXPOSE_REP"/>
</Match>
<!-- Tests often have inner classes that don't need to be static -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may compare floats directly for exact expected values -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="FE_FLOATING_POINT_EQUALITY"/>
</Match>
<!-- Tests may use static fields that should be static (test fixtures) -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="SS_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may use date format patterns that SpotBugs considers suspicious -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="FS_BAD_DATE_FORMAT_FLAG_COMBO"/>
</Match>
<!-- Tests may intentionally test security manager methods -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
</Match>
<!-- Tests may create objects used only for side effects or assertions -->
<Match>
<Class name="~.*Test.*"/>
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

The class regex ~.*Test.* matches any class name containing Test, not just test classes (e.g., io.confluent.ksql.test.TestFrameworkException lives under src/main/java). This broad match can unintentionally suppress SpotBugs findings in non-test / main-source code. If the intent is to cover test classes and their inner classes, consider tightening the pattern to something like “ends with Test” plus optional inner class suffix (e.g., ~.*Test($|\$.*)).

Suggested change
<Class name="~.*Test.*"/>
<Bug pattern="EI_EXPOSE_REP"/>
</Match>
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="EI_EXPOSE_REP2"/>
</Match>
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="MS_EXPOSE_REP"/>
</Match>
<!-- Tests often have inner classes that don't need to be static -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may compare floats directly for exact expected values -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="FE_FLOATING_POINT_EQUALITY"/>
</Match>
<!-- Tests may use static fields that should be static (test fixtures) -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="SS_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may use date format patterns that SpotBugs considers suspicious -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="FS_BAD_DATE_FORMAT_FLAG_COMBO"/>
</Match>
<!-- Tests may intentionally test security manager methods -->
<Match>
<Class name="~.*Test.*"/>
<Bug pattern="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
</Match>
<!-- Tests may create objects used only for side effects or assertions -->
<Match>
<Class name="~.*Test.*"/>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="EI_EXPOSE_REP"/>
</Match>
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="EI_EXPOSE_REP2"/>
</Match>
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="MS_EXPOSE_REP"/>
</Match>
<!-- Tests often have inner classes that don't need to be static -->
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may compare floats directly for exact expected values -->
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="FE_FLOATING_POINT_EQUALITY"/>
</Match>
<!-- Tests may use static fields that should be static (test fixtures) -->
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="SS_SHOULD_BE_STATIC"/>
</Match>
<!-- Tests may use date format patterns that SpotBugs considers suspicious -->
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="FS_BAD_DATE_FORMAT_FLAG_COMBO"/>
</Match>
<!-- Tests may intentionally test security manager methods -->
<Match>
<Class name="~.*Test($|\$.*)"/>
<Bug pattern="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
</Match>
<!-- Tests may create objects used only for side effects or assertions -->
<Match>
<Class name="~.*Test($|\$.*)"/>

Copilot uses AI. Check for mistakes.
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