Drop Sonar integration

[jponge]

Sonar has gradually become useless in our workflows:

• Codecov is providing us with clear insights on tests coverage, including deltas on pull-requests, and
• RevAPI is enforcing checks and documentation of binary API incompatibilities, and
• the current Sonar configuration gives us mostly false positives (e.g. when we use volatile) or pesky code smells (e.g. using public classes and methods with... JUnit5).

I hereby propose to pull the plug. Thoughts?

/cc @cescoffier @heubeck

[heubeck]

didn't even recognize that it was there ;)

[cescoffier]

No problem from my side. We are not using it anymore.

[m-g-sonar]

Hey @jponge, I'm Michael, a java dev' at Sonar, part of the small team working on our in-house static analyzers. In particular, my little (sub-)team and I are working on the Java Analyzer itself.

Out of curiosity, I looked at your SonarCloud project page, and from what I can see, it seems that mainly two rules caused you a lot of pain.

• Rule S5786: JUnit5 test classes and methods should have default package visibility (2,797 issues marked as "won't fix" out of the 2,842 "won't fixed"... so ~98.5%)
• Rule S3077: Non-primitive fields should not be "volatile" (seems to be the main troublemaker here)

I consequently have a few 2-3 questions for you, if you don't mind taking the time to answer!

• The rule about unit test modifiers (S5786) is indeed extremely noisy for projects which just moved to junit5, or as still in mixed mode (and OK with it). It is probably a mistake to have it part of the default quality profile (we are going to discuss this internally). Did you try to disable the rule?

  • I know that for rules already part of a quality profile, it could be a pain. Maybe you tried... and decided to give up. Would the following feature would have helped you? Deactive rules in an extended quality profile

• The rule about volatile fields (S3077) currently have 2 tickets open on our side (SONARJAVA-3992 and SONARJAVA-4256). Do you believe that fixing these 2 issues would have prevented some (if not all) of the False Positives (FP) you suffered from? If there is something else (the rule is just wrong, or some cases are really badly supported), could I ask you to report it in our community forum with some more details? The Java Analyzer is open source and free, and we try our best to adjust based on feedback we get! Could you help us improve it?

• Of course, I'm not saying that you didn't face other FPs from other rules, and I'm very sorry for you. As of today, to improve the existing rules and the user experience, we highly rely on user feedback. We rarely have enough time to explore issues status on SonarCloud, for instance, on top of the fact that we don't have expertise on all projects out there. If some other rules than these 2 have been causing you problems, it would be HUGE if you could try to explain to us how and why, by again opening threads in our community. This would help a lot, and make other open source projects (including ours, at Sonar) benefit from your experience!

Thanks for your time! The java analyzer is being shipped with 600+ rules, I feel it's sad to see you leave because of a few painful and noisy rules.

Cheers,
Michael

[jponge]

Hi @m-g-sonar, and thanks for reaching out.

First of all and as mentioned here and in other places our decision is mainly driven by the adoption of codecov and revapi.
Both tools provide the type of QA we need right now.

Sonar was certainly valuable before, but it gradually became useless in our workflows.
I shall also mention that it significantly increased our time in CI, so removing it makes sense from a cloud resource consumption perspective.

The rule about unit test modifiers (...)

JUnit 5 has a stylistic preference for avoiding public declarations, but raising flags is quite questionable, and as you said it shall probably not be part of the default profile.

The rule about volatile fields (...)

From what I recall it was always about not-so-obvious cases for a static analysis tool (e.g., drain-loop patterns, threading assumptions the checker cannot know about, etc).

But basically what was always super annoying is that it would raise a flag "volatile access is not thread safe" (I grossly simplified) every time we'd use a volatile.

Of course, I'm not saying that you didn't face other FPs from other rules (...)

I get that and thanks again for reaching out.

On the other hand I'm not sure this codebase is the sweet spot for Sonar unless heavily tuned, and we've come down to relying on more narrow tools.

All the best!

-- Julien