How to test or exclude private unreachable code from code coverage

Question

I have a bunch of assemblies with near 100% test coverage but I often run into a situation like in the example below. I cannot test the default switch case, which is there to guard against future bugs where I add more items to the enum but forget to update the switch statement to support new items.

I would like to be able to either find a pattern where I can eliminate that "untestable" code, test it or mark that line of code (but not the entire method) to be excluded by the coverage analysis.

It may sound silly but I do not want to assume the default case will never happen, and I do not want to bundle the default case with one that already exist. I want the exception to be thrown when I create such bugs. Which will happen sooner or later.

I use DotCover to calculate coverage at the moment.

Note: This is just example code but I think it illustrates a fairly common pattern.

public class Tester
{
    private enum StuffToDo
    {
        Swim = 0, 
        Bike,
        Run
    }

    public void DoSomeRandomStuff()
    {
        var random = new Random();
        DoStuff((StuffToDo)random.Next(3));
    }

    private void DoStuff(StuffToDo stuff)
    {
        switch (stuff)
        {
            case StuffToDo.Swim:
                break;
            case StuffToDo.Bike:
                break;
            case StuffToDo.Run:
                break;
            default:
                // How do I test or exclude this line from coverage?
                throw new ArgumentOutOfRangeException("stuff");
        }
    }
}

Smoothspoken

Possible duplicate of #113895 – Superdominant

Do you really need 100%? (~100% is already very good) – Punishment

@DavidL No, how is that question the same as mine? It discusses expected exceptions in unit tests. – Andrel

@vc74 No, but I would like it if possible. It is very easy to see if someting new is not tested if the normal case is 100%. However, this specific case has been bugging me for a while. – Andrel

Why have that line of code in the first place - are you expecting that random.Next(3) will produce a value outside the range of 0-2? – Trail

The default case is there in case I update my random logic to be 0-3 for example. However, it is of course a somewhat contrived example but shows a pattern I often run into. A more general case is perhaps code translating one enum to another. I am always surprised how often I hit those "impossible" cases, especially during development. – Andrel

Even though this is just an example, I disagree that it is a fairly common pattern. Each scenario has to be taken individually. In this specific case, the source of your pain is the dependency on Random. I would prefer to inject an abstracted random number generator (preferably Constructor). – Carlynne

@JakobMöllås what Kelly mentions here is the main reasoning behind my answer. You may think this is the same as other code where you have these switch statements, but it is very telling you used a Random in the example i.e. the class has a dependency that introduces extra trouble with the tests. – Clishmaclaver

Ecery single comment/answer that I have seen misses the point... What if a new enum value is added.. but the switch statement is not updated...We want to FAIL in that case by throwing the exception... but unless there is an error in updating the switch statement there is no way to actually reach this code... hence the dilema... looking at DoSomeRandomStuff has absolutely NO bearing on this. – Clamshell

Answer 1

Private methods are always candidates to be extracted into its own classes. Especially, if they hold complex logic, like yours do. I suggest you make a StuffDoer class with the DoStuff() as a public method and inject it into your Tester class. Then you have:

• A DoStuff() method, reachable by tests
• A DoSomeRandomStuff() method that can be tested with a mock instead of relying on the outcome of DoStuff(), thus making it a true unit test.
• NO SRP-violation.
• All in all, nice, crisp and SOLID code.

Degenerate

Agreed, that is what I usually do (with or without the injection). However, depending on the source of the enums (shared in a proto definition for example) there may be a lot of small methods like this. Sometimes the code will simply be more complicated and harder to maintain by creating those extra methods and classes ("Doers"). – Andrel

@JakobMöllås: In my experience, all those small methods make a mess of a class with a lot of code that can't be reached by tests. With careful naming, namespace hierarchies and folder structure, I prefer small classes that respect SRP, and are easily tested. IMHO, proper unit testing is, by far, the biggest contributor to maintainable code. – Degenerate

+1 I really agree with this. In the practical side is hard to pull off on a rush when dealing with legacy code bases. The stuff in my answer is usually an intermediate step to quickly be able to test the legacy code. – Clishmaclaver

This is good advice, but doesn't solve the original problem: default values on enum switches aren't easy to cover in unit tests. Switching to a Strategy pattern just moves the problem over to the creational side. The Wikipedia entry for Factory Method or Abstract Factory suffers from the same issue. – Putup

Answer 2

Update: given the answers in here, it seems some important clarifications are needed.

1. an enum doesn't prevent you from reaching the default clause, this is a valid way to reach it: DoStuff((StuffToDo)9999)
2. the default clause isn't required in a switch statement. In that case, the code would just silently continue after the switch
3. If the method was internal, you could use InternalsToVisible to unit test it directly
4. You could refactor the code in such a way that the strategy handled in the switch can be reached by the unit tests, like @Morten mentions on his answer
5. Random, dates and similar are a typical problem for unit tests, and what you do is to change the code in such a way that those can be replaced during unit tests. My answer is just one way of the many ways to do so. I show how you can inject it via a field member, but it could be via the constructor, and it can also be an interface if desired.
6. If the enum wasn't private, my code sample below could instead expose a Func<StuffToDo>, which would make the tests a bit more readable.

---

You could still do this and get to 100%:

public class Tester
{
    private enum StuffToDo
    {
        Swim = 0, 
        Bike,
        Run
    }
    public Func<int> randomStuffProvider = GetRandomStuffInt;

    public void DoSomeRandomStuff()
    {
        DoStuff((StuffToDo)randomStuffProvider());
    }
    private static int GetRandomStuffInt()
    {
        //note: don't do this, you're supposed to reuse the random instance
        var random = new Random();
        return random.Next(3);
    }

    private void DoStuff(StuffToDo stuff)
    {
        switch (stuff)
        {
            case StuffToDo.Swim:
                break;
            case StuffToDo.Bike:
                break;
            case StuffToDo.Run:
                break;
            default:
                // How do I test or exclude this line from coverage?
                throw new ArgumentOutOfRangeException("stuff");
        }
    }
}

ps. yes it exposes the RandomStuffProvider so that you can use in an unit test. I didn't make it Func<StuffToDo> because that's private.

Clishmaclaver

I am not sure I understand how this would solve anything, the provider in your example simply returns a random integer, right? – Andrel

Right, that is the current behavior. On your unit tests you can override it to return a fixed value. This allows you to have reliable tests for the 0-2 values, plus you can set it to a higher value which will cause your default clause to be hit. – Clishmaclaver

Ah, I guess you meant to to something like DoStuff((StuffToDo)RandomStuffProvider());? Otherwise nothing will change as far as I can see. It would however add this somewhat wierd-looking provider to the public interface, which is not something I fancy too much. – Andrel

Oops, yes, that is exactly what I meant. It is true it makes this random generator dependency visible. Note another option is making it a constructor parameter. In all cases you really want a way to set a predictable value for the random in your unit tests, otherwise you can't really check for a predictable output. – Clishmaclaver

Yes, that is what I want, however the random behavior is just an example. It could simply be a common translate-one-enum-to-another operation or something similar. – Andrel

I get bloated by just looking at this – Plank

-1 because I don't think that you have understood properly the question. It is not a matter of being truly important or not; but a matter of answering a question by delivering an answer theoretically meeting its requirements. Your answer is just saying "don't worry, everything is fine" what, IMO, is not an answer at all. NOTE: To be honest I thought that -1 other's question in a bounty wasn't the right thing to do (I am new here) and that's why I didn't do this before; but you have proven me wrong and that's why I am doing it now. – Emarie

My answer is showing a quick way you can test even legacy code bases. For the right thing to do look at @Morten's answer. – Clishmaclaver

@varocarbas about the -1 votes on a bounty. It makes sense if you strongly believe the answer will mislead people that come later looking for an answer to some problem. Specially if there is a better answer (like the one from Morten). Also normally if both answers are at the same vote level, you'd just upvote the one you think should be there. But if there is a 2 votes difference, it makes sense to do it (so now both answers are at the same level). – Clishmaclaver

@Clishmaclaver are you saying that my answer is misleading and that should be deleted? That it does not provide any valuable information (even right the contrary)? I am pretty new on this answering world and you seem to have quite long experience on it. If you can explain your point properly and prove that the proposed alternative (of pre-checking inputs; and, eventually, linking inputs->behaviours) is not better that the original trial and error + reliance on a default case, from a certainty/theoretical point of view, I might consider the deletion of my answer. – Emarie

@varocarbas the thing is that the top answer is regarded as the solution to the OP's question. Your answer presents some good refactoring techniques, that are very helpful when dealing with this type of code, so it makes for a good read. The reason I view as problematic in the top spot, is because in my experience the change on behavior leads to big trouble in the wild; some people just see the top answer and apply that to their environments without understanding well the implications. re pre-checking, technically the case is doing that, in a way that can be optimized by the compiler. – Clishmaclaver

@varocarbas re linking inputs->behaviours. As I mentioned before " the last sample is usually good for this type of code (for different reasons)". It's clearly very readable when you're dealing with lists of actions, and I have used that and similar in several cases. The original question remains though, how do we reach the branch of code that will throw an exception, which would be in the else of this check if(accountedStuff.ContainsKey(stuff)) if you don't change behavior. – Clishmaclaver

Answer 3

You can use pre-compiler directive using DEBUG like this (or create your own):

#if DEBUG
    public void DoStuff(StuffToDo stuff)
#else
    private void DoStuff(StuffToDo stuff)
#endif
    {
        switch (stuff)
        {
            case StuffToDo.Swim:
                break;
            case StuffToDo.Bike:
                break;
            case StuffToDo.Run:
                break;
            default:
                // How do I test or exclude this line from coverage?
                throw new ArgumentOutOfRangeException("stuff");
        }
    }

Anabiosis

Although I don't find this pretty, If I was forced to use it I would made it using internal access modifier, sign my assemblies and use InternalsVisibleTo to allow access to test classes only. – Inclose

+1, great approach. I would use a custom variable and put the compiler directives around the default or whatever smallest area of code needed to be excluded, not around method definitions as that is harder to read and maintain – Plank

Answer 4

Just do this:

private static int GetRandomStuffInt()
{
    var random = new Random();
    DoStuff((StuffToDo)random.Next(Enum.GetNames(typeof(StuffToDo)).Length));
}

This will ensure that the number it returns is part of the enum, and therefore will never even reach the default: switch. (Link)

Moye

Thanks, although as stated the problem is not really the exact example code but the problem of how to test out-of-range situations inside the switch of the DoStuff method (or others like it). – Andrel

Well with this code you won't get out-of-range circumstances, at least your not supposed to, so there might be no reason to check for them. Anyways, I guess that wasn't the question. – Moye

Except that it will reach the default case when someone down the road adds a new value to the enum. – Clishmaclaver

Did you check the code? It's basically an array, and when a value is added or removed, the Length of the array changes accordingly. This isn't a list though, so you cannot add or remove items after compiling. Someone correct me if I'm wrong. – Moye

Getting a random enum like this does not work if you are using enums with non-standard ordering.. such as if you want flags, or breaks in your numbers for some other reason. ex. a = 1, b = 3, c = 5. – Betjeman

Answer 5

One way to test private methods in unit test case is using reflection but I feel that might be a over kill in most pf the situation where its possible to test with other ways. In your code, to test the switch case, what you can do is, if your class method which is public is taking StuffToDo as a parameter then you need to write multiple test cases with each passing a different set of value for StuffToDo. This way when your switch statement is executed you can verify the behaviour using your public method itself, but again in this case I am assuming you are getting a output from your public method.

Looking at your code another feeling I get is, your public method is not taking any input and not giving any out but doing modification that does not look right. It looks like it silently changing things which can confuse.

Try having more specific method which clearly states what it is taking as input and how modifying that.

Apiary

Thanks, although I do not want to change the signature, that is one of the reasons I have this issue. In reality my problem is more complicated than my example of course and sometimes involves multiple projects in multiple languages. The enum definition could be shared and written in a meta-language, later getting compiled to C# and involving a manual step to update the code to match the latest definition. Stuff like that is hard to automate and get around. In general, the simple solution is to break out the private method into an internal method which enables me to test all cases. – Andrel

Answer 6

The "problem" you refer is intrinsic to switch...case and thus the best thing you can do to avoid it is relying on a different alternative. I personally don't like too much switch...case because of its inflexibility. Thus, this answer intends to provide an alternative to switch statements capable to deliver the kind of zero-uncertainty you are looking for.

In this kind of situations, I usually rely on a set of conditions taken from an array populated at the start of the function which will automatically filter any unaccounted input. Example:

private void DoStuffMyWay(StuffToDo stuff)
{
    StuffToDo[] accountedStuff = new StuffToDo[] { StuffToDo.Bike, StuffToDo.Run, StuffToDo.Swim };

    if (accountedStuff.Contains(stuff))
    {
        if (stuff == accountedStuff[0])
        {
            //do bike stuff
        }
        else if (stuff == accountedStuff[1])
        {
            //do run stuff
        }
        else if (stuff == accountedStuff[2])
        {
            //do swim stuff
        }
    }
}

This is certainly not too elegant, but I guess that I am not an elegant programmer.

As far as perhaps you prefer a different type of approach to the problem, here you have another alternative; it is less efficient but looks nicer:

private void DoStuffWithStyle(StuffToDo stuff)
{
    Dictionary<StuffToDo, Action> accountedStuff = new Dictionary<StuffToDo, Action>();
    accountedStuff.Add(StuffToDo.Bike, actionBike);
    accountedStuff.Add(StuffToDo.Run, actionRun);
    accountedStuff.Add(StuffToDo.Swim, actionSwim);

    if (accountedStuff.ContainsKey(stuff))
    {
        accountedStuff[stuff].Invoke();
    }
}

private void actionBike()
{
    //do bike stuff
}

private void actionRun()
{
    //do run stuff
}

private void actionSwim()
{
   //do swim stuff
}

With this second option, you might even create the dictionary as a global variable and populate it together with the enum.

Emarie

imho, this is not the right answer: 1. both approaches are changing behavior (unsupported values just silently fail, which in my experience is worst) 2. the OP could just remove the default clause, and it'd be exactly the same 3. the first code sample shouldn't be doing .ToString, just store the enum values in the array and compare enums 4. on the later code, you don't need the explicit new Action, you can just say accountedStuff.Add(StuffToDo.Bike, actionBike);. That said, the last sample is usually good for this type of code (for different reasons) – Clishmaclaver

Unsupported values just silently fail? How can this be true? You are saying that each condition used in a code which is not met implies a failure? I thought that true/false were the two posible results for a condition and thus none of them might be considered the trigger of an error. Could you please explain your point better? – Emarie

2. the OP could just remove the default clause, and it'd be exactly the same -> it would be exactly the same from a practical point of view but not from the theoretical point of view asked in this question; that is, some cases (the default ones) wouldn't be accounted for (unlikely in a condition where "not being there" makes them accounted). – Emarie

3. the first code sample shouldn't be doing .ToString, just store the enum values in the array and compare enums -> You are right but I wasn't expecting this to be considered as an error (not even a half error). – Emarie

4. on the later code, you don't need the explicit new Action, you can just say accountedStuff.Add(StuffToDo.Bike, actionBike); -> OK, same thing that for the previous point; not aware that this might be considered as something wrong. – Emarie

the main thing is point 1 and 2. The OP explicitely stated it in the question: "it may sound silly but I do not want to assume the default case will never happen, and I do not want to bundle the default case with one that already exist. I want the exception to be thrown when I create such bugs. Which will happen sooner or later.". The OP is right, later on someone will add another value to an enum, and miss updating something. No exception means it'll go unnoticed, so it ends up in bigger trouble. – Clishmaclaver

@Clishmaclaver I have edited by answer to account for your suggestions (the ones I considered relevant), thanks. In any case, I would love to hear a proper explanation of your "silently fail" point. – Emarie

I did understand the question: he looks for a way to make sure that any eventuality is (theoretically) under control. My answer is based on the idea that such an aim cannot be accomplished by relying on a switch...case statement which, by definition includes a default statement. What I proposed was an alternative delivering a 100% certainty: there is no default/unaccounted situation; the condition will always deliver certainty: if it is there, do that; if it is not there, trigger an error (via else); both behaviours (it is there or not) are the right/direct/certain outputs from the condition. – Emarie

So you're saying now, that you should have an else clause in your code to throw the exception (thus going back to the original problem)? But you're also saying my #2 in the original comment is wrong, I refer you to the switch reference: msdn.microsoft.com/en-us/library/06tc147t(v=vs.80).aspx "If there is no default label, control is transferred outside the switch.". I see, that's what you meant with "The "problem" you refer is intrinsic to switch...case"; well, no, it's not. – Clishmaclaver

The default case should take care of any problem, this is clear. Then, what is the reason for this question? From a theoretical point of view, making sure that any situation is accounted for, having complete certainty that the code is only executed when the given enum case is accounted for by the function. Practically my approach and a switch case do deliver the same answer? Yes. Theoretically is the switch case accounting for everything as the heading condition does? No. My approach checks whether the given input should be analysed or not; the switch case analyses it anyway. – Emarie

Answer 7

Write tests that test the out of range values and expect an exception.

Dielectric

And how do I best do that in my case? – Andrel

Well, you could add a sentinal value to StuffToDo, such as StuffToDo.None or something similar, then have that be the default value, though still an 'error state' value. – Dielectric

Yes, that will catch the error state as well but it does not always make sense to have a value like that. And it may not even be possible if the enum declaration is not originating in code I can modify. Even with a a default value (error value) I still want to test for it. – Andrel

Answer 8

I agree with Nate here above. That is probably the easiest (StuffToDo.None). As an alternative you could also overload the method like this:

    private void DoStuff(StuffToDo stuff)
    {
        DoStuff(stuff.ToString());
    }

    private void DoStuff(string stuff)
        switch (stuff)
        {
            case "Swim":
                break;
            case "Bike":
                break;
            case "Run":
                break;
            default:
                throw new ArgumentOutOfRangeException("stuff");
        }
    }

Then you can actually test the situation that you cover in your default statement.

Kalgan

you don't need to do that to be able to hit the default clause – Clishmaclaver

Answer 9

It is unreachable code, because the switch case type is Enum, And you have coded all possible cases. There will never be the case of 'Default'.

You can add 'Unknown' or some extra enum value which should not have a corresponding case statement, then this warning will go away.

private enum StuffToDo
{
    Swim = 0, 
    Bike,
    Run,
    // just for code coverage
    // will never reach here
    Unknown
}

Code Coverage does not go into depth of evaluating every possibility, so it is not question of testing it. It matches code pattern based on their precoded expected behaviour

Code Coverage thinks it this way, you have added all possibility of existing enum, there is no way default will be called. This is how their rule is setup, you cannot change their rule.

Spermic

Answer 10

My ReSharper also "complains" about unreachable code, which I don't like while programming. Call me anal, but I like to get rid of dead code from the moment I see it.

For your case specifically, I'd pick one of these cases as the default case, and put it to something like this:

private void DoStuff(StuffToDo stuff)
{
    switch (stuff)
    {
        case StuffToDo.Swim:
            break;
        case StuffToDo.Bike:
            break;
        case StuffToDo.Run:
        default:
            break;
    }
}

Since you strive to 100% test coverage, ideally, this should break when you add an enum entry that should be treated differently.

And if you really want to be sure, you can still uglify it:

private void DoStuff(StuffToDo stuff)
{
    switch (stuff)
    {
        case StuffToDo.Swim:
            break;
        case StuffToDo.Bike:
            break;
        case StuffToDo.Run:
        default:
            if (stuff != StuffToDo.Run) throw new ArgumentOutOfRangeException("stuff");
            break;
    }
}

Tremblay

the problem with the first code sample is that down the road when someone adds a new value, DoStuff will just run 'Run' and never report any problem. That makes it harder to find about the problem. On the second code sample, you have the same as the OP, a branch of code that needs to be hit by the unit tests. – Clishmaclaver