search

junit-team / junit4

A programmer-oriented testing framework for Java.
https://junit.org/junit4
Eclipse Public License 1.0
8.52k stars 3.26k forks source link

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

Open Tibor17 opened 11 years ago

Tibor17 commented 11 years ago

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:

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

Stephan202 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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. :+1:

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. :-1:

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

Tibor17 commented 11 years ago

@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 commented 11 years ago

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

stephenc commented 11 years ago

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 notifications@github.com 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 commented 11 years ago

Sounds good, thanks!

stephenc commented 11 years ago

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:

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 commented 11 years ago

@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 commented 11 years ago

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

Tibor17 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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

dsaff commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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

marcphilipp commented 11 years ago

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