Junit5: Consider using static analysis and formal verification tools

[jbduncan]

Overview

Currently, AFAICT, no static analysis or formal verification tools are being used to catch bugs in the JUnit 5 code base. IMO it would be a sensible thing to do to start using a combination of various tools now, so that when we reach GA and proceed beyond GA, we are less likely to be inundated by preventable bug reports and/or suffer from potential security issues.

I'd personally suggest a combination of the following tools, but I'm more than happy to discuss the merits of the tools listed and to discuss other tools which aren't listed or that I may have not thought of:

• error-prone
• SpotBugs or FindBugs
• SpotBugs/FindBugs plugins:
  
  
  
  • fb-contrib
  
  
  • find-sec-bugs
  
  
• PMD
• Checker Framework (specifically for nullness formal verification)

Deliverables

• [ ] A number of agreed-upon static analysis and/or formal verification tools are adopted.

[sormuras]

Would you mind filing a PR with the tool(s) of your choice?

[jbduncan]

No, I wouldn't mind at all!

I'll see if I can find the time later today or this week to submit a PR.

[alixwar]

SonarQube exists in a cloud version, free for use by Open Source projects: https://about.sonarcloud.io/

SonarQube includes support for PMD, FindBugs and more

[jbduncan]

@alixwar We could indeed use SonarQube in the future, but I'd feel leery about us primarily depending on it considering that Gradle already comes with first- and third-party support for SpotBugs (FindBugs' current successor), PMD, and other great tools like error-prone.

[jbduncan]

...and also considering that SonarQube's cloud version is yet another thing which we don't have a lot of control over.

[alixwar]

@jbduncan Maybe one solution does not exclude the other? My thinking is that bootstrapping SonarQube (which apparantly now supports SpotBugs as well) is probably very quick and easy (based on my experience setting it up locally). It will have no impact on the code base more than a properties file with some metadata so it can easily be removed if decided so.

[jbduncan]

You make a good point. I'm most likely dismissing SonarQube out of fear too quickly. :)

I personally have no experience with SonarQube, so it's not something I could setup unless I get a lot of free time and develop the inclination to work on it in the future, so I wouldn't mind at all if you wanted to have a stab at it yourself at some point. :)

[alixwar]

I personally have no experience with SonarQube, so it's not something I could setup unless I get a lot of free time and develop the inclination to work on it in the future, so I wouldn't mind at all if you wanted to have a stab at it yourself at some point. :)

Definitely. I can wire it up to a local installation and post some screenshots :)

[alixwar]

I have set it up locally. A few comments:

• I configured it the "Gradle way" which actually means changing the root build.gradle and adding the Gradle SonarQube plugin with config. This is not necessary, although it is the recommended way. The alternative is to use a separate properties file (sonar-project.properties) and analyze the project using an external command line tool (SonarQube Scanner). That way there is no "pollution" to the code base but it will require adding the command tool binary ("SonarQube scanner") to the CI environment and it may not yield as reliable results as the Gradle SonarQube plugin.
• I had to create empty folders that were missing in some modules (src/main or src/test) for the SonarQube gradle build to complete successfully.
• I don't have a Clover license locally so I could not report any coverage data. I have configured SonarQube to report from Clover but I cannot test it.
• ~I enabled all available rules which might not be what is configured in the cloud version~ Edit: Apparantly I forgot to start using the "All rules" profile... So this is with the default rules enabled.
• A good (essential imo) thing with SonarQube is that you have seamless integration with some IDEs (using the SonarLint plugin that exists for Eclipse and IntelliJ).

Some sceenshots (dashboard, list of issues and viewing the details of an issue):

[alixwar]

New screenshots with all rules applied:

[alixwar]

Some screenshots from IntelliJ:

[alixwar]

Here are the changes: https://github.com/junit-team/junit5/pull/1008

[alixwar]

@jbduncan @sormuras Anything else you want to know about SonarQube and its capabilities?

[sormuras]

Thanks for all your work @alixwar and @jbduncan in this section!

Given the amount of time left for RC3 (and higher) and especially for GA, static code analysis might not find it's way into this project's automated build process in the near future. As far as I can see from your manually executed runs and screenshots, there's no show-stopper found in the JUnit 5 code base that would prevent a release. Do you agree?

[alixwar]

There are some (9) violations concerning optionals being retrieved without a "Optional#isPresent()" check.
Example:

RepeatedTest repeatedTest = AnnotationUtils.findAnnotation(testMethod, RepeatedTest.class).get();

I'm not familiar with the code so I don't know whether this is a show stopper.

[sormuras]

IIRC, those un-guarded Optional.get() calls are all located in our test cases. Here, a failing .get() would result in a failing test. Which is okay for me.

[alixwar]

Then I'm not aware, at least, of a show stopping issue. But it is a good thing to have static code analysis set up ASAP to avoid building up debt

[jbduncan]

@alixwar I actually do have a question regarding SonarQube, thanks for asking.

In one of your IntelliJ screenshots above, it shows "violations" like "move left curly brace one line ahead" and "convert tabs to spaces". Although I personally agree with spaces, the JUnit team decided on tabs a long time ago, and they also decided on placing left curly braces as they currently are, so is there a way of telling SonarQube to not report cases like this?

[jbduncan]

@sormuras It's a pleasure! :)

[alixwar]

@jbduncan

For your first question, someone has already answered it in StackOverflow: https://stackoverflow.com/questions/23908058/can-i-edit-some-rules-in-sonarqube

The first step is to identify the rule keys of the rules that you want to change. In this case (see screenshot below):

• squid:LeftCurlyBraceEndLineCheck
• squid:S00105

It appears that none of the two rules are configurable so you need to disable them if you don't like them. There are some rules in SonarQube that conflict with each other (in the case of code conventions). As part of the normal setup procedure you first define a Quality Profile (rule container) and fine tune it according to your needs, which means enabling/disabling/configuring rules.

Screenshots (where to find the rule key) + check details of a rule (some are configurable, this one isn't):

[jbduncan]

@alixwar Great, thanks for your comprehensive response.

Am I right to think the that SonarQube comes with checks or rules that aren't in FindBugs, PMD and the other tools that SonarQube bundles? If yes, then I'd be even more convinced that it's a tool worth using.

[alixwar]

@jbduncan You're welcome.

Yes. The rules with the rule key prefix "squid" are rules that come from SonarQubes own Java rule set. Some of them will be very similar to rules coming from PMD, FindBugs etc. In this case their strategy is to deprecate the ones coming from the external plugin (https://blog.sonarsource.com/already-158-checkstyle-and-pmd-rules-deprecated-by-sonarqube-java-rules/).

Although posted by the creators themselves (so biased) there is some evidence that their own rule set can help detect issues where other static tools fail: https://blog.sonarsource.com/sonarqube-enters-the-security-realm-and-makes-a-good-first-showing/

Note that both posts are old and that a lot has happened since. They have spent some effort classifying violation severities in a consistent way, https://blog.sonarsource.com/we-are-adjusting-rules-severities/ and lately made improvements to data flow analysis (https://blog.sonarsource.com/sonarqube-6-4-in-screenshots/)

[jbduncan]

@alixwar Very cool, thanks for showing me those blogs!

Their mention of custom rules for security issues looks rather promising to me, so I'm indeed more convinced of SonarQube's usefulness.

Perhaps it could be combined with find-sec-bugs (mentioned as Find Security Bugs on the first blog), which is a tool I was already considering adding to JUnit 5. :)

[alixwar]

Find-sec-bugs is actually part of the "FindBugs/SpotBugs"-plugin in SonarQube: https://github.com/SonarQubeCommunity/sonar-findbugs

...uses SpotBugs, fb-contrib and Find Security Bugs to provide coding rules.

It seems as though this plugin is gradually becoming obsolete though (here they have tracked their conversion process): http://dist.sonarsource.com/reports/coverage/findbugs.html

[alixwar]

@jbduncan @sormuras This add-on might save some time for reviewing pull requests: https://docs.sonarqube.org/display/PLUG/GitHub+Plugin

[sormuras]

Indeed. Looks nice and clean.

[smoyer64]

Related to #358.

[pauldingemans]

In pull request #1008 an implementation of SonarQube was set up.

Finally the team decided not to merge it to master.

Does it also mean that this issue should be closed or should it be kept open waiting for an alternative?

[sbrannen]

Does it also mean that this issue should be closed or should it be kept open waiting for an alternative?

This issue will remain open for the time being. The JUnit 5 team is open to alternative solutions but primarily for simple solutions that provide real value to the team. The terms "simple" and "real" are of course left to interpretation by the team itself.