Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion statements with static conditions do not have coverage #232

Open
bauersimon opened this issue Jan 15, 2024 · 4 comments
Open

Assertion statements with static conditions do not have coverage #232

bauersimon opened this issue Jan 15, 2024 · 4 comments
Assignees

Comments

@bauersimon
Copy link

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
Copy link
Member

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
Copy link
Member

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

@marek-parfianowicz marek-parfianowicz self-assigned this Jan 15, 2024
@bauersimon
Copy link
Author

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants