Unit testing finally blocks in Java 6

Question

While reviewing my code coverage i noticed a lot of Unit tests fail to check finally blocks which try to close open InputStreams in finally blocks.

One Example excerpt is:

  try {
      f = new BufferedInputStream(new FileInputStream(source));
      f.read(buffer);
  } finally {
      if (f != null)
          try {
              f.close();
          } catch (IOException ignored) {
          }
      }
  }

Is there any appropriate solution to check everything inside the finally block using JUnit4 ?

I know that a code coverage of 100% is not achievable while keeping maximum productivity in mind. However these red lines are sort of an eyecatcher in the report.

Answer 1

First of all consider using IOUtils.closeQuietly(), which will reduce your untested code (and probably duplication) into:

  try {
      f = new BufferedInputStream(new FileInputStream(source));
      f.read(buffer);
  } finally {
      IOUtils.closeQuietly(f);
  }

Now it becomes hard. The "right" way would be to externalize the creation of BufferedInputStream into another class and inject mock. Having a mock you can verify whether appropriate close() method was invoked.

@JeffFoster's answer is pretty close to what I mean, however I would recommend composition over inheritance (at the expense of more code):

  try {
      f = fileSystem.open(source);
      f.read(buffer);
  } finally {
      IOUtils.closeQuietly(f);
  }

Where fileSystem is an instance of FileSystem interface with simple real implementation injected in production code or mock for testing.

interface FileSystem {

    InputStream open(String file);

}

Another benefit of externalizing file opening is that if you one decide to remove buffering or add encryption, there is just a single place to modify.

Having that interface you instantiate your test code with mocks (using Mockito):

//given
FileSystem fileSystemMock = mock(FileSystem.class);
InputStream streamMock = mock(InputStream.class);

given(fileSystemMock.open("file.txt")).willReturn(streamMock);

//when
//your code

//then
verify(streamMock).close();

Answer 2

You could refactor the code a little

public class TestMe {
  public void doSomething() {
    try {
      f = new BufferedInputStream(new FileInputStream(source));
      f.read(buffer);
    } finally {
      if (f != null)
      try {
          f.close();
      } catch (IOException ignored) { }
    }
  }
}

To something like this

public class TestMe {
  public void doSomething() {
    try {
      f = createStream()
      f.read(buffer);
    } finally {
      if (f != null)
      try {
          f.close();
      } catch (IOException ignored) { }
    }
  }

  public InputStream createStream() {
      return new BufferedInputStream(new FileInputStream(source));
  }
}

And now you can write your test to capture the input stream class and verify that is closed. (code is rough, but hopefully you get the general idea).

public void TestSomething () {
   InputStream foo = mock(InputStream.class); // mock object
   TestMe testMe = new TestMe() {
     @Override
     public InputStream createStream() {
          return foo;
     } 
   }

   testMe.something();

   verify(foo.close());
}

Whether this is worth it or not is a different question!

Answer 3

You should inject a mocked BufferedInputStream - or create it with a factory - and when the mock's close() method is called then throw an IOException.

Additionally, I won't that the finally block above until you don't any logic in there.

Answer 4

I think you need to ask yourself whether that is truly worth the testing effort. Some testing junkies tend to miss the diminishing returns of trying to achieve ~100% test coverage. In this case it looks like some of the proposed solutions add more complexity to the actual code in order to make it "testable". I'm fine with complex test code, but adding complexity to the actual code just to make it "testable" strikes me as a terrible idea.