6

I like to write classes with two constructors: a primary constructor used in production code, and a default constructor just for unit tests. I do this because the the primary constructor creates other concrete dependencies that consume external resources. I can't do all that in a unit test.

So my classes look like this:

public class DoesSomething : BaseClass
{
    private Foo _thingINeed;
    private Bar _otherThingINeed;
    private ExternalResource _another;

    public DoesSomething()
    {
        // Empty constructor for unit tests
    }

    public DoesSomething(string someUrl, string someThingElse, string blarg)
    {
         _thingINeed = new Foo(someUrl);
         _otherThingINeed = Foo.CreateBar(blarg);
         _another = BlargFactory.MakeBlarg(_thingINeed, _otherThingINeed.GetConfigurationValue("important");
    }
}

This is a pattern I follow with many of my classes so that I can write unit tests for them. I always include the comment in the default constructor so others can tell what it's for. Is this a good way to write testable classes or is there a better approach?


It was suggested that this might be a duplicate of "Should I have one constructor with injected dependencies and another that instantiates them." It's not. That question is about a different way to create the dependencies. When I use the default unit test constructor the dependencies don't get created at all. Then, I make most methods public and they don't use the dependencies. Those are the ones I test.

  • 8
    "Real" constructors are excluded from code coverage since they can't be tested. If they can't be tested, then you should make them testable. I hope this post is a joke because the premise is absurd. – Dan Wilson Jan 19 at 18:27
  • If you were to guess what my motivation is for asking and answering this question, it is 99% likely that you would guess correctly. Nonetheless, other than a bit of obfuscation, this is as real as it gets. I suppose I could trim some extraneous details. – Scott Hannen Jan 19 at 20:36
  • Bad ideas don't necessarily make bad questions. Not so sure a down-vote on the question is justified. – Greg Burghardt Jan 20 at 1:50
  • @GregBurghardt - I updated the question to show the difference between this question and the possible duplicate. – Scott Hannen Jan 20 at 12:26
  • There is absolutely nothing wrong with what you’re doing. It can greatly simplify your code and avoid the complexity of adding an IoC container to your project. – RubberDuck Jan 20 at 16:56
16

Never, ever create separate code paths for unit testing. Doing so defeats the purpose of unit testing because you're testing one code path in your unit tests and executing a different one in production.

For some reason that seems insufficient because I'm just repeating parts of your question back to you. You already know you're executing different code in your unit tests. The comment you put in your constructor says that. It's like asking if it's appropriate to take someone else's cell phone because it belongs to them, not to you, and you want to have it.

Would an argument from consensus help? Don't do that because no one else does it. Someone who knows better will be shocked. Even worse, someone who doesn't know better might learn from it, which would be bad, because... I feel like I'm still going in circles. But hopefully it carries some weight that other people say you shouldn't do that.

Perhaps I could show you that there is a better way: Dependency inversion and dependency injection. Dependency inversion means that you depend on abstractions (like interfaces) instead of concrete implementations. That's impossible if your class is creating its own dependencies, which is where dependency injection comes in.

Instead of creating instances of dependencies, you define abstractions that describe how your class needs to use them. For example, if ExternalResource is something you download files from, you could create an interface like this:

public interface IFileStorage
{
    byte[] GetFile(string filePath);
}

Then you create your class like this:

public class DoesSomething : BaseClass
{
    private readonly IFileStorage _fileStorage;

    public DoesSomething(IFileStorage fileStorage)
    {
        _fileStorage = fileStorage;
    }
}

Now you don't need a separate constructor. Your unit tests can call the real constructor and pass in a mock or a test double, which means an implementation of IFileStorage that returns exactly what you want it to. That way you can write tests for how your class behaves if the file contains this or that or nothing at all, without using an actual concrete implementation of IFileStorage.

For example, what if you want to see how your class behaves if IFileStorage returns an empty byte array? You could use a framework like Moq or just do this:

public class FileStorageThatReturnsEmptyArray : IFileStorage
{
    public byte[] GetFile(string filePath)
    {
        return new byte[] {};
    }
}

Then your test subject looks like this:

var subject = new DoesSomething(new FileStorageThatReturnsEmptyArray());

Isn't that better? Your unit tests are worth something because they test your class end-to-end, allowing it to behave exactly as it does in production.

True, in production you'll have a concrete implementation of IFileStorage instead of a test double. But the point is that to DoesSomething it's all the same. One IFileStorage is the same as another.

Is that a contradiction, since I just said that you should test the "real" code, but now I'm recommending using mocks? No. The question was about unit tests, which means that you wanted to test DoesSomething, so you would use mocks to test that class. You could write additional unit tests for other classes or integration tests to test them together. In the scope of this answer, if you're going to test DoesSomething then the point is that you wouldn't want to deceive yourself by testing a non-production method of that class, thinking that it's the same as testing the production method. ("Production" and "Non-production" methods aren't even a thing.)

I hope that showing how other developers solved this problem helps.

  • There’s another problem with the unit-test-only default ctor. It adds an opportunity to screw up. If production code erroneously uses the default ctor, then what? You’d have to introduce more tests somewhere else that reliably catch the problems arising from using those improperly created objects. – besc Jan 20 at 9:15
  • You are no longer deceiving yourself by testing a non-production method but by testing non-production arguments to the method. I disagree that this is better than before. I'd argue it's even worse, because dependency injection has its own share of liabilities (it makes the architecture more complicated). – Christian Hackl Jan 21 at 6:33
  • 3
    It's like asking if it's appropriate to take someone else's cell phone because it belongs to them, not to you, and you want to have it. I'd say a more apt analogy is that when you want to see if your phone still works, you use someone else's and (wrongly) use that to conclude that your own phone is probably still working as well. There may be some overlap (e.g. if you use the same provider, you have indeed proven that the provider isn't down), but you haven't actually tested you own phone's hardware, which means you didn't test the full chain. – Flater Jan 21 at 7:32
2

I'm on the fence whether or not this answer is any different than Scott Hannen's, but there is a difference in terminology and intent.


With some inspiration (and copying and pasting) from What's the difference between a mock & stub?, you are testing code that does not use the dependencies passed in to the constructor. What you are looking for is a Dummy Object.

From Mocks Aren't Stubs by Martin Fowler (emphasis, mine):

  • Dummy objects are passed around but never actually used. Usually they are just used to fill parameter lists.
  • Fake objects actually have working implementations, but usually take some shortcut which makes them not suitable for production (an in memory database is a good example).
  • Stubs provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in for the test.
  • Spies are stubs that also record some information based on how they were called. One form of this might be an email service that records how many messages it was sent.
  • Mocks are what we are talking about here: objects pre-programmed with expectations which form a specification of the calls they are expected to receive.

You have a constructor. It needs to be passed an object. Your test is not using that object. You need a Dummy object that contains the same public interface as the real dependency, but doesn't actually do anything — in fact it can throw exceptions whenever a method gets called on it.

I'll use your code example to show the way forward from here.

First, you have a constructor that does some more stuff than it really should. It's time to refactor this to create a new constructor that allows you to pass in the real objects:

public class DoesSomething : BaseClass
{
    private Foo _thingINeed;
    private Bar _otherThingINeed;
    private ExternalResource _another;

    public DoesSomething(string someUrl, string someThingElse, string blarg)
        : this(new Foo(someUrl), Foo.CreateBar(blarg), BlargFactory.MakeBlarg(_thingINeed, _otherThingINeed.GetConfigurationValue("important"))
    {
    }

    public DoesSomething(Foo thing1, Bar thing2, ExternalResource thing3)
    {
         _thingINeed = thing1;
         _otherThingINeed = thing2;
         _another = thing3;
    }
}

Note the differences:

  1. There is no empty constructor
  2. The signature for the existing constructor still exists, supporting backwards compatibility with existing code
  3. A new constructor was added that takes in a Foo, Bar and ExternalResource
  4. The old constructor calls the new constructor, ensuring initialization logic isn't being duplicated

I'm assuming ExternalResource is a little complicated to create. You need an interface for this depenency, which translates to another change:

public class DoesSomething : BaseClass
{
    // ...

    public DoesSomething(Foo thing1, Bar thing2, IExternalResource thing3)
    {
        // ...
    }
}

public interface IExternalResource
{
    // ...
}

public class ExternalResource : IExternalResource
{
    // ...
}

Then you need to ensure Foo and Bar do not do any actual work in their constructors. Furthermore, if Foo and Bar are not used by any instance methods of this class, then they should not be kept as fields! This would further simplify the dependencies of this class, making it even easier to mock in unit testing.

So that really just leaves you with creating an empty, dumb object:

internal class DummyExternalResource : IExternalResource
{
    public void Something()
    {
        throw new NotImplementedException();
    }
}

Just create a dummy object with all the necessary methods of IExternalResource which just throw exceptions. Then in your tests:

// Arrange
var foo = new Foo("fakeFile.text");
var bar = new Bar("fake blarg");
var externalResource = new DummyExternalResource();
var something = new DoesSomething(foo, bar, externalResource);

// Act
something.Bazz();

// Assert
Assert.IsTrue(something.FooBar);

If in the future a code change requires calling a method on IExternalDependency then you will get a failing test, as a new dependency for the method under test has been introduced. This should force developers to reassess the test coverage of this method.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.