How do I test code that should never be executed?

Question

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.

Lanell

The mere fact you say it shouldn't be executed and can't be reached says to me "Remove it". You've validated the string as invalid to get here, so you know the property will return something. – Heighho

Answer 1

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.

Harquebus

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. – Lanell

Also Debug.Fail still will not be code-covered, also replacing that throw with Debug.Fail is a compile error. – Lanell

If Jon Skeet posted the exact same answer, he would have had 4x more up-votes by now. Is not that fabulous? – Assimilable

Oh, why doesn't C# have a ShouldlNeverGetHere exception? – Greenwald

Answer 2

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.

Woodcut

I agree - writing code that "should never be executed" sounds like quite a wasted exercise to me. – Spheroidal

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. – Lanell

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. – Woodcut

I think the issue is that you don't want to assume that it's literally impossible to get there, but at the same time, you just can't get there from a test. It's not that you don't want to test it, it's that there's no "normal" way to get there. – Nynorsk

Answer 3

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.

Cherish

Answer 4

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))]

Lichenology

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. – Lanell

<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. – Ause

Answer 5

To rephrase the issue, basically: how do you test an invalid state of your system when you've already gone to a lot of trouble to make sure that the system can't get into that invalid state in the first place?

It will make for slightly slower tests, but I would suggest using reflection to put your system into the invalid state in order to test it.

Also notice that reflection is precisely one of the possible ways for your system to get into that invalid state, which will execute your supposedly impossible to reach code.

If your code was truly impossible to reach, which is not quite the case in your example as far as I see, then you should remove the code. Eg:

public int GetValue() {
   return 5;
   throw new InvalidOperationException("Somehow the return statement did not work!");
}

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

What do you mean by "should" here? Like, that it ought not to be executed? Surely it should be ok for the code to be executed in a test. It should be executed in a test.

Nynorsk