views:

268

answers:

4

Following method shall only be called if it has been verified that there are invalid digits (by calling another method). How can I test-cover the throw-line in the following snippet? I know that one way could be to merge together the VerifyThereAreInvalidiDigits and this method. I'm looking for any other ideas.

public int FirstInvalidDigitPosition {
    get {
        for (int index = 0; index < this.positions.Count; ++index) {
            if (!this.positions[index].Valid) return index;
        }
        throw new InvalidOperationException("Attempt to get invalid digit position whene there are no invalid digits.");
    }
}

I also would not want to write a unit test that exercises code that should never be executed.

+19  A: 

If the "throw" statement in question is truly unreachable under any possible scenario then it should be deleted and replaced with:

Debug.Fail("This should be unreachable; please find and fix the bug that caused this to be reached.");

If the code is reachable then write a unit test that tests that scenario. Error-reporting scenarios for public-accessible methods are perfectly valid scenarios. You have to handle all inputs correctly, even bad inputs. If the correct thing to do is to throw an exception then test that you are throwing an exception.

UPDATE: according to the comments, it is in fact impossible for the error to be hit and therefore the code is unreachable. But now the Debug.Fail is not reachable either, and it doesn't compile because the compiler notes that a method that returns a value has a reachable end point.

The first problem should not actually be a problem; surely the code coverage tool ought to be configurable to ignore unreachable debug-only code. But both problem can be solved by rewriting the loop:

public int FirstInvalidDigitPosition 
{ 
    get 
    { 
        int index = 0;
        while(true) 
        {
            Debug.Assert(index < this.positions.Length, "Attempt to get invalid digit position but there are no invalid digits!");
            if (!this.positions[index].Valid) return index; 
            index++;
        } 
    }
}

An alternative approach would be to reorganize the code so that you don't have the problem in the first place:

public int? FirstInvalidDigitPosition { 
    get { 
        for (int index = 0; index < this.positions.Count; ++index) { 
            if (!this.positions[index].Valid) return index; 
        } 
        return null;
    } 
} 

and now you don't need to restrict the callers to call AreThereInvalidDigits first; just make it legal to call this method any time. That seems like the safer thing to do. Methods that blow up when you don't do some expensive check to verify that they are safe to call are fragile, dangerous methods.

Eric Lippert
I should have mentioned, I'm doing TDD, which exempts me from testing bogus cases :P It is a class-public, but module-internal method. So, throwing an exception is not a part of the requirement, hence no test.
Also Debug.Fail still will not be code-covered, also replacing that throw with Debug.Fail is a compile error.
@user93422: I've updated my answer to address your points.
Eric Lippert
If Jon Skeet posted the exact same answer, he would have had 4x more up-votes by now. Is not that fabulous?
Hamish Grubijan
+2  A: 

I don't understand why you wouldn't want to write a unit test that exercises "code that should not happen".

If it "should not happen", then why are you writing the code at all? Because you think it might happen of course - perhaps an error in a future refactoring will break your other validation and this code will be executed.

If you think it's worth writing the code, it's worth unit testing it.

Jason Williams
I agree - writing code that "should never be executed" sounds like quite a wasted exercise to me.
EnOpenUK
good point. I write that code because C# requires non void method to end either with return or throw. I would rather prefer run-time throw exception at run time. So I don't have to write that ugly line. Which means I need a workaround for language feature, or find a flaw in the design.
To improve the design, I'd use a method rather than a property. A property "looks" like a simple member variable access to the caller, so they are likely to be *surprised* if it takes a long time to execute, has side effects (like changing member variables in a 'get' call), causes events to be fired, or throws an exception. On the other hand, it's normal for a method to return a value to indicate "not found" (e.g. -1, like string.IndexOf) or throw an exception - so a method is a much better design choice.
Jason Williams
+2  A: 

Part of the purpose of testing is to test both things that should happen and things that can happen.

If you have code that should never execute, but could under the right (or wrong) conditions, you can verify that the exception occurs in the right conditions (in MS's Unit Test Framework) by decorating your test method with:

[ExpectedException(typeof(InvalidOperationException))]
kbrimington
I'm doing TDD, as such I don't test for things that _can_ but never _should_ happen. In this case that exception is an indication of a bug in the code, and NOT an exceptional condition.
<don't test for things that can> A way of restating what you wrote is that there are things that you know can happen but you are doing nothing about! Since I don't think you want to leave your program open to undefined behavior, you might want to think through your testing and coding strategy some more! As much as feasible, your requirements should be to satisfactorily deal with all possible inputs, include undesirable ones. Write tests to verify that your program meets these requirements.
binarycoder
A: 

It's sometimes necessary to write small bits of code that can't execute, just to appease a compiler (e.g. if a function which always throw an exception, exits the application, etc. is called from a function which is supposed to return a value, the compiler may insist that the caller include a "return 0", "Return Nothing", etc. statement which can never execute. I wouldn't think one should be required to test such "code", since it may be impossible to make it execute without seriously breaking other parts of the system.

A more interesting question comes with code that the processor should never be able to execute under normal circumstances, but which exists to minimize harm from abnormal circumstances. For example, some safety-critical applications I've written start with code something like (real application is machine code; given below is pseudo-code)

  register = 0
  test register for zero
  if non-zero goto dead
  register = register - 1
  test register for zero
  if non-zero goto okay1
dead:
  shut everything down
  goto dead
okay1:
  register = register + 1
  if non-zero goto dead

I'm not sure how exactly one would go about testing that code in the failure case. The purpose of the code is to ensure that if the register has suffered electrostatic or other damage, or is otherwise not working, the system will shut down safely. Absent some very fancy equipment, though, I have no idea how to test such code.

supercat