Static Code Analysis Checks

[svilenvul]

I would like to continue a discussion that started here : Add Analysis Tool for New Bindings

I will give some highlights :) :

svilenvul commented on 4 Oct • edited
I would like to start a discussion about the integration of an analysis tool for PRs in the openhab2-addons repo. This should speed up the review process for new bindings.

maggu2810 commented on 4 Oct
..
For my projects I use maven plugins for Checkstyle, FindBugs, PMD (you stated them above) etc.
The Jenkins instance checks the Quality Gates (execute all the plugins and continues on error - generate the report only) and Jenkins will fail if the report breaks the limits.

I also use a pre commit Maven profile that fails if an error occurred and use it before doing a commit (myself).

MHerbst commented on 4 Oct
First of all I like the idea of automatically checking as much as possible. I also would prefer to use only Checkstyle, Findbugs and PMD. We are using them in our company very similar to @maggu2810. And especially the Findbugs checks are really helpful.
...

kaikreuzer commented on 5 Oct
I agree with @maggu2810 and @MHerbst. It would be ideal if all tools could be used by the developers locally before even creating a PR. A Maven profile should enable those checks and new PRs would be build on Jenkins with this profile.

What should be tried is to make such a mechanism easily extensible and configurable; we e.g. might e.g. have a different set of checks for bindings than for UIs. And the guidelines might evolve over time. Also note that almost everything is applicable to ESH as well, so openHAB2 might not be the right place for such a feature (at least not for most of it).

There was a discussion for ESH a long time ago about such automatic checks and I have seen Maven plugins that used Groovy to do more advanced checks - this might be a good solution for all non-Java files.

Last but not least, it would be nice if a kind of summary/report could be created; I expect that we might have warnings (besides errors), which would NOT stop the build, but which we should nonetheless get to the attention of the developer - if those warnings only appear somewhere in the Maven build log, I doubt that anyone will ever see them.

[svilenvul]

I have built a prototype following some of the things discussed above, a PR will be opened shortly.

[kaikreuzer]

Great, looking forward to it!

[svilenvul]

I have created a simple online poll, where you can vote for the automated code checks that you want to be added to the Static Code Analysis Tool. Please share your opinion, you have only to enter a name to vote.

The initial suggestions listed there are from the ESH Code Guidelines, feel free to add your own suggestions at the last row of the poll.

A link to the poll is - https://poll.ly/#/LApbZzMZ.

You can find inspiration in some of the predefined rules for:

• PMD;
• Checkstyle;
• FindBugs.

[kaikreuzer]

The initial suggestions listed there are from the ESH Code Guidelines.

These are the agreed rules for the project, so if technically feasible ALL of them need to be checked by the tool - because this is one of the main manual jobs currently done during a review.

If anybody says that any of these is not important to check, we should discuss to adapt the guidelines, not discuss adapting the tooling!

The potential interesting rules from PMD/Checkstyle/FindBugs are imho too many to add them to the poll in any meaningful way.

In general I would weigh them as

• important for most of FindBugs
• partially important (->warning?) for many from PMD
• hardly important (with the exception of the guidelines above) for Checkstyle

I think one has to "play" with them a bit on the current code base. My suggestion is that you come up with a rule set that highlights real bugs (and code that is against the guidelines) and which might show a "few" warnings of things that you feel should be addressed.
We could then incrementally add further rules, once existing stuff is (mostly) addressed and we can also "upgrade" certain rules that we think are important to be listed in the guidelines.

[svilenvul]

PR is closed.

[svilenvul]

As the tool is now moved to an its own repo and used in the openHAB builds I would like to reopen this discussion.

In the last two weeks several PRs have been merged that fix issues found by the tool, that are considered to be build breaking.

I have created a branch that includes the necessary changes to add the tool to the ESH build.

The build is still not green, but only a few issues in the code base still remain unfixed. I understood that ESH is a fast moving target :).

Should I open a PR for that ?

[maggu2810]

Hm, can we fix the remaining issues in front of?
Or could we whitelist the known issues but enable the tool usage, so no new ones are added?

[kaikreuzer]

can we fix the remaining issues in front of?

I understood that this is @svilenvul's proposal of opening a PR for it.
We will definitely only add the static analysis once the code is in a state that it passes the checks.
I will meanwhile go and create a build-time CQ, so that we are allowed to use it.

[svilenvul]

can we fix the remaining issues in front of?

Yes. As soon as we have a green light for the CQ, I could fix the high priority issues until the latest commit in the repository at that moment. If someone else wants to help, he is always welcome of course.

[maggu2810]

As soon as we have a green light for the CQ, I could fix the high priority issues

I don't think we need to wait for a CQ for the tool in front of fixing high priority issues the tool (not integrated into the official build) will complain about.

[kaikreuzer]

FTR: I have created CQ 13980.