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 SUTIn 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 systemHaving 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?
Repository and Controller tests
c#;unit testing;entity framework;asp.net mvc
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 doInstant feedback for future refactoringsWhile 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 queriesYou 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 DbContextUnfortunately, this is a bit messy. There's essentially three things you might want:DbSet for querying, through the IQueryable interfaceDbSet for adding items (Add), through the IDbSet interfaceDbContext for saving changes (SaveChanges), not on any interfaceThis 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 patternThe 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 functionalityYou find yourself creating in a lot of testsHas properties which are likely to be relevant to some, but not all, testsYou 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!MiscIndex_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.