Discussing the checkstyle, code coverage and code analysis plugins in new build process.

[Tibor17]

After #511 we may now discuss addons of Maven plugins which may automate the development process we are doing manually like checking the code style, checking code issues and test coverage.

@avandeursen mentioned his link in #511 which forced me to start this discussion.
http://avandeursen.wordpress.com/2012/12/21/line-coverage-lessons-from-junit

After little searching on google:

• checkstyle plugins
  http://maven.apache.org/plugins/maven-checkstyle-plugin/index.html
• code analysis plugins FindBug, PMD plugin maven 1x?
• code coverage plugins JaCoCo, EMMA

EMMA does not integrate with JUnit, and there might be more other plugins in these categories.

How much can we benefit from reports produced by these plugins ?
How can we integrate these reports in CloudBees/Jenkins ?

[dsaff]

In my experience, adding automatic code coverage analysis to a project with already fairly high coverage often is an expensive cost/benefit choice, especially when you bring on board the cost of forward maintenance.

Checkstyle and code analysis could be interesting. The first step would probably be to choose one of these analyses, run it on the code, and come to an agreement that fixing the problems identified would be an improvement.

[Tibor17]

I agree that checkstyle and code analysis might be applicable in our environment.

I do not have spare time to make an analysis in the near future. Any volunteers ?

[Stephan202]

I'm personally a fan of Sonar, but do not have a public facing instance. Perhaps there are third parties which provide such service; never looked into that.

Anyway, I guess I could run a Sonar analysis against the current HEAD and see what comes up; might do that this weekend.

[Tibor17]

@Stephan202
We are getting Maven ready.
Finally, the solution needs to be intergated through a plugin in to our pom.xml.
Thx.

[Stephan202]

@Tibor17: Understood. Sonar basically "bundles" a bunch of analysis tools, among which the ones you mentioned, and can be run as a Maven plugin. Ideally it is automatically invoked by a CI system, either after each commit or according to some schedule. At my work we use Jenkins for this, the "glue" is provided by the Sonar plugin. (That plugin is not strictly necessary, since one could just invoke mvn sonar:sonar, but there are some benefits.) Of course, as I stated in my previous comment, all this does require a Sonar server running somewhere, and I'm not sure how/where to host that.

The alternative is to add some of the analysis tools you mentioned as separate maven plugins. These can then generate reports that can be included in the Maven site. This approach doesn't require any additional services, but doesn't provide the benefit of a collaborative environment catered towards code review and automatic build-up of a code quality history.

Anyway, this was but one suggestion, let's explore all possibilities :)

[Tibor17]

@Stephan202
The CloudBees can execute Windows batch command, shell or Ant in pre-build.
Could this feature be used in order to start the Sonar server, and kill it in post-build?

[Tibor17]

@Stephan202
It would be great if you could provide us with a snapshot of Sonar report so that we can compare with other tools.
Can we see statistics befre a build and after the build in one report? The point is to see how much an examined pull request would affect the code line by issues, bugs, broken code style.

[avandeursen]

For example Sonar reports of open source projects, see the Nemo site.

Cloudbees offers a Sonar service for open source systems. I assume @stephenc can provide more information.

[avandeursen]

For code coverage of JUnit itself, Cobertura is better suited than Emma/EclEmma/JaCoCo, since the latter does not report coverage of code raising exceptions (which JUnit uses a lot).

Cobertura runs out of the box with maven -- it just follows the Surefire settings to execute the right test cases. I just included the cobertura configuration in my pom.xml so that html reports are generated -- I can put that in a pull request if there is interest.

[marcphilipp]

Code coverage would be very helpful, especially for our test source folder. Since we're manually maintaing an AllTests suite, we would be able to find tests missing from the list there. Cobertura sounds good, especially since the free FOSS plan of CloudBees includes the Cobertura plugin. 👍

Regarding Sonar: To me it looks like CloudBees does not provide a free instance to FOSS projects anymore. However, we can ask the guys at SonarSource to include JUnit on their Nemo site once we have sensible checks up and running.

Regarding Checkstyle: IMHO the default rules are not very helpful, so we would have to discuss what checks and how to set limits etc. 👎

Findbugs and PMD both have reasonable and helpful defaults, I'm open to try them out and see what they find.

[Tibor17]

@marcphilipp
The simplest would be to see the FindBug analysis.
I did and started webstart and can already report fixes which are real to accept. Other like ExpectedException should be ExpectedExceptionRule which is unchangable, thus not internal/experimental, unfortunately cannot be taken back.

http://findbugs.cs.umd.edu/cloud/intellij.jnlp

Let's see how we can visualize findbug plugin report.

[marcphilipp]

Jenkins comes with a FindBugs plugin that is included in the free plan so we could simply use that as a start.

[stephenc]

Violations plugin is the one to use IMHO as it supports all the static
analysis tools, so you get a consolidated view.

Just a question of adding the appropriate configurations to the maven
pom.xml to apply the rules you want to enforce. Once #614 is merged and we
have the base jobs set up I will take a look at getting some of the static
analysis tools enabled via maven.

On 18 January 2013 08:40, Marc Philipp [email protected] wrote:

Jenkins comes with a FindBugs pluginhttps://wiki.jenkins-ci.org/display/JENKINS/FindBugs+Pluginthat is included
in the free planhttps://cloudbees.zendesk.com/entries/496694-what-are-dev-cloud-essentials-pluginsso we could simply use that as a start.

—
Reply to this email directly or view it on GitHubhttps://github.com/KentBeck/junit/issues/603#issuecomment-12412267.

[marcphilipp]

Sounds good, thanks!

[stephenc]

I've had a look at the default findbugs checks. Currently there are about 30 issues, perhaps 50% are real, the other 50% are things like:

• findbugs thinks you should be using a method that is avail in JRE 1.6 but not JRE 1.5, but JUnit only requires 1.5
• findbugs doesn't like some of the classnames picked in the JUnit API which are necessary to maintain backwards compat
• etc

So, all in all not a bad code base. Should be possible to get that down to 0 (fix the half that are bugs, add exclusion flags to the bits findbugs should ignore)

For Checkstyle, it will be a case of mapping the existing codestyle to a set of checkstyle rules... that can be trickier... once that is set up a simple reformat commit using the formatting rules in an IDE will get that down to 0.

Code coverage, reported by cobertura, is running at about 85% line and 82% branch. Here are the worst
org.junit.matchers 9% (<-- noddy code, not something worth prioritising tests for)
junit.runner 46% (<-- legacy code, could be forgiven for lower coverage)
org.junit.internal.runners 74% (<-- MethodRoadie and ClassRoadie to blame here)
org.junit.internal.matchers 75% (<-- missing tests for one class)
junit.framework 75% (<-- legacy code, could be forgiven for lower coverage)
junit.textui 76% (<-- legacy code, could be forgiven for lower coverage)
junit.extensions 82% (<-- legacy code, could be forgiven for lower coverage)
org.junit.experimental.max 84% (<-- experimental code, could be forgiven for lower coverage)

All in all, coverage is not bad at all.

[Tibor17]

@stephenc
Yeah it is not that bad code, but the point is that the commiter may start a build and see how much the statistics have changed.

I would like to have findbugs:gui in develoment profile not used in build machine, nothing but by developers.
This way the developers will see a candidate bug even before they start pushing a pull.
This will save commiter's time. Why you should handle all, if the tools can help the devs to check their code automatically in their private.
The only thing is to write some dev rules and recommendations.

[Tibor17]

btw, the findbugs 2.0.1 was wrong in enums saying that the field has to be final.
So do not accept that tool orthodox.

So other like findbugs requires transient on static serialVersionUID. So there are also findbugs bugs.

In 3.x code, most of the time a File passed by a method parameter was deleted, which findbugs does not like. This I do not like either, the javadoc does not say that, and anyway we cannot take it back. Other things in 3.x like closing Properties, which is missing.

Realistically what I can fix in main code which does not change the behavior but would look nicer to the plugin

• the boolean varialbe to AtomicBoolean,
• public-static-field to public-static-final
• ambiguous invocation of inherited method
• making inner class static
• internationalization
• #equals in ParameterizedAssertionError
• \n to %n in formated output
• other stupid issues except for deprecated junit.framework.Assert

[Tibor17]

@Stephan202
Regarding the checkstyle, the percentages do not harm the code itself. The problem is that we now have changed from one style to another.
So such checkstyle plugin has to be configured, but realistically it is a lot of work here to improve the style.

[marcphilipp]

Please let's not start discussing potential "fixes" right away. First, everyone should have a chance to look at the warnings generated by Findbugs and other tools in a central location, e.g. our Jenkins instance at CloudBees. I would strongly recommend starting with code coverage as soon as #614 is merged. No need to hurry, though. ;-)

[Tibor17]

@marcphilipp
We have to know in advance how we can improve dev processes and how to save our time.
Also we should know in advance where we can get even before we run the path.

[marcphilipp]

To some extent, yes, but not in such a detail. I'd rather try some tool for a while and then reflect whether it has been useful.

[Tibor17]

@marcphilipp
sure, anyway i do not have much spare time to pull it now anyway.
We need to check out one more tool like findbugs to see the diff and magnitude of bugs.

I have to pull abstractions on ParallelComputer asap, and the OSGi IT tests after #614. :)

[marcphilipp]

@Tibor17 Please do not feel obligated to submit pull requests for all these tools. It was great that you started this discussion in the first place, thanks!

[stephenc]

Actually there is only one or two small changes needed to support most of
the tools.

Findbugs is supported out f the box with the #614 Pom

Cobertura just needs one tweak for Jenkins to be able to pick up the results

Checkstyle is the most work in that you are most likely not using exactly
any of the in-built style rules soa rule needs developing (or you decide to
change to one of the built in rule sets (please don't pick the Maven rule
set... It's a waste of verticals IMHO)

I'll probably do a pull request once I get the ok from @marcphilipp to go
tweaking Jenkins (some issue with mail ailing to deliver to the email
address registered with CloudBees)

On Friday, 18 January 2013, Marc Philipp wrote:

@Tibor17 https://github.com/Tibor17 Please do not feel obligated to
submit pull requests for all these tools. It was great that you started
this discussion in the first place, thanks!

—
Reply to this email directly or view it on GitHubhttps://github.com/KentBeck/junit/issues/603#issuecomment-12426365.

[Tibor17]

@marcphilipp It was not my ambition to submit pull for these tools meanwhile the discussion is in progress. The most important in past was to persuade you and @dsaff to accept Maven and let you try to deploy.

I would like to request for two things:

• a development profile in pom.xml for findbugs:gui. The devs should have chance to fix own issues before the pull.
• distinguish between Maven project documentation for devs and maintainers. The one in 4.12 release notes is for developers with local use as simple as possible. The pom.xml use to be part of your IDE project. So the Git devs are not going to release versions, therefore they do not need to read building-junit.txtdoc even if it will be in wiki.

[dsaff]

From a prioritization standpoint, if someone has cycles to work on automating code correctness and cleanliness tasks, I think that getting formatting settings for Eclipse and IDEA checked in is the first order of business, and then we move on to considering some of these.

Your thoughts?

[Tibor17]

@dsaff For IDEA it would only mean to install FindBugs-IDEA plugin, and nothing special just to press a button to start code analysis. Settings -> Plugins -> Available, then search FindBugs-IDEA, right click, download and install.
http://plugins.jetbrains.com/plugin?pluginId=3847

Custom project files is perhaps not good idea to submit.
I am using maven-idea-plugin in pom.xml to auto-generate project files, but not able to configure auto-installation of any non-default IDEA plugins. Has to be done manually.
http://code.google.com/p/maven-idea-plugin/

[stephenc]

Why are you using that plugin. Since IDEA 8 or 9 there has been no
requirement to se that plugin at all.

On Wednesday, 23 January 2013, Tibor Digana wrote:

@dsaff https://github.com/dsaff For IDEA it would only mean to install
FindBugs-IDEA plugin, and nothing special just to press a button to start
code analysis. Settings -> Plugins -> Available, then search FindBugs-IDEA,
right click, download and install.
http://plugins.jetbrains.com/plugin?pluginId=3847

Custom project files is perhaps not good idea to submit.
I am using maven-idea-plugin in pom.xml to auto-generate project files,
but winot able to configure auto-installation of any non-default IDEA
plugins. Has to be done manually.
http://code.google.com/p/maven-idea-plugin/

—
Reply to this email directly or view it on GitHubhttps://github.com/KentBeck/junit/issues/603#issuecomment-12577582.

[stephenc]

Yes I agree that the code formatting style configurations for each IDE
should be made available. Less clear that they need to go in SCM as these
files usually need to be installed in the IDE separate from the project (ie
add as a global scheme). If you want them in SCM that's fine, though from
my PoV an attachment to a wiki page on code style would work just as well.

On Tuesday, 22 January 2013, David Saff wrote:

From a prioritization standpoint, if someone has cycles to work on
automating code correctness and cleanliness tasks, I think that getting
formatting settings for Eclipse and IDEA checked in is the first order of
business, and then we move on to considering some of these.

Your thoughts?

—
Reply to this email directly or view it on GitHubhttps://github.com/KentBeck/junit/issues/603#issuecomment-12567593.

[Tibor17]

@dsaff
@stephenc
In IDEA you can export project settings via File -> Export Settings....
It will be settings.jar by default.
http://www.jetbrains.com/idea/webhelp/exporting-and-importing-settings.html
I propose to move it to src/main/config/idea and similar for src/main/config/eclipse instead of creating ugly dirs in root.
@dsaff I will export that file according to CODING_STYLE and give it to FTP for your downloads.

[marcphilipp]

The default IDE for JUnit is and has always been Eclipse. Eclipse
allows to set project-specific formatter settings. Those are written
into a .settings folder directly beneath the project folder (in our
case the repo root). This way, when someone imports import the Eclipse
project into his workspace those settings will automatically be
applied.

This would help us a lot when reviewing pull requests, especially if
we configure formatting as an automatic "save action" in Eclipse.
Thus, I would strongly vote for adding these files to the Git repo.

AFAIK IntelliJ can import Eclipse projects, right? Thus, providing
"native" settings for it seems secondary to me.

[Tibor17]

yes, it can import eclipse.
it is better to split these activities to separate issues and pulls

[marcphilipp]

You're right. There's already an issue for it: #426.

[mdesir8]

@marcphilipp In the interest of cleaning up the issues, I think this one could be closed.