Wednesday Feb 25, 2015

Why Checkstyle matters...

I’ve read something about ZeroMQ and its java clone recently and decided I should give it a try. JeroMQ is quite small project (same as ZeroMQ I guess), so I decided to checkout the sources directly, since that usually gives me last version of everything, including samples etc. When I opened the project in my IDE, I noticed some minor issues in the code, starting with some unnecessary properties, ending with missing null checks and invalid String comparisons. (You can find these in any project, so that was not so surprising).

Interesting part started when I fixed the issues locally and ran “mvn clean install” to verify my changes, build failed. No surprises there ;) but it failed on check style issues. That was something I did not expect. It turned out, that the project enforces defined set of rules. Not saying that I like the style chosen for that particular project (common guys, its Java, not C), I really liked the idea behind that decision.

Generally, it is far better when single project uses single code style – developer might need to adapt to that and it can take time, but for me, when I got past this period, code is much easier to read and understand, because I don’t need to think about that particular formatting, looking for potentially mistyped code blocks or parenthesis etc. Even when switching among projects with different code style, it is still better than having to adapt to various styles in single module or even class. (Yes, that happens. In any older project, which accepted contributions from lot of people).

After this reflection, I thought about applying this to one project, where I can do almost anything I want – Tyrus. Short version: I did it!

Slightly longer version: It was a pain. Really. And I don’t have by any chance that rich check style rule set as mentioned JeroMQ. To be honest, I thought that we should be fine, since we do often detailed code reviews, but you can always miss something which is not really well formatted.. Nice fact about Checkstyle is, that there are no compromises. You can exclude some rules for some class or even some lines of that class, but .. why? It would add unnecessary housekeeping when modifying the code and it could be accidentally disabled by just refactoring the class name. So I went for “zero tolerance” for defined rules.

The problem is, that there is no simple tool which would fix all check style issues. It kind of makes sense, since lots of them are not easily fixable automatically. Anyway, I found some recipe which involved Eclipse plugin and generating Eclipse formatter from checkstyle.xml. That worked somehow and the formatter can be exported and included to other IDEs (I personally use IntelliJ). There are some issues. One of caveats was max line length. The formatter was able to “clean” these warnings from the build, but sometimes produced code like:

1
2
3
4
5
6
7
8
9
10
if (wseMaxSessionsAnnotation != null) {
TyrusServerEndpointConfig.Builder builder = TyrusServerEndpointConfig.Builder.create(annotatedClass,
wseAnnotation
.value()).
encoders(
encoderClasses)
.decoders(decoderClasses)
.subprotocols(
Arrays.asList(
subProtocols));

Which is far from acceptable :). Long story short, I ended up with defining own formatting rules, setting them in IntelliJ code style, reformatted and then manually checked and fixed issues in all changed source files. It was originally ~500 files, it shrunk a little after my review, since I removed all non-java files from that change. You can see the commit at github (don’t worry if it takes some time to load, it is a bigger one).

Anyway, I’m still glad I did that. Now I know that once the build or pull request passes, it has the code already formatted in a way, that I don’t need to check it (that much). If you are interested, there is checkstyle-build.xml used for build time validation, and the configuration in pom.xml.

What can be better? IntelliJ and I guess even other IDEs could be more accessible for checkstyle configuration. Meaning that formatter/code style/… should have better support for importing checkstyle config file + it should be able to actually use it well. The latter statement is almost true (for some edgecases, which cannot be easily solved I guess), so the first one is more important. Since I know that the settings are already there and its only about correct values of some properties, I cannot see it is not already there.

That’s it for now ;) Hopefully you’ll feel inspired and try to at least run the checkstyle configured to your code style and see whether you can incorporate it easily in your build. If you have any suggestions or comments about this topic, please share them with me, especially if I left out something usable for Tyrus or other projects. Thanks!

Wednesday Feb 23, 2011

Code coverage using Cobertura

We were requested to investigate and implement code coverage measurement and report generation to our project - Jersey. Nice opportunity to get into new thing and have a motivation to finish (since this was a "must do" type of request..).

Alright, let's get into it. At the begging, we had to decide which tool we are going to use - Cobertura [1] won, one of main arguments are nicer reports, active development and price (it's free).

There is a nice set of scripts, which can instrument classes/jars, collect & merge generated files and finally create report. This can be used in environment, where you have total control of your classpath and can easily run tests against it. Well, Jersey uses maven, so this is little more difficult, but cobertura does have maven support, so lets try it..

Maven support works perfectly when you have tests in each module and nowhere else, otherwise it is probable you'll run into similar issues as I did.. To be more descriptive: let's say, I have module A (which has some tests) and module B, which depends on A (and also has some tests). By default, code coverage results from B will contain only classes from B, which is not correct, because we want to have complete results. I hit this in Jersey, for example substitute A with "jersey-core" and B with "jersey-tests" and you are there. Solution is not that simple, because you have to somehow force B to use instrumented version of A.. which is not very straightforward using maven..

Solution:

You might find or think of few solutions, I like the one described here [2].

Main pom (profile which enables building&local deploying cobertura classified artifacts: 

        <profile>
            <id>cobertura</id>
            <activation>
                <property>
                    <name>cobertura</name>
                </property>
            </activation>
            <dependencies>
                <dependency>
                    <groupId>net.sourceforge.cobertura</groupId>
                    <artifactId>cobertura</artifactId>
                    <optional>true</optional>
                    <version>1.9.4.1</version>
                </dependency>
            </dependencies>
            <build>
                <plugins>
                    <plugin>
                        <groupId>org.codehaus.mojo</groupId>
                        <artifactId>cobertura-maven-plugin</artifactId>
                        <executions>
                            <execution>
                                <id>cobertura-instrument</id>
                                <phase>pre-integration-test</phase>
                                <goals>
                                    <goal>instrument</goal>
                                </goals>
                            </execution>
                        </executions>
                    </plugin>

                    <plugin>
                        <groupId>org.apache.maven.plugins</groupId>
                        <artifactId>maven-jar-plugin</artifactId>
                        <executions>
                            <execution>
                                <id>cobertura-jar</id>
                                <phase>post-integration-test</phase>
                                <goals>
                                    <goal>jar</goal>
                                </goals>
                                <configuration>
                                    <classifier>cobertura</classifier>
                                    <classesDirectory>${basedir}/target/generated-classes/cobertura</classesDirectory>
                                </configuration>
                            </execution>
                        </executions>
                    </plugin>

                    <plugin>
                        <groupId>org.apache.maven.plugins</groupId>
                        <artifactId>maven-install-plugin</artifactId>
                        <executions>
                            <execution>
                                <id>cobertura-install</id>
                                <phase>install</phase>
                                <goals>
                                    <goal>install</goal>
                                </goals>
                                <configuration>
                                    <classifier>cobertura</classifier>
                                </configuration>
                            </execution>
                        </executions>
                    </plugin>
                </plugins>
            </build>
        </profile>

and additionally, you need to make sure that instrumented artifacts are used during cobertura:cobertura run. What we have here is two profiles, one activated by -Dcobertura and other one by default:

        <profile>
            <id>cobertura</id>
            <activation>
                <property>
                    <name>cobertura</name>
                </property>
            </activation>
            <dependencies>
                <dependency>
                    <groupId>com.sun.jersey</groupId>
                    <artifactId>jersey-core</artifactId>
                    <version>${project.version}</version>
                    <classifier>cobertura</classifier>
                </dependency>
            </dependencies>
        </profile>
        <profile>
            <id>default</id>
            <activation>
                <activeByDefault>true</activeByDefault>
            </activation>
            <dependencies>
                <dependency>
                    <groupId>com.sun.jersey</groupId>
                    <artifactId>jersey-core</artifactId>
                    <version>${project.version}</version>
                </dependency>
            </dependencies>
        </profile>

(this is taken from jersey-server module). I encountered some issues with report generation and actually jersey project structure is not simple, so I couldn't use automatic html report generation in pom file.. So we need to collect all generated cobertura.ser files separately, merge it, prepare all sources and generate report. This can be done by following commands:

# prepare sources
mkdir ../sources
find . | grep src/main/java$ | while read X ; do cp -r "$X"/\* ../sources/ ; done

# compile and run tests
mvn clean install -Dmaven.test.skip=true -Dcobertura
mvn cobertura:cobertura -Dcobertura -DforkMode=never

# collect cobertura.ser files, merge and generate report
find . | grep cobertura.ser$ | while read X  ; do cp "$X" ../cobertura$I.ser ; I=$(($I+1)) ;  done;
cobertura-merge.sh --datafile ./cobertura_final.ser ../cobertura\*
cobertura-report.sh --datafile ./cobertura_filtered.ser --destination ../report --format html ../sources/

Hope it helps somebody who wants to do code coverage for different project. Btw, coverage report for jersey: http://anise.cz/~paja/jersey/report/

[1] http://cobertura.sourceforge.net/
[2] http://foobar.lacoctelera.net/post/2008/09/15/uso-del-plugin-cobertura-dentro-un-proyecto-multi-modulo-en

About

Pavel Bucek-Oracle

Search

Categories
Archives
« August 2015
SunMonTueWedThuFriSat
      
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
     
Today