Search | Blog | Contact

Uncovered not always bad 12

Posted by Brett Schuchert Thu, 31 May 2007 05:10:00 GMT

Let’s say for the sake of argument that you are:
  • Using a code coverage tool such as Cobertura, Emma
  • You are actually looking at the coverage of your tests (say you’re looking for stale code in our unit tests – [and you’re thinking gosh, I wish I had some unit tests so I could have some unused code in my unit tests])
  • You are testing that unit under test generates an exception
In JUnit 4 we might write the following: 
@Test(expected=RuntimeException.class)
public void methodThatWeExpectAnException() {
    throw new RuntimeException();
}

This test will pass. Yes it’s trivial, of course it would pass. (In reality the single line of code would instead send a message to some object that ultimately would need to generate a RuntimeException for a “real” test to pass.) Fine. That’s not the point.

So what’s the problem with this? Nothing, except that some coverage tools will report the last “line” (the close curly-brace) as not being covered since we did not exit the method cleanly.

Here’s a way to rewrite the above test so that you can assure coverage: 
@Test
public void methodThatWeExpectWillThrowAnException() {
    boolean expectedThrown = false;

    try {
        throw new RuntimeException();
    } catch (RuntimeException e) {
        expectedThrown = true;
    }

    assertTrue(expectedThrown);
}

This version is a bit longer, isn’t it?

Here are some comments I’d like to hear from you:
  • Is it any better?
  • Does is express our intent any better?
  • Isn’t it just silly to run coverage tools on your test code?
  • Is anybody having Pascal flashbacks? (If you don’t get this question…you poor &*$^@~).
Comments

Leave a response

  1. Avatar
    Rytmis about 2 hours later:

    Flashbacks? Lucky &*$^@~... Pascal is a thing some of us have to deal with on a near-daily basis!

    The second version has a lot of syntax that really doesn’t do anything to clarify the intent.

  2. Avatar
    Si about 4 hours later:

    Personally, I prefer the first version, as I’m a big believer in ”’Good’ is good enough.”

    The intent of the first test is perfectly clear.

  3. Avatar
    Jason about 7 hours later:

    I don’t see the value in making the code bigger just to get a coverage report number higher. To me it would make more sense to spend some time with the coverage report and investigate things that aren’t covered as to why, because as you demonstrated sometimes there’s a good reason.

  4. Avatar
    YAChris about 11 hours later:

    Interesting to see how wildly out-of-favor comments are these days. You could just add a comment line before the close-brace in the first case, saying, Gee, this isn’t covered, but it’s no big deal... if that kind of thing was the fashion these days.

  5. Avatar
    Dean Wampler about 13 hours later:

    I would keep the first syntax and get my tests out of the coverage count. Being anal retentive, I hate having uncovered lines in my coverage reports and I don’t personally think it’s important for the tests to be tested for coverage.

    Actually, if you don’t buy the anal-retentive argument, there is another good reason to strive for 100% coverage; you don’t waste your time reviewing the same uncovered lines every time (which are “crying wolf…”). You’ll eventually just stop checking and then you won’t notice when new code that really should have coverage is not covered. To me, it’s potentially a slippery slope like starting to not care if a few unit tests are failing.

  6. Avatar
    Jason about 19 hours later:

    That’s a great point Dean. We should strive for 100% coverage because then anything that’s not covered is what we should investigate.

  7. Avatar
    Ben Rady about 22 hours later:

    Yeah, I wouldn’t tweak the structure of my tests just to increase the coverage in my test code. What does increasing/decreasing/constant test code coverage indicate anyway? Seems just like noise to me.

    Now, I will say that I don’t like the JUnit 4 exception syntax because there doesn’t seem to be a way to check what the exception message is (or maybe I just haven’t found it yet)

  8. Avatar
    Uncle Bob 1 day later:

    I’d fix the freakin tool.

    ;-)

    Or rather, I would not warp my code because of the way cobertura, or emma, behaves. I like the first version, and would either fix the tool, or tolerate the coverage miss.

    Actually, there’s no way to cover all the production code anyway. Interfaces, for example, aren’t covered by most tools. So there will always be some code that is missed. A few extra closing braces don’t bother me.

    I like covering my tests. I have found some very interesting surprises by looking at the portions of my tests that are covered and the portions that are not.

  9. Avatar
    Brett Schuchert 2 days later:

    I think fixing the tool is a good way to go. I do think the above fix is way too much. On the other hand I really like covering my tests as well.

    I’m under the impression we want to refactor our code. When I hear this, I hear we should do so for both “production” code as well as “test” code. If that’s the case, then I don’t see why we should not cover our tests.

  10. Avatar
    Eric Bodden 5 days later:

    I think you are asking the wrong question. The more interesting seems to be to me: How bad are those code coverage tools??? I think the least they should be able to do is some decent control flow analysis, if not even data flow analysis. If a tool does not report 100% coverage for your three lines example, I would dump it!

  11. Avatar
    Timo Rantalaiho 13 days later:

    While I don’t like coverage filling, I like making assertions on the expected exception message.

  12. Avatar
    Mike 15 days later:

    Prefer #2, as I usually check the message of the thrown exception: try { throwSomething(); fail(“FooException expected”) } catch (FooException expected) { assertEquals(“Foo”,expected.getMessage()); }

Comments