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: #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 @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.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

[jbduncan]

I'm happy to close this issue, actually. I've learned since opening it that a judicious use of static analysis tools is the best kind of use.

I still quite like error-prone though. ;)

And if SonarQube is ever deemed useful in the future, a new issue could easily be opened there and then.