GH-790 quality plugin

[barthanssens]

GitHub issue resolved: #790

This may partially solve the issue since I did not touch the Jenkins config, though there seems to be a plugin that can use the XML output from these plugins (https://wiki.jenkins.io/display/JENKINS/Warnings+Next+Generation+Plugin) to generate nicer looking reports.

See also https://spotbugs.readthedocs.io/en/latest/maven.html and https://spotbugs.github.io/

SpotBugs will generate reports in xml format, that can be loaded in some IDEs (apparently from within Eclipse/Intellij), or by invoking the standalone GUI via mvn spotbugs:gui and opening the files manually.

Note that SpotBugs is a bytecode analyzer, which means that one has to compile the sources first, and only then the plugin can be used to perform an analysis (mvn spotbugs:spotbugs)

Apparently there might also be some false positives when "final" fields are involved, but it appears to provide some interesting feedback, though I haven't compared the results with e.g. PMD or other tools.

[hmottestad]

I'm fairly sceptical of static code analyzers because they tend to give a lot of false positives per true positive. How many false positives does this currently give? And how easy is it to mark something as a false positive?

[barthanssens]

In attachment an example from one of the modules (as XML and as HTML).

I think it does a rather nice job for what it is designed for (it might have caught an issue #1666), though it will need manual analysis and/or tweaking, so I guess it's more a run-once-in-a-while tool (unlike the formatting plugin)

E.g using a "+" to concatenate a string instead of using StringBuilder is slower but might be more readable, do we want to use the Nullable annotation to document methods that may return null, do we really need to provide a toString for each and every class, and various other nitty-gritty "best" practices

And it obviously won't catch potential big architectural issues.

spotbugs.zip

[hmottestad]

I had a look, I think I prefer using the static code analyzer in IntelliJ. It is a lot easier to understand the issue when you can click yourself through to the code. Does findbugs find more bugs because it uses bytecode analysis compared to Intellij?

Of those ~300 warnings. How many do you reckon are actually bugs that have gone undetected until now?

[barthanssens]

Well, I haven't checked all the reports yet, but in terms of actual bugs I guess it will only be a few real ones. Most of the others will be suggestions for improvement, and occasionally (a small dozen ?) it seems worth to actually implement the suggestions or look into the details.

I assume the low-hanging fruit was already picked (since you are using IntelliJ and I've done
some Netbeans Inspect and Transform, and maybe Jeen or someone else has a similar tool in Eclipse...)

So no strong feelings about whether or not using this (or another) tool in Jenkins :-)

[abrokenjester]

@barthanssens thanks for looking into this, and apologies for not reviewing sooner - ok if I get back to this after we've done the patch release this weekend, and perhaps also have a clearer idea of the role of Jenkins vs the Github actions that we've been experimenting with?

[barthanssens]

No rush :-)

[abrokenjester]

FWIW I'd be happy to merge this in as it doesn't impact our default flow - the plugin is not tied to the build/test life cycle AFAICT, you have to manually invoke it. So if you don't want to use it, you don't have to.

[barthanssens]

I probably have to file a CQ 'works-with' request.

FWIW, I'm analyzing the reports on the dev branch, nothing spectacular (like discouraging string concatenation, some hints on possible NPEs and suggestion to add Nullable annotations and minor thingies), though there are a few good hints to improve some pieces of code

[abrokenjester]

Let's not merge this before the 3.1 release then, so that the CQ doesn't block the release.

[barthanssens]

Still have to file a few CQs for the additional rulesets, so not merging it yet