Is it bad to add code just for unit testing?

Question

I am writing a class to help me unit test my code. It looks like this:

/// <summary>
/// Wrapper for the LogManager class to allow us to stub the logger
/// </summary>
public class Logger
{
    private static ILogger _logger = null;

    /// <summary>
    /// This should be called to get a valid logger.
    /// </summary>
    /// <returns>A valid logger to log issues to file.</returns>
    public static ILogger GetLogger()
    {
        if (_logger == null)
          _logger = LogManager.GetLogger("logger");

        return _logger;
    }

    /// <summary>
    /// This is used by unit tests to allow a stub to be used as a logger.
    /// </summary>
    /// <param name="logger"></param>
    /// <returns></returns>
    public static ILogger GetLogger(ILogger logger)
    {
        _logger = logger;
        return _logger;
    }
}

The second method is for unit testing only. I never intend to have it called in my production code.

Is this bad practice? Should I find another way that does not do this?

Answer 1

In my opinion yes, this is a bad practice. Unit testing is meant to test implementation of your code, not really to influence it. In some cases I've found it practical to organize my code/methods in a manner to make it more easily/thoroughly tested, but writing code in a class being tested for specific use in testing is a different story.

Answer 2

Any time you have code in a file/library/whatever that is not intended to be used in a production capacity it is just code bloat. Your production objects should not be coded to support your unit tests, your unit tests should be implemented to support your production objects.

Answer 3

You could consider this:

/// <summary>
/// Wrapper for the LogManager class to allow us to stub the logger
/// </summary>
public class Logger
{
    public static ILogger _logger = null;

    /// <summary>
    /// This should be called to get a valid logger.
    /// </summary>
    /// <returns>A valid logger to log issues to file.</returns>
    public static ILogger Logger
    {
        get
        {
            if (_logger == null)
            {
                _logger = LogManager.GetLogger("logger");
            }
            return _logger
        }
        set
        {
            _logger = value;
        }
    }
}

It sets the logger the first time the getter is called if it's not already set using the setter.

Answer 4

I wouldn't say this particular piece of code is a problem as such. As long as you state in your xmldoc comments that the method(s) are intended for unit testing only, no (sane!) person would use it in production code.

Though, when possible, try to keep such things in your unit testing library. As Joel Etherton states, it is code bloat, in any case...

Answer 5

Normally, code that exists only to facilitate unit testing hides a design flaw.

In this case, the flaw you're hiding is the use of global state. Classes that use your logger are secretly dependent on it: you can't tell they need a logger unless you read their source code!

Those classes should require an ILogger in their constructors, making it obvious what they need to work and making them easy to test.

Answer 6

IMO, yes, this is bad code. Two reasons:

• Logger knows about LogManager. That tightly couples Logger to LogManager, and presents the problem you encounter; you have to use LogManager to get an ILogger object, and cannot stub or mock LogManager.
• You have added code to a production class for the sole purpose of unit testing. That will confuse developers down the road, who see the test hook and think they can use it in production code.

I would recommend one or more of the following:

• Define an interface ILogManager that exposes GetLogger(), and implement it on LogManager. Then, create a property on Logger of type ILogManager (or require one to be passed in when instantiating a Logger) and give Logger a LogManager from outside the scope of Logger (through an IoC framework, or simply from whatever instantiates Logger). This is loose coupling; you can now supply any class that implements ILogManager including MockLogManager, and Logger doesn't know the difference.
• For purposes of your unit test, derive a "proxy" from Logger and implement any test-only methods there. I use this method mainly to unit test protected methods of a class, but if you find yourself unable to decouple LogManager from Logger, you can at least hide this derived proxy and its hooks in your test assemblies, where production code shouldn't be able to reference them.

Answer 7

Yes, the design is not perfect :( GetLogger method actually sets the logger! Use setter method - this looks more suitable.

BTW, what do you test? If you use .NET standard loggers - you can add or delete listeners (as I remember) and if you don't need tests to write something somewhere - configure apropriate log listeners.

And have seen the Moq library? http://code.google.com/p/moq/ I liked it for mocking.

Answer 8

I don't have any strange constructors. Instead I have a private variable in the View:

private readonly ILog _log = LogManager.GetLogger(typeof(MyViewClassName));

Which can then be used as such, in this case it is called inside the catch block:

_log.Error("SomePage.aspx.cs Page_Load failed", ex);

There is no wrapper for log4net. ILog is part of log4net. I'm curious as to why you aren't using Mocks also. That would make sense and then you wouldn't need to do what you are doing.

Answer 9

To answer the general question of "Is it bad to add code just for unit testing?", no it's not bad in general. Testability is an important feature of a system because it helps guarantee the correctness of the system (through the actual tests) and it is highly correlated with other good design tenets, like loose coupling and high cohesion.

Look at it another way; if you have two similar ways of building some code, and the main difference is testability, then go for the more testable option.

But as for the specific case of your code, I agree with most of the other posts here; there is a better way to write that code.

Answer 10

What's wrong here is that the code that gets the logger has to know what logger it wants. In fact, it has no reason to care.

That's why this is a poster child for inversion of control. At the very simplest, stick the name of the logger in a config file and use a different value in your test code. At the more sophisticated level, use something like MEF to decide which logger to load.