Instrumentation causing coverage checks to fail

[dwightmulcahy]

I pretty much have the minimal setup for instrumentation. Not all the classes have nullable or nonnull annotations.

<!--https://github.com/osundblad/intellij-annotations-instrumenter-maven-plugin-->
<plugin>
    <groupId>se.eris</groupId>
    <artifactId>notnull-instrumenter-maven-plugin</artifactId>
    <version>0.6.6</version>
    <executions>
        <execution>
            <goals>
                <goal>instrument</goal>
                <goal>tests-instrument</goal>
            </goals>
        </execution>
    </executions>
    <configuration>
        <notNull>
            <param>javax.annotation.Nonnull</param>
        </notNull>
        <nullable>
            <param>javax.annotation.Nullable</param>
        </nullable>
    </configuration>
</plugin>       

I have default-jacoco-check maven phase that has the following rule set:

<rule implementation="org.jacoco.maven.RuleConfiguration">
    <element>BUNDLE</element>
    <limits>
        <limit implementation="org.jacoco.report.check.Limit">
            <counter>INSTRUCTION</counter>
            <value>COVEREDRATIO</value>
            <minimum>0.87</minimum>
        </limit>

The coverage before instrumentation was 89%... after it reports 79% (and fails building). Looking at the coverage report shows that all the classes that have nonnull/nullable are the ones that are falling in coverage percentage.

I'm I missing something here or is this not going to be compatible with using default-jacoco-check?

[osundblad]

This is why I introduced the "Turn of Instrumentation flag". The instrumentation adds null checks in the byte code, so code analysers and coverage tools see that there are no tests on null check code resulting in lower code coverage.

You should get back to your old coverage if you turn off the instrumentation during analysis by for example having an analysis maven profile you can try it out by hand using:

mvn clean install -Dse.eris.notnull.instrument=false

See Turn off Instrumentation

I personally usually set this property in a maven profile since Sonar usually warns a lot about unnecessary null checks if you don't.

[osundblad]

Another way to see it is that you actually don't have any tests for null parameters and that this plugin helps you realise that. So you should start writing those tests now that you know...

That is not something I would do since it costs a lot of time/money and I think the instrumentation gives fast enough feedback at runtime.

[atorstling]

As a side note, could you try measuring line coverage instead of instruction coverage? This way, I think that the coverage of synthetic instructions will not be measured and not affect the end result. See http://www.jacoco.org/jacoco/trunk/doc/counters.html.

[iggymoran]

There seem to be some differences between the code that this tool produces, and the code that IntelliJ produces.

Exceptions from IntelliJ seem to be reported from a $$$reportNul$$$ method, where as exceptions from this tool are thrown from inlining the null check:

if (foo == null) throw new IllegalArgumentException(...)

I think that if this tool generated a (synthetic) reportNull method that did the evaluation, then we wouldn't get a drop in code coverage.

void myMethod(@NotNull String myParam) {
    //code generated:
    this.$$$reportNull$$$(myParam, "myParam");
}

...
//synthetic
void $$$reportNull$$$(Object param, String paramName) {
    if (param == null) 
        throw new IllegalArgumentException(paramName);
}

[osundblad]

It would most likely reduce the loss in code coverage (and if you test just one notnull parameter with a null value per class remove it).

I will try look into it when I have some time over, but I have had very little time to spend on this project this last year so I can't promise it will be soon. (A pull request would be great!)