Add filter for assert statement

[bjkail]

Add a filter for assert statements (JAVAC.ASSERT):

• Ignore the initialization of the $assertionsDisabled static field in the <clinit> method.
• Ignore the assert by looking for the GETSTATIC+IFNE of $assertionsDisabled.

[bjkail]

On second thought, rather than removing all assert-related code, perhaps we should only remove the "assertions disabled" and "assertions failed" code. That is, assume -ea and that assertions pass. Thoughts?

[marchof]

@bjkail First of all thanks a lot for your contribution and helping us with the filters!

In the first round for the built-in filters we're focusing on synthetic compiler artifacts which have no direct correspondence to the source code. For Java assertions I would see the following artifacts as synthetic:

• Initialization of $assertionsDisabled
• Check for $assertionsDisabled

The assertion code itself is present in source code and should therefore be treated like regular code to be executed and tested. From my point of view also for both cases (pass and fail).

[bjkail]

Ok, I'll see if I can rework along those lines, thanks.

It's a little unfortunate to leave the failed assertion code in place, since assert is almost always used to test internal invariants, which might be difficult/impossible/undesirable to force negative. For the "second round", do you intend to add options to allow users to ignore user-written code?

[marchof]

@bjkail I agree. In general Java assertions are questionable and probably not widely used in practice. As far as I remember we received only a single question about it.

In our filter candidates there is a section "Test Execution is Questionable or Impossible by Design".

[bjkail]

@marchof I guess you can consider this to be a second question about them :). I see that we implemented "Private empty constructors", so would it be ok for this PR to also ignore the 'throw new AssertionError' + "message" portion of assert? I understand if you'd rather not by default, but it would be nice to have some kind of plan to address that in the future.

[bjkail]

Rebased to master, and updated to only filter compiler-generated code not present in the source code (initialization and load+jump for $assertionsDisabled).

[marchof]

@bjkail Regarding ignore AssertionError path: As I personally don't like the usage of Java assertions at all I'm ok with ignoring the throws AssertionError(...) path. So we will only see whether the assertion was executed (only the case if run with -ea) and there will be no branches visible.

[bjkail]

@marchof Thanks for the feedback. I found it's non-trivial to accurately find the boundary between the assert expression (especially those containing ?:) and the throw statement, so I removed it from this PR. My first attempt was to walk backward from the assertion branch target label and to look for the first NEW AssertionError, but that approach fails for assert x : new AssertionError() and other complicated expressions involving ?: and AssertionError in the message. Perhaps those edge cases aren't worth worrying about, or perhaps we could walk the instructions following branches until the stack size drops to 0. Perhaps I'll get motivated to attempt something in another PR :-).

[marchof]

@bjkail ok! Let's do baby steps. This his how we try to come to a first 0.8.0 release.

[bjkail]

@Godin Did my latest commit address your concerns?

[MarkSinke]

Would be helpful if this got merged - we are probably one of the few teams actually liking asserts, and we sprinkle them through our code where preconditions should be met. Our coverage numbers are now artificially low because of that, and it's harder to keep a baseline.

[marchof]

@MarkSinke Have you actually tested this PR with your code base? Does it provide the expected results for you?

[MarkSinke]

Sorry no - I'm not sure how I would test a change that is not released yet? Mark.

On Mon, Sep 24, 2018 at 3:59 PM Marc R. Hoffmann ***@***.***> wrote: @MarkSinke <https://github.com/MarkSinke> Have you actually tested this PR with your code base? Does it provide the expected results for you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#613 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFi1D0n4l8ojsZ3Y72xj-RwYYDPU9XJZks5ueOVQgaJpZM4P5odh> .

-- *Mark Sinke* CTO +31 646 255 635 +31 30 699 1930 mark.sinke@forcare.com www.forcare.com

[marchof]

@MarkSinke I just rebased this PR on master. In case you want to it with Ant or the CLI you can download a build of this PR here:

https://ci.appveyor.com/project/JaCoCo/jacoco/build/1.0.2195/artifacts

[Godin]

@bjkail I started working on getting this done - wanted to mention your contribution in our changelog and realized that you didn't wrote name in headers. So wondering if this was on purpose or just was forgotten? is there any IP/legal issues?

[bjkail]

I don't recall, I probably copied from somewhere, cleared it out, and then just didn't bother to fill in my name :-). There are no IP/legal issues.

[marcust]

@Godin What's the state of this? Any chance of getting this merged and released soon? (My use-case being ignoring Lombok generated null check assertions that are possible since 1.18.8)

[lblasa]

@Godin any update on this? Is this going to be merged and released soon?

[kno10]

I would appreciate to see this in the next jacoco release.

[mirabilos]

I’ve just stumbled on this and would also like to see something fixing it merged.

I managed to test 3 out of 4 branches (by specifically writing tests that trigger the assertions) and am fine with requiring that at first, but the fourth is virtually impossible (unload the class, then reload it with assertions disabled, with a custom classloader, ugh… no, thanks).

So, could we please exclude the “untestable” branch (Edit: and the static initialiser, see #1063) first, get that into a release I can use with Maven, and perhaps consider ignoring assertions later/elsewhere?

Thanks in advance!

[mirabilos]

What’s holding this up?

[TomerFi]

This pr getting merged will be much appreciated.
:-)

[Godin]

#1196 was merged instead

[TomerFi]

@Godin do you perhaps know of a timeline for a possible release containing the modifications in #1196 ?

[mirabilos]

This bug is unfortunately still open with jacoco-maven-plugin 0.8.8 (I still get the partial coverage at the beginning of a class using assert and on the assert lines).

[marchof]

@mirabilos What exactly is "still open" for you? Note that in #1196 we added a filter to hide the internal initialization code for asserts. But still each assert gives two branches: Good case and failure -- which is how assert works.

[mirabilos]

Marc R. Hoffmann dixit: ***@***.*** What exactly is "still open" for you? Note that in #1196 we ***@***.*** a filter to hide the internal initialization code for asserts. ***@***.*** still each assert gives two branches: Good case and failure -- ***@***.*** is how assert works. I still get the partial branch coverage at the beginning of the class and partial branch coverage for the asserts. At least Sonar says so… Asserts *cannot* have two (or more) cases for the domain of coverage because they’re literally a statement that the other condition cannot happen at that point in control/data flow. bye, //mirabilos -- I believe no one can invent an algorithm. One just happens to hit upon it when God enlightens him. Or only God invents algorithms, we merely copy them. If you don't believe in God, just consider God as Nature if you won't deny existence. -- Coywolf Qi Hunt

[marchof]

@mirabilos This filter is implemented since our latest release 0.8.8. Here is the test case:

https://github.com/jacoco/jacoco/blob/master/org.jacoco.core.test.validation.java5/src/org/jacoco/core/test/validation/java5/targets/AssertTarget.java

Please open a new bug with an executable reproducer which demonstrates the problem - preferably a Git repository with simple Maven project. Thx!

[Godin]

Furthermore in addition to what @marchof said there is example in #1196 demonstrating behavior before and after.

[mirabilos]

Testcase is still https://github.com/tarent/rfc822/ (git master this time).

[marchof]

@mirabilos We cannot provide support for Sonarqube. If you think the problem is with JaCoCo please provide a executable build which includes a JaCoCo report goal and shows the problem.

Otherwise please report the problem to Sponarqube.