[
https://issues.apache.org/jira/browse/GROOVY-10878?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17650492#comment-17650492
]
Damir Murat commented on GROOVY-10878:
--------------------------------------
There are some good and bad news :)
First, the good part. For the branch coverage, the patch works as expected. In
the following picture, it can be observed partial branch coverage (yellow
diamond) for Groovy 4.0.6 and full branch coverage for 4.0.7-SNAPSHOT (green
diamond).
!screenshot-1.png|width=943,height=212!
However, the picture also shows partial line coverage (line covered in yellow)
for both versions. The problem is again in JaCoCo, which can not cover
{{throw}} statements inside {{catch}} blocks. In generated code for the Groovy
{{assert}} statement, this relates to the most-outer {{try-catch}} block.
While having full branch coverage is a substantial improvement, it would be
ideal also to have full line coverage. That way, Groovy {{assert}} statements
will not add false positive noise to the line coverage report. This is
important since we might have many assert statements for various pre or
post-condition checks.
For the Groovy {{assert}} statement generated code, the line coverage problem
can be fixed by changing the most-outer {{try-catch}} block
{code}
try {
...
}
catch(Throwable var3) {
var1.clear();
throw var3;
}
{code}
into {{try-finally}}
{code}
try {
...
}
finally {
var1.clear();
}
{code}
because, as far I can see, the most-outer {{try-catch}} is only used for
ensuring the clearing of the {{var1}} variable.
I'm sure this is not a trivial refactoring (if possible at all), but I believe
the benefits can be substantial. In any way, the fixed problem of branch
coverage is already a great improvement.
If you want to try it yourself, I added the {{407_SNAPSHOT}} branch at
[https://github.com/dmurat/groovy-assert-jacoco-coverage-problem] repository.
> Improve JaCoCo's branch code coverage of a Groovy assert statement
> ------------------------------------------------------------------
>
> Key: GROOVY-10878
> URL: https://issues.apache.org/jira/browse/GROOVY-10878
> Project: Groovy
> Issue Type: Improvement
> Affects Versions: 4.0.6
> Reporter: Damir Murat
> Assignee: Eric Milles
> Priority: Major
> Fix For: 4.0.7
>
> Attachments: screenshot-1.png
>
>
> At the moment, there is no way to have full branch coverage for Groovy
> {{assert}} statements. In a larger project, this is very distractive when
> trying to find the pieces of actual logic that should be better covered with
> tests. I believe a slight change in {{assert}} statement code generation can
> significantly improve the situation.
> JaCoCo has long-standing issues with covering calls of methods that throw
> exceptions. When such methods are called inside of, {{if/else}} branches, for
> example, the result is partial coverage reported for those branches.
> However, there is a JaCoCo idiom
> ([https://github.com/jacoco/jacoco/issues/370#issuecomment-267854179|https://github.com/jacoco/jacoco/issues/370#issuecomment-267854179])
> that can be used to avoid uncovered code in those cases. The basic idea is
> to create and return an exception from a called method and throw that
> exception from a caller, like in:
> {code:java}
> void fail() {
> throw create();
> }
> RuntimeException create() {
> return new RuntimeException();
> }
> {code}
> How this relates to the Groovy {{assert}} statement? For example, for a
> simple {{assert}} statement like
> {code:java}
> assert firstName != null
> {code}
> Groovy generates something like
> {code:java}
> ValueRecorder var1 = new ValueRecorder();
> try {
> String var10000 = this.firstName;
> var1.record(var10000, 8);
> var1.record(var10000, 8);
> if (var10000 != null) {
> var1.clear();
> } else {
> ScriptBytecodeAdapter.assertFailed(AssertionRenderer.render("assert
> firstName != null", var1), (Object)null);
> }
> } catch (Throwable var3) {
> var1.clear();
> throw var3;
> }
> {code}
> The problem with generated code is a
> {code:java}
> ScriptBytecodeAdapter.assertFailed(AssertionRenderer.render("assert firstName
> != null", var1), (Object)null);
> {code}
> method call. Inside that method, an exception is created and thrown. Since
> JaCoCo cannot cover that line completely, the branch
> {code:java}
> if (var10000 != null)
> {code}
> will be reported as partially covered.
> To avoid those issues, {{ScriptBytecodeAdapter.assertFailed()}} can be
> adapted (or a new method can be introduced like in the example below) to
> return the exception instead of throwing it. And then, the calling generated
> code can throw that returned exception:
> {code:java}
> try {
> ...
> if (var10000 != null) {
> ...
> } else {
> Throwable throwable =
> ScriptBytecodeAdapter.createAssertionError(AssertionRenderer.render("assert
> firstName != null", var1), (Object)null);
> throw throwable
> }
> } catch (Throwable var3) {
> ...
> }
> {code}
> I have a small project demonstrating the issue and a possible solution here:
> [https://github.com/dmurat/groovy-assert-jacoco-coverage-problem|https://github.com/dmurat/groovy-assert-jacoco-coverage-problem|]
> Tnx
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
