openclover / clover

This repository contains source code of OpenClover Core as well as its integrations: Clover-for-Ant, Clover-for-Eclipse and Clover-for-IDEA plugins. Sources are licensed under Apache 2.0 license.
Other
57 stars 14 forks source link

Assertion statements with static conditions do not have coverage #232

Open bauersimon opened 5 months ago

bauersimon commented 5 months ago

Assertion statements with static conditions (i.e. assert true) are not detected by clover when they are executed. CC @zimmski

Reproducer

// demo/Demo.java

class Demo {
    public static void main(String[] a) {
        primitive();
        object();
    }
    static void static_assert() {
        assert true;
    }
    static void object_assert() {
        assert new Boolean(true);
    }
}
  1. java -cp <path-to-jars>/clover-4.5.1.jar com.atlassian.clover.CloverInstr -i demo/coveragedb -d demo/instructed demo/Demo.java
  2. javac -cp <path-to-jars>/clover-4.5.1.jar -g:source,lines,vars demo/instructed/Demo.java
  3. java -cp <path-to-jars>/clover-4.5.1.jar:./demo/instructed -ea Demo
  4. java -cp <path-to-jars>/clover-4.5.1.jar com.atlassian.clover.reporters.xml.XMLReporter -i demo/coveragedb -o demo/coverage.xml -l

Inspecting the generated xml report, only the assert statement for the "object" case has coverage:

...
<line complexity="1" visibility="package" signature="static_assert() : void" num="7" count="1" type="method"/>
<line complexity="2" visibility="package" signature="object_assert() : void" num="10" count="1" type="method"/>
<line falsecount="0" truecount="1" num="11" type="cond"/>
...

Version

openclover 4.5.1

marek-parfianowicz commented 5 months ago

Hi Simon,

Thank you for reporting this interesting finding. I checked OpenClover's source code and indeed, the assert statement is not being instrumented:

https://github.com/openclover/clover/blob/master/clover-core/src/main/java/com/atlassian/clover/instr/java/java.g#L2229

// an assert statement
        ASSERT
        {
            enterContext(ContextStore.CONTEXT_ASSERT);
            instrumentable = false;  // <<<< HERE
            saveContext = getCurrentContext();
        }
        {
            tmp=lt(1);
        }
        expression
        ( colon:COLON! {instrBoolExpr(tmp, ct(colon)); assertColonPart=true;}  expression )?
        semi:SEMI!
        {
            if (!assertColonPart) {
                 instrBoolExpr(tmp, ct(semi));
            }
            exitContext();
        }

This means it does not measure statement coverage for asserts.

However, what is being instrumented is assert's expression, this means it measures branch coverage for expressions.

With one exception though. It does not instrument constant expressions, because in constants there's no true and false branch to be executed:

https://github.com/openclover/clover/blob/master/clover-core/src/main/java/com/atlassian/clover/instr/java/java.g#L2971

conditionalExpression
{
    CloverToken startOfCond = null;
    CloverToken lf = null;
}
    :
        (lambdaFunctionPredicate) => lf=lambdaFunction
    |
        (logicalOrExpression (QUESTION)?) =>
        {startOfCond = lt(1);}
        logicalOrExpression
        (   endOfCond:QUESTION
            {if (!constExpr) instrBoolExpr(startOfCond, ct(endOfCond));}  // <<<<< HERE
            assignmentExpression COLON! conditionalExpression
        )?
    ;

This is exactly what we can see in the instrumented code of Demo.java:

/* $$ This file has been instrumented by Clover 4.5.2-SNAPSHOT#20240111215423 $$ */package demo;

class Demo {public static class __CLR4_5_200lrfebc62{public static com_atlassian_clover.CoverageRecorder R; /* code removed for readability */}
    public static void main(String[] a) {__CLR4_5_200lrfebc62.R.inc(0); // THIS IS METHOD COVERAGE
        __CLR4_5_200lrfebc62.R.inc(1);static_assert();    // THIS IS STATEMENT COVERAGE - R.inc call
        __CLR4_5_200lrfebc62.R.inc(2);object_assert();  // THIS IS STATEMENT COVERAGE - R.inc call
    }
    static void static_assert() {__CLR4_5_200lrfebc62.R.inc(3);
        assert true;    // NO STATEMENT COVERAGE - no R.inc(), NO BRANCH COVERAGE EITHER
    }
    static void object_assert() {__CLR4_5_200lrfebc62.R.inc(4);
        // NO STATEMENT COVERAGE, BUT BRANCH COVERAGE FOR AN EXPRESSION
        assert (((new Boolean(true))&&(__CLR4_5_200lrfebc62.R.iget(5)!=0|true))||(__CLR4_5_200lrfebc62.R.iget(6)==0&false));
    }
}
marek-parfianowicz commented 5 months ago

Another question is why the assert as a statement is not being instrumented. I checked OpenClover's code history and this "instrumentable = false" exists since the first commit in the repository - so since the original Clover version open-sourced by Atlassian.

I suspect that there might be some specific grammar context in which the assert keyword might appear, preventing it from being instrumented.

A similar example is the "catch" block. OpenClover does not insert the recorder call before "catch" keyword, because it would make no sense. It inserts it inside the catch { } block.

try { ... }
/* NO R.inc HERE */ catch(...) {
   /* R.inc HERE */
}

Similarly, OpenClover does not insert the recorder call before "super()" in the constructor, because super must be the first statement executed.

SomeConstructor() {
  /* NO R.inc HERE */ super(...);
}

I will try to investigate in which conditions asserts cannot be instrumented as a statement...

bauersimon commented 5 months ago

Thank you so much for your swift reply and for taking a look already.

Interesting to see that it is actually the branch coverage that is responsible for the difference between "static" and "dynamic" assert conditions. Thanks for all the explanations so far!

There was a bit of extra confusion since Clover has an extra "coverage context" for assert statements. So I was very certain that assert statements would be covered by default.

Thanks for taking a deeper look.

marek-parfianowicz commented 5 months ago

You can exclude include/assert statements using coverage context during report generation. But this shall include/exclude only the branch coverage for them.