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.

Tomasz Nurkiewicz · Accepted Answer · 2012-01-10T14:44:17.620

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();

I agree. I do find the option of just overriding a method in a test very useful, but it's often an intermediate step on the way to choosing composition. C# is a PITA in that respect as methods aren't virtual by default so I find I often have to jump the whole way (which is annoying as you want to work with the smallest changes possible). — Jeff Foster, Jan 10 '12 at 14:43
Thanks that was exactly what i was looking for :) Thanks also to Jeff — fyr, Jan 10 '12 at 14:47

score 5 · Answer 2 · answered Jan 10 '12 at 14:36

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!

score 0 · Answer 3 · answered Jan 10 '12 at 14:38

0

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.

answered Jan 10 '12 at 14:38

Zsolt

1,464
13
22

score 0 · Answer 4 · answered Jan 10 '12 at 14:45

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.

Unit testing finally blocks in Java 6

4 Answers4