14
\$\begingroup\$

Lately I have been researching how to best unit test an EF repository and given a properly tested repository, what to test in the controller.

My research did not reveal many sound examples or explanations but I did what I could. Now I am kindly asking that you review my tests in order to verify their correctness.

Example SUT

In order to demonstrate what I have tried so far and to give you folks something to go on - here is an example SUT.

I am coding a small blog engine. I have a controller by the name of PostsController whose role is obvious:

public class PostsController : Controller
{
    private readonly IPostsRepository repository;
    private readonly IMappingEngine mapper;

    public PostsController(IPostsRepository repository, IMappingEngine mapper)
    {
        this.repository = repository;
        this.mapper = mapper;
    }

    public ViewResult Index()
    {
        var posts = repository.GetAllPublished();

        var model =
            mapper.Map<IEnumerable<PostViewModel>>(posts);

        return View("Index", model);
    }
}

In an attempt to keep my controllers thin (and to achieve lose coupling) I introduced a repository by the name of PostsRepository who implements an interface called IPostsRepository:

public interface IPostsRepository
{
    IEnumerable<Post> All();
    IEnumerable<Post> GetAllPublished();
}

public class PostsRepository : IPostsRepository
{
    private readonly DatabaseContext context;

    public PostsRepository(DatabaseContext context)
    {
        this.context = context;
    }

    public IEnumerable<Post> All()
    {
        return 
            context.Posts
                   .OrderBy(post => post.PublishDate);
    }

    public IEnumerable<Post> GetAllPublished()
    {
        return 
            context.Posts
                   .Where(post => !post.Draft)
                   .OrderBy(post => post.PublishDate);
    }
}

Note that only one of the two methods defined by the repository interface are actually used by the controller in this example. I included the ineffectual method to better reflect a real-world example as that is important to me. I included the IMappingEngine abstraction (that comes from AutoMapper) for the same reason.

My best efforts at testing this system

Having amalgamated a number of articles and videos I came up with the following unit tests:

[TestFixture]
public class PostsControllerTest
{
    private PostsController controller;
    private Mock<IPostsRepository> repository;
    private Mock<IMappingEngine> mapper;

    [SetUp]
    public void SetUp()
    {
        repository = new Mock<IPostsRepository>();
        mapper = new Mock<IMappingEngine>();
        controller = new PostsController(repository.Object, mapper.Object);
    }

    [Test]
    public void Index_ReturnsCorrectViewName()
    {
        var actual = controller.Index();

        Assert.AreEqual("Index", actual.ViewName);
    }

    [Test]
    public void Index_UsesPostsRepository()
    {
        controller.Index();

        repository.Verify(repo => repo.GetAllPublished(), Times.Once);
    }

    [Test]
    public void Index_ReturnsCorrectModel()
    {
        var posts = Enumerable.Repeat(new PostViewModel(), 2).ToList();

        mapper.Setup(m => 
            m.Map<IEnumerable<PostViewModel>>(It.IsAny<IEnumerable<Post>>()))
              .Returns(posts);

        var actual = controller.Index().Model;

        Assert.IsAssignableFrom<List<PostViewModel>>(actual);
    }
}

[TestFixture]
public class PostsRepositoryTest
{
    private IQueryable<Post> posts;
    private Mock<DbSet<Post>> databaseSet;
    private Mock<DatabaseContext> databaseContext;
    private PostsRepository repository;

    public void SetUp()
    {
        databaseSet = new Mock<DbSet<Post>>();
        databaseSet.As<IQueryable<Post>>().Setup(query => query.Provider)
                                          .Returns(posts.Provider);
        databaseSet.As<IQueryable<Post>>().Setup(query => query.Expression)
                                          .Returns(posts.Expression);
        databaseSet.As<IQueryable<Post>>().Setup(query => query.ElementType)
                                          .Returns(posts.ElementType);
        databaseSet.As<IQueryable<Post>>().Setup(query => query.GetEnumrator())
                                          .Returns(posts.GetEnumerator);

        databaseContext = new Mock<DatabaseContext>();
        databaseContext.Setup(context => context.Posts)
                       .Returns(databaseSet.Object);

        repository = new PostsRepository(databaseContext.Object);
    }

    [Test]
    public void All_ReturnsAll()
    {
        // arrange
        posts = Enumerable.Repeat(new Post(), 2).AsQueryable();
        // Because we need to setup the seed data (which varies for each test)
        // before mocking the database objects, we cannot use the NUnit SetUp 
        // attribute and must instead call SetUp manually *after* we setup the
        // seed data. 
        SetUp();

        // act
        var actual = repository.All();

        // assert
        Assert.AreEqual(2, actual.Count());
    }

    [Test]
    public void All_OrdersResultsByPublishDate()
    {
        // arrange
        posts = new List<Post>
        {
            new Post { PublishDate = new DateTime(2014, 1, 3) },
            new Post { PublishDate = new DateTime(2014, 1, 2) },
            new Post { PublishDate = new DateTime(2014, 1, 1) },
        }.AsQueryable();
        SetUp();

        // act
        var actual = repository.All();

        // assert
        var sorted = posts.OrderBy(post => post.PublishDate);
        CollectionAssert.AreEquivalent(actual, sorted);
    }

    [Test]
    public void GetAllPublished_ExcludesDrafts()
    {
        // arrange
        posts = new List<Post>
        {
            new Post(),
            new Post(), 
            new Post { Draft = true }
        }.AsQueryable();
        SetUp();

        // act
        var actual = repository.GetAllPublished();

        // assert
        Assert.AreEqual(2, actual.Count());
    }

    [Test]
    public void GetAllPublished_OrdersResultsByPublishDate()
    {
        // arrange
        posts = new List<Post>
        {
            new Post { PublishDate = new DateTime(2014, 1, 3) },
            new Post { PublishDate = new DateTime(2014, 1, 2) },
            new Post { PublishDate = new DateTime(2014, 1, 1) },
        }.AsQueryable();
        SetUp();

        // act
        var actual = repository.GetAllPublished();

        // assert
        var sorted = posts.OrderBy(post => post.PublishDate);
        CollectionAssert.AreEquivalent(actual, sorted);
    }
}

Particular areas of concern:

  • Starting with the repository - are my repository tests correct?
  • Given correct repository tests - are my controller tests correct?

A correct test in my mind is one that is purposeful and thorough. Are my tests correct and thorough?

\$\endgroup\$
  • \$\begingroup\$ IMappingEngine should not injected as dependency , so it should resolved inside the constructor. good code through. \$\endgroup\$ – paritosh Aug 8 '14 at 20:55
  • \$\begingroup\$ @paritosh Would you care to elaborate on that? It is my understanding that I must not depend on the implementation (AutoMapper) as that would result in an integration test. \$\endgroup\$ – Caster Troy Aug 8 '14 at 21:11
  • \$\begingroup\$ I meant is that mapping engine should not be in constructor injection as Calling code should not know about the MapperEngine. so either you can new up inside class (bad way ) or register in a DI and resolve using UnityContainer.Resolve<IMappingEngine>() \$\endgroup\$ – paritosh Aug 8 '14 at 21:14
  • 1
    \$\begingroup\$ But then my controller knows about Unity. Would property injection be applicable? \$\endgroup\$ – Caster Troy Aug 8 '14 at 21:19
  • 2
    \$\begingroup\$ If you see it as a dependency, pass it in the constructor in most cases. I see no reason to use property injection, and definitely don't reference the container in your class. \$\endgroup\$ – Phil Sandler Aug 8 '14 at 21:23
9
+50
\$\begingroup\$

As Phil Sandler said, the code here is so thin that it's possibly questionable how much value unit tests actually add. However, I would say that their two main benefits are both still relevant:

  • Living documentation of what a unit of code should do
  • Instant feedback for future refactorings

While it's much more likely that you'll add to PostsRepository rather than modifying either of those methods, even simple controller methods may be refactored. For example, it's quite likely that you'll want to add paging, which will mean refactoring Index. Unit tests could be valuable there.

Mock commands, not queries

You may have heard of the idea of "command-query separation" (CQS). To quote wikipedia:

It states that every method should either be a command that performs an action, or a query that returns data to the caller, but not both.

Putting aside whether you adhere to CQS, or to what extent, the idea of everything being either a command or a query is a useful one in guiding how you test it.

For a pure query, a necessary and sufficient condition for its correctness is the correctness of what it returns. It really doesn't matter what it does, which means that a test shouldn't be concerned with its implementation. A test that relies on implementation details is coupling itself to the way the query method works in a way that violates encapsulation. This adds extra maintainance effort if you ever want to change the implementation. It also undermines both of the purposes of unit tests I mentioned at the top. They're not good living documentation if they test details which are not actually aspects of expected behaviour to an external consumer, and they can't give useful instant feedback if they have to be updated alongside the SUT.

So, all that said, take a look at Index_UsesPostsRepository. What this is doing is taking a pure query method, and instead of checking the result it returns, it's looking at the specific implementation of how it gets that result. Consider again the possible future requirement I mentioned before, of adding paging. For efficiency, you might add a new method to the repository which takes the page number and entries per page as parameters (so that you can do the paging in generated SQL, rather than in memory), and switch to using that in Index instead. Now, even though Index is still perfectly valid, your test will fail, and will need rewriting. That's because your test is testing an implementation detail rather than actually documenting what the method should do.

The smell here is that you're calling a Verify method on a mock when testing a pure query method. It's fine to use a mock object as a stub or provider of fake data for a method like this, but you should be very wary about any Verify calls.

You don't have any commands yet, but when you do, that will be where using a mock as a full mock, including Verify, will come in useful. For a system like this, a command will often be something that modifies the persisted state, like adding a new blog post. Say that's done by calling an Add method on the repository. Calling that Add method, and passing it the correct parameters, will be the intended behaviour of the method, rather than just a particular implementation, so in that case, verifying that it's called correctly will be just what you want in a test. In this case, it will accurately be documenting what the method should do, and it never break during a refactoring unless you actually introduce a bug, which is exactly what you want.

Dealing with DbSet and DbContext

Unfortunately, this is a bit messy. There's essentially three things you might want:

  • DbSet for querying, through the IQueryable interface
  • DbSet for adding items (Add), through the IDbSet interface
  • DbContext for saving changes (SaveChanges), not on any interface

This isn't very easy to do neatly. As you've seen, if you pass the DbContext into your repository, then mocking the DbSet just for querying is a bit nasty, and you'll run into further problems if you want to mock the context to check if SaveChanges is called.

I'm not fully up on how people tend to deal with this, but one strategy might be to define an interface like:

public interface IDataAccess<TEntity>
{
    IQueryable<TEntity> AsQueryable();
    void Add(TEntity entity);
    int SaveChanges();
}

You may be able to come up with a better name than IDataAccess. The default implementation would be very simple, with AsQueryable just returning the appropriate DbSet<TEntity>, and Add and SaveChanges passing through to the corresponding methods on the DbSet and DbContext respectively. Then the repository would never touch the DbSet or DbContext directly and use this instead. Because this would be so thin, and should pretty much never change, it wouldn't need its own testing, and would be straightforward to mock. For the tests so far, you'd just set up your mock to return posts.AsQueryable() for the AsQueryable() method, and ignore the other two.

It's a bit nasty having to add yet another abstraction layer, especially as all it's doing is just isolating code to not test, but to me it seems like a good way of dealing with the lack of an easy to isolate DbContext and DbSet.

Data builder pattern

The post repository test class contains a couple of tests where you're creating a few instances of Post. It's likely that, especially as the Post class grows, this is going to be a common task. This can lead to a lot of repetition or near-repetition.

It'll be even more of a headache if you start requiring properties to be populated (either by using a constructor with required parameters, or by having some validation like ModelState.IsValid). It's likely that you're often going to only care about a subset of the properties on Post, meaning you'll be having to pass in a lot of dummy values. Plus, if you add any new required properties to Post, it'll break all your old tests, and you'll have to fix them individually. Yuck!

So if you have a class like this, which:

  • Is more focussed on holding data than functionality
  • You find yourself creating in a lot of tests
  • Has properties which are likely to be relevant to some, but not all, tests

You might consider using the data builder pattern. Builders should live in your test project, are often written with a fluent style, and would look something like this:

public class PostBuilder
{
    private DateTime _publishDate;

    public PostBuilder()
    {
        _publishDate = DateTime.Now;
    }

    public PostBuilder WithPublishDate(DateTime publishDate)
    {
        _publishDate = publishDate;
        return this;
    }

    public Post Build()
    {
        return new Post { PublishDate = _publishDate };
    }
}

As you added new properties, you'd add them in the same way as PublishDate. The key here is that each property has a sensible default, so that in tests where you don't care about the publish date, you wouldn't have to specify anything for it.

There is a bit of overhead to doing it this way, so a data builder is something you should refactor to when you feel the need. For now, methods on the test class might be good enough, and you may be able to get away in a lot of cases with just returning a default Post then modifying it directly, since it's (presumably) not immutable. But don't leave it too late to refactor to, otherwise you'll have to go and update a lot of tests!

Misc

  • Index_ReturnsCorrectModel is quite confusing. What does "returns correct model" mean? If you were documenting what Index does, you wouldn't have a bullet point reading "returns correct model", in the same way that if you were writing a method that solves some mathematical problem you wouldn't write a test called ReturnsCorrectAnswer.

    What it actually seems to be testing is that it returns models of the correct type. I guess there's nothing wrong with this, but it seems like a pretty low-value test. I can't think of any situation where just returning the correct type is enough to say the method is returning the correct thing, and as soon as you start testing more specific properties, you'll end up casting to the type anyway, so you'll end up testing that implicitly.

    It's just a possibility, but perhaps you arrived at this test through wanting to be TDD-y, and thinking something like "I'm going to need a Post type, so I'd better write a test that will then require that type in order to be made to pass." If so, this is going about TDD the wrong way around! Just because you write the test first, doesn't make it TDD. If you write a test purely to deliberately motivate some design decision you already made, then that's the invented design driving the tests. TDD should be about the tests driving you to discover the design.

  • Small point, but it seems more appropriate to pass PostsRepositoryTest's posts in to SetUp as a parameter, rather than have it as a class-level variable. In this case I'm not sure it'll actually make any difference, but it's the kind of thing that sometimes comes back to bite you later, so at the very least it's a good habit to develop.

  • GetAllPublished_ExcludesDrafts isn't actually testing what it claims quite as well as it could. If posts have some kind of unique ID property, I'd suggest using that here. Select out the IDs of the returned posts and check that they're identical to the hard-coded IDs of the desired posts. Alternatively I think the way you're setting up the underlying queryable should preserve reference equality, so you could use that to check the same thing. At the very least, you should probably check that .Any(post => post.Draft) is false, though this would mean two asserts in the same test.

\$\endgroup\$
  • \$\begingroup\$ @CasterTroy You're quite welcome! I added another paragraph to the first bullet point in "Misc". It's guesswork, so maybe it totally misses the mark, but if not, it's probably a relatively important point. \$\endgroup\$ – Ben Aaronson Aug 14 '14 at 22:33
3
\$\begingroup\$

I only took a brief look at your tests. It looks like you have a decent (or better) grasp of how to write tests and mock dependencies.

That said, in looking at your Controller and Repository implementations, I don't see anything worth unit testing.

So my question would be: why do you want to unit test your controllers and repositories? What do you intend to get out of these tests?

\$\endgroup\$
  • \$\begingroup\$ I am honestly under the impression that unit-testing in this particular case is causing more harm than good but I suspect that is because the project is currently small and that I am a noob. I am perusing unit-tests in this particular case for educational purposes if nothing else. I actually used TDD originally but based on some feedback I started experimenting with some new techniques. Obviously I want the same benefits as everybody else (immediate feedback etc.) too. Thanks for answer. \$\endgroup\$ – Caster Troy Aug 8 '14 at 21:39

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.