What are the possible problems with unit testing ASP.NET MVC code in the following way?

Question

I've been looking at the way unit testing is done in the NuGetGallery. I observed that when controllers are tested, service classes are mocked. This makes sense to me because while testing the controller logic, I didn't want to be worried about the architectural layers below. After using this approach for a while, I noticed how often I was running around fixing my mocks all over my controller tests when my service classes changed. To solve this problem, without consulting people that are smarter than me, I started writing tests like this (don't worry, I haven't gotten that far):

public class PersonController : Controller
{
    private readonly LESRepository _repository;

    public PersonController(LESRepository repository)
    {
        _repository = repository;
    }

    public ActionResult Index(int id)
    {
        var model = _repository.GetAll<Person>()
            .FirstOrDefault(x => x.Id == id);

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

public class PersonControllerTests
{
    public void can_get_person()
    {
        var person = _helper.CreatePerson(username: "John");
        var controller = new PersonController(_repository);
        controller.FakeOutContext();

        var result = (ViewResult)controller.Index(person.Id);
        var model = (VMPerson)result.Model;
        Assert.IsTrue(model.Person.Username == "John");
    }
}

I guess this would be integration testing because I am using a real database (I'd prefer an inmemory one). I begin my test by putting data in my database (each test runs in a transaction and is rolled back when the test completes). Then I call my controller and I really don't care how it retrieves the data from the database (via a repository or service class) just that the Model to be sent to the view must have the record I put into the database aka my assertion. The cool thing about this approach is that a lot of times I can continue to add more layers of complexity without having to change my controller tests:

public class PersonController : Controller
{
    private readonly LESRepository _repository;
    private readonly PersonService _personService;

    public PersonController(LESRepository repository)
    {
        _repository = repository;
        _personService = new PersonService(_repository);
    }

    public ActionResult Index(int id)
    {
        var model = _personService.GetActivePerson(id);
        if(model  == null)
          return PersonNotFoundResult();

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

Now I realize I didn't create an interface for my PersonService and pass it into the constructor of my controller. The reason is 1) I don't plan to mock my PersonService and 2) I didn't feel I needed to inject my dependency since my PersonController for now only needs to depend on one type of PersonService.

I'm new at unit testing and I'm always happy to be shown that I'm wrong. Please point out why the way I'm testng my controllers could be a really bad idea (besides the obvious increase in the time my tests will take to run).

Answer 1

Hmm. a few things here mate.

First, it looks like you're trying to test the a controller method. Great :)

So this means, that anything the controller needs, should be mocked. This is because

1. You don't want to worry about what happens inside that dependency.
2. You can verify that the dependency was called/executed.

Ok, so lets look at what you did and I'll see if i can refactor it to make it a bit more testable.

-REMEMBER- i'm testing the CONTROLLER METHOD, not the stuff the controller method calls/depends upon.

So this means I don't care about the service instance or the repository instance (which ever architectural way you decide to follow).

NOTE: I've kept things simple, so i've stripped lots of crap out, etc.

Interface

First, we need an interface for the repository. This can be implemented as a in-memory repo, an entity framework repo, etc.. You'll see why, soon.

public interface ILESRepository
{
    IQueryable<Person> GetAll();
}

Controller

Here, we use the interface. This means it's really easy and awesome to use a mock IRepository or a real instance.

public class PersonController : Controller
{
    private readonly ILESRepository _repository;

    public PersonController(ILESRepository repository)
    {
       if (repository == null)
       {
           throw new ArgumentNullException("repository");
       }
        _repository = repository;
    }

    public ActionResult Index(int id)
    {
        var model = _repository.GetAll<Person>()
            .FirstOrDefault(x => x.Id == id);

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

Unit Test

Ok - here's the magic money shot stuff. First, we create some Fake People. Just work with me here... I'll show you where we use this in a tick. It's just a boring, simple list of your POCO's.

public static class FakePeople()
{
    public static IList<Person> GetSomeFakePeople()
    {
        return new List<Person>
        {
            new Person { Id = 1, Name = "John" },
            new Person { Id = 2, Name = "Fred" },
            new Person { Id = 3, Name = "Sally" },
        }
    }
}

Now we have the test itself. I'm using xUnit for my testing framework and moq for my mocking. Any framework is fine, here.

public class PersonControllerTests
{
    [Fact]
    public void GivenAListOfPeople_Index_Returns1Person()
    {
        // Arrange.
        var mockRepository = new Mock<ILESRepository>();
        mockRepository.Setup(x => x.GetAll<Person>())
                                   .Returns(
                                FakePeople.GetSomeFakePeople()
                                          .AsQueryable);
        var controller = new PersonController(mockRepository);
        controller.FakeOutContext();

        // Act.
        var result = controller.Index(person.Id) as ViewResult;

        // Assert.
        Assert.NotNull(result);
        var model = result.Model as VMPerson;
        Assert.NotNull(model);
        Assert.Equal(1, model.Person.Id);
        Assert.Equal("John", model.Person.Username);

        // Make sure we actually called the GetAll<Person>() method on our mock.
        mockRepository.Verify(x => x.GetAll<Person>(), Times.Once());
    }
}

Ok, lets look at what I did.

First, I arrange my crap. I first create a mock of the ILESRepository. Then i say: If anyone ever calls the GetAll<Person>() method, well .. don't -really- hit a database or a file or whatever .. just return a list of people, which created in FakePeople.GetSomeFakePeople().

So this is what would happen in the controller ...

var model = _repository.GetAll<Person>()
                       .FirstOrDefault(x => x.Id == id);

First, we ask our mock to hit the GetAll<Person>() method. I just 'set it up' to return a list of people .. so then we have a list of 3 Person objects. Next, we then call a FirstOrDefault(...) on this list of 3 Person objects .. which returns the single object or null, depending on what the value of id is.

Tada! That's the money shot :)

Now back to the rest of the unit test.

We Act and then we Assert. Nothing hard there. For bonus points, I verify that we've actually called the GetAll<Person>() method, on the mock .. inside the Controller's Index method. This is a safety call to make sure our controller logic (we're testing for) was done right.

Sometimes, you might want to check for bad scenario's, like a person passed in bad data. This means you might never ever get to the mock methods (which is correct) so you verify that they were never called.

Ok - questions, class?

Answer 2

Even when you do not plan to mock an interface, I strongly suggest you to do not hide the real dependencies of an object by creating the objects inside the constructor, you are breaking the Single Responsibility principle and you are writing un-testable code.

The most important thing to consider when writing tests is: "There is no magic key to write tests". There are a lot of tools out there to help you write tests but the real effort should be put in writing testable code rather than trying to hack our existing code to write a test which usually ends up being an integration test instead of a unit-test.

Creating a new object inside a constructor is one of the first big signals that your code is not testable.

These links helped me a lot when I was making the transition to start writing tests and let me tell you that after you start, that will become a natural part of your daily work and you will love the benefits of writing tests I can not picture myself writing code without tests anymore

Clean code guide (used in Google): http://misko.hevery.com/code-reviewers-guide/

To get more information read the following:

http://misko.hevery.com/2008/09/30/to-new-or-not-to-new/

and watch this video cast from Misko Hevery

http://www.youtube.com/watch?v=wEhu57pih5w&feature=player_embedded

Edited:

This article from Martin Fowler explain the difference between a Classical and a Mockist TDD approach

http://martinfowler.com/articles/mocksArentStubs.html

As a summary:

• Classic TDD approach: This implies to test everything you can without creating substitutes or doubles (mocks, stubs, dummies) with the exception of external services like web services or databases. The Classical testers use doubles for the external services only

  • Benefits: When you test you are actually testing the wiring logic of your application and the logic itself (not in isolation)
  • Cons: If an error occurs you will see potentially hundreds of tests failing and it will be hard to find the code responsible

• Mockist TDD approach: People following the Mockist approach will test in isolation all the code because they will create doubles for every dependency

  • Benefits: You are testing in isolation each part of your application. If an error occurs, you know exactly where it occurred because just a few tests will fail, ideally only one
  • Cons: Well you have to double all your dependencies which makes tests a little bit harder but you can use tools like AutoFixture to create doubles for the dependencies automatically

This is another great article about writing testable code

http://www.loosecouplings.com/2011/01/how-to-write-testable-code-overview.html