CODEWITHSUNDEEP
unit-testing
moarboilerplate
moarboilerplate

Reputation: 1643

unit test all permutations or start publicly exposing more? Where's the line?

I know that there have been a few posts about this already but I wanted to post one with a concrete example to focus on the gray areas you face when choosing between testing private/internal methods and refactoring into a public class.

Say I have a simple class I want to test that has some internal code refactored into a private or internal method.

Example:

public class Guy
{
    public void DoSomeWork()
    {
        try
        {
            //do some work
        }
        catch(Exception e)
        {
            LogError(e.Message);
        }

        try
        {
            //do some more work
        }
        catch(SomeSpecificException e)
        {
            LogError(e.Message);
        }
    }
    private void LogError(string message)
    {
        //if logging folder doesn't exist, create it
        //create a log file
        //log the message
    }
}

Most people would advise that I should take the error logging logic out and put it in a class marked public because it's starting to be complex enough to stand on its own, but the assembly's internal error logging logic shouldn't be exposed publicly--it's not a logging assembly.

If I don't do that, then every time I have code that could call LogError(), I need to have subsequent tests that retest all of the internal logic for LogError(). That quickly becomes oppressive.

I also understand I can mark the method as internal and make it visible to a testing assembly but it's not very "pure."

What sort of guidelines can I abide by in this type of situation?

Upvotes: 1

Views: 359

Answers (2)

Doug R
Doug R

Reputation: 5779

One possible way to handle this is instead of logging the error in your DoSomeWork method, you simply raise the error, and let it get handled upstream. Then you test that the error was raised, and for the purposes of testing the DoSomeWork method, you don't care what happens after that.

Whether or not this is a good idea depends a lot on how the rest of your program is structured, but having LogError sprinkled all over the place may indicate you need a broader error handling strategy that this approach requires.

UPDATE:

public void DoSomeWork() {
  try {
    WorkPartA()
  }
  catch(Exception e) {
    LogError(e.Message)
  }

  try {
    WorkPartB()
  }
  catch(SomeSpecificException e) {
    LogError(e.Message)
  }
}

public void WorkPartA() {
  //do some work
}

public void WorkPartB() {
  //do some more work
}

As a response to your comment below, the problem might be that you need to break up your code into smaller pieces. This approach allows you to test raising the exception within WorkPartA() and WorkPartB() individually, while still allowing the process to run if WorkPartA fails. As far as testing DoSomeWork() in this case, I might not even bother. There's a high probability it would be a very low-value test anyway.

Upvotes: 1

k.m
k.m

Reputation: 31454

but the assembly's internal error logging logic shouldn't be exposed publicly

That's true. It can be internal and tested as such. But this

private void LogError(string message)
{
    //if logging folder doesn't exist, create it
    //create a log file
    //log the message
}

Calls for a separate component. Create folder? Create file? Whatever DoSomeWork does, it should have nothing to do with creating log file internals. All it should require is simple "log this message"-feature. Files, folders, formats -- it's all specific to logging component and shouldn't leak to any other.

Your sample code is perfect example of single responsibility principle violation. Extracting logging to fix it solves your testing issues completely.

Upvotes: 2

Related Questions