Findbugs is reporting false positive bugs in test code

[ArnaudLec]

Issue Description

Since I migrated my SonarQube server to 9.9 from 9.7 and the Sonar-FindBugs plugin (from 4.2.2) my test code (located in standard location src/test/java) is reporting a lot of bugs from some FindBugs rules :

• fb-contrib:NAB_NEEDLESS_BOOLEAN_CONSTANT_CONVERSION
• fb-contrib:BED_BOGUS_EXCEPTION_DECLARATION
• fb-contrib:WI_MANUALLY_ALLOCATING_AN_AUTOWIRED_BEAN
• fb-contrib:WOC_WRITE_ONLY_COLLECTION_FIELD
• fb-contrib:PSC_PRESIZE_COLLECTIONS
• [probably others but I stopped after 30 bugs]...

Environment

Component	Version
SonarQube	9.9 (build 65466 / LTS)
Sonar-FindBugs	4.2.3
Maven	3.8.7
Java	11

Code (If needed)

public class FbFpTest {
  @Test
  void fp() {
     List<Boolean> list = new ArrayList<>();
     list.remove(true); // fb-contrib:NAB_NEEDLESS_BOOLEAN_CONSTANT_CONVERSION reported here
  }
}

[gtoison]

Hello, due to some limitations in the SonarQube API the compiled .class files corresponding to unit tests were not analyzed.
Starting with version 4.2.3 of the plugin AND when running on SonarQube 9.8+ the unit tests are now analyzed.

So these are analyzed by SpotBugs for the first time, does it make sense to you?

[ArnaudLec]

It makes sense to analyze the test code but test code should be readable first before being perfect.

In my opinion, it doesn't make sense to report a collection presize on test code, neither giving a native boolean to an Object Boolean method.

Like I have bugs reported on my spring beans because I'm instantiating the bean under test with new. The right way to fix this is not to create an integration test instead of an unit test to fix this bug.

And I don't want to put SuppressWarnings everywhere in my test code to ignore FindBugs rules, that doesn't seem sane to me.

Rules should be applied where it makes sense and not everywhere.

[gtoison]

I agree that the rules should be different for unit tests compared to normal code. In fact I have completed excluded unit tests from the SonarQube analysis on some projects. The SpotBugs plugin should not report issues on excluded sources.
However some users still want to see issues reported.
Would it be ok for you to exclude unit tests completely?

[ArnaudLec]

No because I have some rules that matters only to testing like java:S3415 or java:S5777 and I wan't those problems to be reported.
You can access them through the url /coding_rules?languages=java&repositories=java&tags=tests if you want a list of native rules dedicated to test code.

[gtoison]

I see what you mean but that would mean a different SonarQube profile for tests, and this does not exist, profiles are per-project.

Maybe you could setup a SpotBugs exclude file?
You need to use the sonar.findbugs.excludesFilters analysis option to tell the plugin where that filter file is and then you can exclude bug codes and/or classes as described here: https://stackoverflow.com/questions/756523/findbugs-filter-file-for-ignoring-junit-tests

[gtoison]

I'll add an option to exclude tests from the SpotBugs analysis, see: #720

[ArnaudLec]

Sorry, I didn't had time to test the exclude filter file or to answer your previous message.
The problem I had with the exclude file is that I'm managing a centralized SonarQube instance in a software company with multiple products.
I'm not working directly on those products so having a per product base file to maintain for each team can be quite challenging.
Having a failing Quality Gate is also a blocker for products release.

The option to exclude tests analysis is a nice fix to return to the same state it was before which on my side is nice and will simplify the migration for many I think.

In the long term, I agree with you that scanning tests is a good thing to have, if the rules makes sense for tests, like :

• JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS
• DM_EXIT

but some don't make sense at all :

• WI_MANUALLY_ALLOCATING_AN_AUTOWIRED_BEAN > Unit testing spring beans is a good thing to have
• FCBL_FIELD_COULD_BE_LOCAL > some mocks in tests are reported when the mock is instantiated in a @BeforeEach method because we don't want to overload the real test with a lot of prepare stuff
• SIC_INNER_SHOULD_BE_STATIC > Junit5 @Nested inner test classes are reported when we can't use this annotation with static inner classes

The long term solution should be to have a default sensible set of rules for the main code and another one for the test code.

[gtoison]

Thanks for the feedback, some of these rules were written a while ago and did not have Spring or JUnit5 in mind.

Let me know if you are able to test with the last changes (run mvn package to build the plugin), otherwise I'll release that soon and hopefully this will fix the problem

[ArnaudLec]

Just tested the flag by disabling it globally it and my simplest project just went back to a passed quality gate so it works great.
Now each product will be able to enable it if they want.

No need to rush a release if I'm the only one impacted, i will gladly run with my local built plugin in the meantime waiting for an official release.

[ArnaudLec]

All our products analysis are back to the way they were before.
I'm closing this.

Thank you for the quick response and the added switch.