8 branches for try with resources - jacoco coverage possible?

Question

I've got some code that uses try with resources and in jacoco it's coming up as only half covered. All the source code lines are green, but I get a little yellow symbol telling me that only 4 of 8 branches are covered.

I'm having trouble figuring out what all the branches are, and how to write code that covers them. Three possible places throw PipelineException. These are createStageList(), processItem() and the implied close()

1. Not throwing any exceptions,
2. throwing an exception from createStageList()
3. throwing an exception from processItem()
4. throwing an exception from close()
5. throwing an exception from processItem() and close()

I can't think of any other cases, yet I still only have 4 of 8 covered.

Can someone explain to me why it's 4 of 8 and is there anyway to hit all 8 branches? I'm not skilled with decyrpting/reading/interpreting byte code, but maybe you are... :) I've already seen https://github.com/jacoco/jacoco/issues/82, but neither it nor the issue it references help very much (other than noting that this is due to compiler generated blocks)

Hmm, just as I finish writing this I had a thought on what case(s) might not be not tested by what I mention above... I'll post an answer if I got it right. I'm sure this question and it's answer will help someone in any case.

EDIT: Nope, I didn't find it. Throwing RuntimeExceptions (not handled by the catch block) didn't cover any more branches

Solution 1

Well I can't tell you what the exact problem with Jacoco is, but I can show you how Try With Resources is compiled. Basically, there are a lot of compiler generated switches to handle exceptions thrown at various points.

If we take the following code and compile it

public static void main(String[] args){
    String a = "before";

    try (CharArrayWriter br = new CharArrayWriter()) {
        br.writeTo(null);
    } catch (IOException e){
        System.out.println(e.getMessage());
    }

    String a2 = "after";
}

And then disassemble, we get

.method static public main : ([Ljava/lang/String;)V
    .limit stack 2
    .limit locals 7
    .catch java/lang/Throwable from L26 to L30 using L33
    .catch java/lang/Throwable from L13 to L18 using L51
    .catch [0] from L13 to L18 using L59
    .catch java/lang/Throwable from L69 to L73 using L76
    .catch [0] from L51 to L61 using L59
    .catch java/io/IOException from L3 to L94 using L97
    ldc 'before'
    astore_1
L3:
    new java/io/CharArrayWriter
    dup
    invokespecial java/io/CharArrayWriter <init> ()V
    astore_2
    aconst_null
    astore_3
L13:
    aload_2
    aconst_null
    invokevirtual java/io/CharArrayWriter writeTo (Ljava/io/Writer;)V
L18:
    aload_2
    ifnull L94
    aload_3
    ifnull L44
L26:
    aload_2
    invokevirtual java/io/CharArrayWriter close ()V
L30:
    goto L94
L33:
.stack full
    locals Object [Ljava/lang/String; Object java/lang/String Object java/io/CharArrayWriter Object java/lang/Throwable
    stack Object java/lang/Throwable
.end stack
    astore 4
    aload_3
    aload 4
    invokevirtual java/lang/Throwable addSuppressed (Ljava/lang/Throwable;)V
    goto L94
L44:
.stack same
    aload_2
    invokevirtual java/io/CharArrayWriter close ()V
    goto L94
L51:
.stack same_locals_1_stack_item
    stack Object java/lang/Throwable
.end stack
    astore 4
    aload 4
    astore_3
    aload 4
    athrow
L59:
.stack same_locals_1_stack_item
    stack Object java/lang/Throwable
.end stack
    astore 5
L61:
    aload_2
    ifnull L91
    aload_3
    ifnull L87
L69:
    aload_2
    invokevirtual java/io/CharArrayWriter close ()V
L73:
    goto L91
L76:
.stack full
    locals Object [Ljava/lang/String; Object java/lang/String Object java/io/CharArrayWriter Object java/lang/Throwable Top Object java/lang/Throwable
    stack Object java/lang/Throwable
.end stack
    astore 6
    aload_3
    aload 6
    invokevirtual java/lang/Throwable addSuppressed (Ljava/lang/Throwable;)V
    goto L91
L87:
.stack same
    aload_2
    invokevirtual java/io/CharArrayWriter close ()V
L91:
.stack same
    aload 5
    athrow
L94:
.stack full
    locals Object [Ljava/lang/String; Object java/lang/String
    stack 
.end stack
    goto L108
L97:
.stack same_locals_1_stack_item
    stack Object java/io/IOException
.end stack
    astore_2
    getstatic java/lang/System out Ljava/io/PrintStream;
    aload_2
    invokevirtual java/io/IOException getMessage ()Ljava/lang/String;
    invokevirtual java/io/PrintStream println (Ljava/lang/String;)V
L108:
.stack same
    ldc 'after'
    astore_2
    return
.end method

For those who don't speak bytecode, this is roughly equivalent to the following pseudo Java. I had to use gotos because the bytecode doesn't really correspond to Java control flow.

As you can see, there are a lot of cases to handle the various possibilities of suppressed exceptions. It's not reasonable to be able to cover all these cases. In fact, the goto L59 branch on the first try block is impossible to reach, since the first catch Throwable will catch all exceptions.

try{
    CharArrayWriter br = new CharArrayWriter();
    Throwable x = null;

    try{
        br.writeTo(null);
    } catch (Throwable t) {goto L51;}
    catch (Throwable t) {goto L59;}

    if (br != null) {
        if (x != null) {
            try{
                br.close();
            } catch (Throwable t) {
                x.addSuppressed(t);
            }
        } else {br.close();}
    }
    break;

    try{
        L51:
        x = t;
        throw t;

        L59:
        Throwable t2 = t;
    } catch (Throwable t) {goto L59;}

    if (br != null) {
        if (x != null) {
            try{
                br.close();
            } catch (Throwable t){
                x.addSuppressed(t);
            }
        } else {br.close();}
    }
    throw t2;
} catch (IOException e) {
    System.out.println(e)
}

Solution 2

I can cover all 8 branches, so my answer is YES. Look at the following code, this is only a fast try, but it works (or see my github: https://github.com/bachoreczm/basicjava and the 'trywithresources' package, there you can find, how try-with-resources works, see 'ExplanationOfTryWithResources' class):

import java.io.ByteArrayInputStream;
import java.io.IOException;

import org.junit.Test;

public class TestAutoClosable {

  private boolean isIsNull = false;
  private boolean logicThrowsEx = false;
  private boolean closeThrowsEx = false;
  private boolean getIsThrowsEx = false;

  private void autoClose() throws Throwable {
    try (AutoCloseable is = getIs()) {
        doSomething();
    } catch (Throwable t) {
        System.err.println(t);
    }
  }

  @Test
  public void test() throws Throwable {
    try {
      getIsThrowsEx = true;
      autoClose();
    } catch (Throwable ex) {
      getIsThrowsEx = false;
    }
  }

  @Test
  public void everythingOk() throws Throwable {
    autoClose();
  }

  @Test
  public void logicThrowsException() {
    try {
      logicThrowsEx = true;
      everythingOk();
    } catch (Throwable ex) {
      logicThrowsEx = false;
    }
  }

  @Test
  public void isIsNull() throws Throwable {
    isIsNull = true;
    everythingOk();
    isIsNull = false;
  }

  @Test
  public void closeThrow() {
    try {
      closeThrowsEx = true;
      logicThrowsEx = true;
      everythingOk();
      closeThrowsEx = false;
    } catch (Throwable ex) {
    }
  }

  @Test
  public void test2() throws Throwable {
    try {
      isIsNull = true;
      logicThrowsEx = true;
      everythingOk();
    } catch (Throwable ex) {
      isIsNull = false;
      logicThrowsEx = false;
    }
  }

  private void doSomething() throws IOException {
    if (logicThrowsEx) {
      throw new IOException();
    }
  }

  private AutoCloseable getIs() throws IOException {
    if (getIsThrowsEx) {
      throw new IOException();
    }
    if (closeThrowsEx) {
      return new ByteArrayInputStream("".getBytes()) {

        @Override
        public void close() throws IOException {
          throw new IOException();
        }
      };
    }
    if (!isIsNull) {
      return new ByteArrayInputStream("".getBytes());
    }
    return null;
  }
}

Solution 3

No real question, but wanted to throw more research out there. tl;dr = It looks like you can achieve 100% coverage for try-finally, but not for try-with-resource.

Understandably, there's a difference between old-school try-finally and Java7 try-with-resources. Here's two equivalent examples showing the same thing using alternate approaches.

Old School example (a try-finally approach):

final Statement stmt = conn.createStatement();
try {
    foo();
    if (stmt != null) {
        stmt.execute("SELECT 1");
    }
} finally {
    if (stmt != null)
        stmt.close();
}

Java7 example (a try-with-resource approach):

try (final Statement stmt = conn.createStatement()) {
    foo();
    if (stmt != null) {
        stmt.execute("SELECT 1");
    }
}

Analysis: old-school example:
Using Jacoco 0.7.4.201502262128 and JDK 1.8.0_45, I was able to get 100% line, instruction and branch coverage on the Old School example using the following 4 tests:

• Basic grease path (statement not null, and execute() is exercised normally)
• execute() throws exception
• foo() throws exception AND statement returned as null
• statement returned as null

Analysis: java-7 example:
If the same 4 tests run against the Java7 style example, jacoco indicates 6/8 branches are covered (on the try itself) and 2/2 on the null-check within the try. I tried a number of additional tests to increase coverage, but I can find no way to get better than 6/8. As others have indicated, the decompiled code (which I did also look at) for the java-7 example suggests that the java compiler is generating unreachable segments for try-with-resource. Jacoco is reporting (accurately) that such segments exist.

Update: Using Java7 coding style, you might be able to get 100% coverage IF using a Java7 JRE (see Matyas response below). However, using Java7 coding style with a Java8 JRE, I believe you will hit the 6/8 branches covered. Same code, just different JRE. Seems like the byte code is being created differently between the two JREs with the Java8 one creating unreachable pathways.

Solution 4

Four years old, but still...

1. Happy path with non-null AutoCloseable
2. Happy path with null AutoCloseable
3. Throws on write
4. Throws on close
5. Throws on write and close
6. Throws in resource specification (the with part, e.g. constructor call)
7. Throws in try block but AutoCloseable is null

Above lists all 7 conditions - the reason for the 8 branches is due to repeated condition.

All branches can be reached, the try-with-resources is fairly simple compiler sugar (at least compared to switch-on-string) - if they cannot be reached, then it is by definition a compiler bug.

Only 6 unit tests are actually required (in the example code below, throwsOnClose is @Ingored and branch coverage is 8/8.

Also note that Throwable.addSuppressed(Throwable) cannot suppress itself, so the generated bytecode contains an additional guard (IF_ACMPEQ - reference equality) to prevent this). Luckily this branch is covered by the throw-on-write, throw-on-close and throw-on-write-and-close cases, as the bytecode variable slots are reused by the outer 2 of 3 exception handler regions.

This is not an issue with Jacoco - in fact the example code in the linked issue #82 is incorrect as there are no duplicated null checks and there is no nested catch block surrounding the close.

JUnit test demonstrating 8 of 8 branches covered

import static org.hamcrest.Matchers.arrayContaining;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.sameInstance;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;

import java.io.IOException;
import java.io.OutputStream;
import java.io.UncheckedIOException;

import org.junit.Ignore;
import org.junit.Test;

public class FullBranchCoverageOnTryWithResourcesTest {

    private static class DummyOutputStream extends OutputStream {

        private final IOException thrownOnWrite;
        private final IOException thrownOnClose;


        public DummyOutputStream(IOException thrownOnWrite, IOException thrownOnClose)
        {
            this.thrownOnWrite = thrownOnWrite;
            this.thrownOnClose = thrownOnClose;
        }


        @Override
        public void write(int b) throws IOException
        {
            if(thrownOnWrite != null) {
                throw thrownOnWrite;
            }
        }


        @Override
        public void close() throws IOException
        {
            if(thrownOnClose != null) {
                throw thrownOnClose;
            }
        }
    }

    private static class Subject {

        private OutputStream closeable;
        private IOException exception;


        public Subject(OutputStream closeable)
        {
            this.closeable = closeable;
        }


        public Subject(IOException exception)
        {
            this.exception = exception;
        }


        public void scrutinize(String text)
        {
            try(OutputStream closeable = create()) {
                process(closeable);
            } catch(IOException e) {
                throw new UncheckedIOException(e);
            }
        }


        protected void process(OutputStream closeable) throws IOException
        {
            if(closeable != null) {
                closeable.write(1);
            }
        }


        protected OutputStream create() throws IOException
        {
            if(exception != null) {
                throw exception;
            }
            return closeable;
        }
    }

    private final IOException onWrite = new IOException("Two writes don't make a left");
    private final IOException onClose = new IOException("Sorry Dave, we're open 24/7");


    /**
     * Covers one branch
     */
    @Test
    public void happyPath()
    {
        Subject subject = new Subject(new DummyOutputStream(null, null));

        subject.scrutinize("text");
    }


    /**
     * Covers one branch
     */
    @Test
    public void happyPathWithNullCloseable()
    {
        Subject subject = new Subject((OutputStream) null);

        subject.scrutinize("text");
    }


    /**
     * Covers one branch
     */
    @Test
    public void throwsOnCreateResource()
    {
        IOException chuck = new IOException("oom?");
        Subject subject = new Subject(chuck);
        try {
            subject.scrutinize("text");
            fail();
        } catch(UncheckedIOException e) {
            assertThat(e.getCause(), is(sameInstance(chuck)));
        }
    }


    /**
     * Covers three branches
     */
    @Test
    public void throwsOnWrite()
    {
        Subject subject = new Subject(new DummyOutputStream(onWrite, null));
        try {
            subject.scrutinize("text");
            fail();
        } catch(UncheckedIOException e) {
            assertThat(e.getCause(), is(sameInstance(onWrite)));
        }
    }


    /**
     * Covers one branch - Not needed for coverage if you have the other tests
     */
    @Ignore
    @Test
    public void throwsOnClose()
    {
        Subject subject = new Subject(new DummyOutputStream(null, onClose));
        try {
            subject.scrutinize("text");
            fail();
        } catch(UncheckedIOException e) {
            assertThat(e.getCause(), is(sameInstance(onClose)));
        }
    }


    /**
     * Covers two branches
     */
    @SuppressWarnings("unchecked")
    @Test
    public void throwsOnWriteAndClose()
    {
        Subject subject = new Subject(new DummyOutputStream(onWrite, onClose));
        try {
            subject.scrutinize("text");
            fail();
        } catch(UncheckedIOException e) {
            assertThat(e.getCause(), is(sameInstance(onWrite)));
            assertThat(e.getCause().getSuppressed(), is(arrayContaining(sameInstance(onClose))));
        }
    }


    /**
     * Covers three branches
     */
    @Test
    public void throwsInTryBlockButCloseableIsNull() throws Exception
    {
        IOException chucked = new IOException("ta-da");
        Subject subject = new Subject((OutputStream) null) {
            @Override
            protected void process(OutputStream closeable) throws IOException
            {
                throw chucked;
            }
        };

        try {
            subject.scrutinize("text");
            fail();
        } catch(UncheckedIOException e) {
            assertThat(e.getCause(), is(sameInstance(chucked)));
        }

    }
}

Caveat

Though not in OP's sample code, there is one case that can't be tested AFAIK.

If you pass the resource reference as an argument, then in Java 7/8 you must have a local variable to assign to:

    void someMethod(AutoCloseable arg)
    {
        try(AutoCloseable pfft = arg) {
            //...
        }
    }

In this case the generated code will still be guarding the resource reference. The syntatic sugar is updated in Java 9, where the local variable is no longer required: try(arg){ /*...*/ }

Supplemental - Suggest use of library to avoid branches entirely

Admittedly some of these branches can be written off as unrealistic - i.e. where the try block uses the AutoCloseable without null checking or where the resource reference (with) cannot be null.

Often your application doesn't care where it failed - to open the file, write to it or close it - the granularity of failure is irrelevant (unless the app is specifically concerned with files, e.g. file-browser or word processor).

Furthermore, in the OP's code, to test the null closeable path - you'd have to refactor the try block into a protected method, subclass and provide a NOOP implementation - all this just get coverage on branches that will never be taken in the wild.

I wrote a tiny Java 8 library io.earcam.unexceptional (in Maven Central) that deals with most checked exception boilerplate.

Relevant to this question: it provides a bunch of zero-branch, one-liners for AutoCloseables, converting checked exceptions to unchecked.

Example: Free Port Finder

int port = Closing.closeAfterApplying(ServerSocket::new, 0, ServerSocket::getLocalPort);

Solution 5

Jacoco has recently fixed this issue, Release 0.8.0 (2018/01/02)

"During creation of reports various compiler generated artifacts are filtered out, which otherwise require unnecessary and sometimes impossible tricks to not have partial or missed coverage:

• Part of bytecode for try-with-resources statements (GitHub #500)."

http://www.jacoco.org/jacoco/trunk/doc/changes.html

View more solutions

Comments

[Antimony]

Can you post the classfile please?

[Gus]

No I can't post my customer's code.

[Torsten Römer]

The best coverage I managed to achieve with Eclemma (Emma in Eclipse) is "3 of 8 branches missed", but Cobertura in Jenkins then still shows only 4/8. Let's hope that soon those coverage tools will handle try-with-resources correctly.

[Thirler]

Note that many constructs that JaCoCo can't fully cover, such as these, are meant to help you reduce the number of possible paths in the code (and thus mistakes). Aiming for 100% coverage on those is often impossible, also it will not add much to your test quality (but it does cost a lot of effort).

[Patrick Michaelsen]

My approach was to simply rewrite my code to not use a try-with-resources clause. It wasn't really adding much value considering it was just syntactic sugar and was causing this testing headache.

[Gus]

Yeah, I wondered if some of the generated code was actually unreachable, thanks. Sure would be nice if Oracle would improve this, or the coverage tools would account for it.

[Torsten Römer]

Great explanation, very interesting! Now I can stop wondering what I have missed. Thanks!

[Gus]

Interesting, though it has been a long time. Things may have changed, what tools & what versions are you using?

[Gus]

your autoClose method has no catch block. It's not the same case (and normally one is not measuring coverage on the test class itself?) Also, a screen shot of the jacoco output showing it covered would be good if you want to claim success.

[Admin]

I attached a screenshot, and yes, watch the coverage of the test class (in the row of the try-with-resources-end, you'll see 8/8).

[Admin]

I also attached a link, where you find the exact description, how try-with-resources works.

[Admin]

I think catch block is irrelevant in the coverage question.

[Gus]

So why not add an exception catch and remove all doubt?

[Admin]

I added it (to remove all doubt, but i don't understand the connection between this topic and the existance of the catch block).

[Gus]

Because as the other answers discuss the issue (at the time of the original post) relates to how the byte code was generated. It's not immediately obvious that the byte code would be the same with and without the catch block. Many things have changed since then, so the next important question is what version of Java and what version of Jacoco did you used? It might be that the byte code in Java 8 simply is more sensible, or it might be that Jacoco got tired of answering these questions and decided to list the unreachable bytecode as covered...

[Gus]

From your answer and the one given by sodamnmad vs Jeff Bennet's answer, it also seems that there may be a difference between byte code generated when several statements are within the try/cach/finally vs a single method call inside try/catch/finally.

[Admin]

I used java 7 and EclEmma 2.3.3, but if there are differences in byte-codes, I don't think, that in any byte-code of try-with-resources statement are dead codes (what we can not cover with tests (if we have a testable code)).

[Parker]

This isn't a try-with-resources as posted in the question, but rather a try-finally containing a conditional.

[Jeff Bennett]

Matyas, I really wanted you to be right, but I cannot replicate your results. As Gus suggests, maybe it's a version thing. I'm still getting "2 of 8 branches missed." I am running jacoco-maven-plugin:0.7.7.201606060606, with JDK 1.8.0_73 and am using JMockit 1.25 and JUnit 4.12. I trust it is working for you (but not me), possibly because I reworked your test code into JMockit style or because of JDK 8 (me) vs JDK 7 (you).

[Joshua Taylor]

There's no need to look at the bytecode here (though it's an interesting exercise). The JLS defines what the try-with-resources is equivalent to, in terms of Java source: 14.20.3.1. Basic try-with-resources, and that makes it easier to see what the branches are.

[earcam]

The bytecode produced by the two code blocks is completely different - the try-with-resources has 3 exception handling regions one starting before conn.createStatement(), the one around the body and another just around the call to if(stmt != null){ stmt.close(); }. Additionally there's a call to Throwable.addSuppressed() and if to guard against suppressing the same the exception.

[Holger]

@JoshuaTaylor the JLS only defines the semantic equivalence. You still need to examine the bytecode to find out whether the compiler uses this strategy literally. Also, you have to add the knowledge that nowadays (mandatory with Java 7), finally blocks get copied for the ordinary and exceptional case, which makes the tests redundant when using the specified pattern literally. As discussed in try with resources introduce unreachable bytecode, this is a javac specific issue, e.g. Eclipse’s compiler does not produce unreachable bytecode.

[Holger]

The problem is that you were looking at Eclipse generated code to dismiss issues raised by javac generated code. It’s a bit harsh to say “if they cannot be reached, then it is by definition a compiler bug”, as the specification does nowhere guaranty that the bytecode is free of unreachable code. Under normal circumstances, you wouldn’t notice at all. And that’s not the only place where javac generates unreachable code, e.g. I’ve seen obsolete access$… methods in the wild. Thankfully, both issues are gone with JDK 11. See also JDK-8194978.