Unit test using Moq doesn't pass, object is null, did I miss something?

Question

The class I want to test is my ArticleManager class, specifically the LoadArticle method:

public class ArticleManager : IArticleManager
{
      private IArticle _article;

      public ArticleManger(IDBFactory dbFactory)
      {
            _dbFactory = dbFactory;
      }

      public void LoadArticle(string title)
      {
            _article = _dbFactory.GetArticleDAO().GetByTitle(title);

      }
}

My ArticleDAO looks like:

public class ArticleDAO : GenericNHibernateDAO<IArticle, int>, IArticleDAO
{
       public virtual Article GetByTitle(string title)
       {
           return Session.CreateCriteria(typeof(Article))
               .Add(Expression.Eq("Title", title))
               .UniqueResult<Article>();
       }
}

My test code using NUnit and Moq:

[SetUp]
public void SetUp()
{
        _mockDbFactory = new Mock<IDBFactory>();
        _mockArticleDao = new Mock<ArticleDAO>();

        _mockDbFactory.Setup(x => x.GetArticleDAO()).Returns(_mockArticleDao.Object);

        _articleManager = new ArticleManager(_mockDbFactory.Object);
}


[Test]
public void load_article_by_title()
{
     var article1 = new Mock<IArticle>();

     _mockArticleDao.Setup(x => x.GetByTitle(It.IsAny<string>())).Returns(article1.Object);

     _articleManager.LoadArticle("some title");

     Assert.IsNotNull(_articleManager.Article);
}

The unit test is failing, the object _articleManager.Article is returning NULL.

Have I done everything correctly?

This is one of my first unit tests so I am probably missing something obvious?

One issue I had, was that I wanted to mock IArticleDao but since the class ArticleDao also inherits from the abstract class, if I just mocked IArticleDao then the methods in GenericNHibernateDao are not available?

Answer 1

Preface: I'm not familiar with using Moq (Rhino Mocks user here) so I may miss a few tricks.

I'm struggling to follow some of the code here; as Mark Seemann pointed out I don't see why this would even compile in its current state. Can you double check the code, please?

One thing that sticks out is that you're injecting a mock of IDBFactory into Article manager. You then make a chained call of:

_article = _dbFactory.GetArticleDAO().GetByTitle(title)

You've not provided an implementation of GetArticleDAO. You've only mocked the LoadByTitle bit that happens after the GetARticleDAO call. The combination of mocks and chained calls in a test are usually a sign that the test is about to get painful.

Law of Demeter

Salient point here: Respect the Law of Demeter. ArticleManager uses the IArticleDAO returned by IDBFactory. Unless IDBFactory does something really important, you should inject IArticleDAO into ArticleManager.

Misko eloquently explains why Digging Into Collaborators is a bad idea. It means you have an extra finicky step to set up and also makes the API more confusing.

Furthermore, why do you store the returned article in the ArticleManager as a field? Could you just return it instead?

If it's possible to make these changes, it will simplify the code and make testing 10x easier.

Your code would become:

public class ArticleManager : IArticleManager
{
      private IArticleDAO _articleDAO

      public ArticleManger(IArticleDAO articleDAO)
      {
            _articleDAO = articleDAO;
      }

      public IArticle LoadArticle(string title)
      {
            return _articleDAO.GetByTitle(title);
      } 
}

You would then have a simpler API and it'd be much easier to test, as the nesting has gone.

Making testing easier when relying on persistence

In situations where I'm unit testing code that interacts with persistence mechanisms, I usually use the repository pattern and create hand-rolled, fake, in-memory repositories to help with testing. They're usually simple to write too -- it's just a wrapper around a dictionary that implements the IArticleRepository interface.

Using this kind of technique allows your ArticleManager to use a fake persistence mechanism that behaves very similarly to a db for the purpose of testing. You can then easily fill the repository with data that helps you test the ArticleManager in a painless fashion.

Mocking frameworks are really good tools, but they're not always a good fit for setting up and verifying complicated or coherent interactions; if you need to mock/stub multiple things (particularly nested things!) in one test, it's often a sign that the test is over-specified or that a hand-rolled test double would be a better bet.

Testing is hard

... and in my opinion, doubly hard if you start with mocking frameworks. I've seen a lot of people tie themselves in knots with mocking frameworks due to the 'magic' that happens under the hood. As a result, I generally advocate staying away from them until you're comfortable with hand-rolled stubs/mocks/fakes/spies etc.

Answer 2

As you have currently presented the code, I can't see that it compiles - for two reasons.

The first one is probably just an oversight, but the ArticleManager class doesn't have an Article property, but I assume that it simply returns the _article field.

The other problem is this line of code:

_mockArticleDao.Setup(x => x.GetByTitle(It.IsAny<string>())).Returns(article1.Object);

As far as I can see, this shouldn't compile at all, since ArticleDAO.GetByTitle returns Article, but you are telling it to return an instance of IArticle (the interface, not the concrete class).

Did you miss something in your description of the code?

In any case, I suspect that the problem lies in this Setup call. If you incorrectly specify the setup, it never gets called, and Moq defaults to its default behavior which is to return the default for the type (that is, null for reference types).

That behavior, BTW, can be changed by setting the DefaultValue property like this:

 myMock.DefaultValue = DefaultValue.Mock;

However, that's not likely to solve this problem of yours, so can you address the issues I've pointed out above, and I'm sure we can figure out what's wrong.

Answer 3

I am not a Moq expert but it seems to me that the problem is in you mocking ArticleDAO where you should be mocking IArticleDAO.

this is related to your question:

One issue I had, was that I wanted to mock IArticleDao but since the class ArticleDao also inherits from the abstract class, if I just mocked IArticleDao then the methods in GenericNHibernateDao are not available?

In the mock object you don't need the methods inherited from the GenericNHibernateDao class. You just need the mock object to supply the methods that take part in your test, namely: GetByTitle. You provide the behavior of this method via mocking.

Moq will not mock methods if they already exist in the type that you're trying to mock. As specified in the API docs:

Any interface type can be used for mocking, but for classes, only abstract and virtual members can be mocked.

Specifically, your mocking of GetByTitle will be ignored as the mocked type, ArticleDao, offers a (non-abstract) implementation of this method.

Thus, my advise to you is to mock the interface IArticleDao and not the class.

Answer 4

As mentioned by Mark Seeman, I couldn't get this to compile "as-is" as the .GetByTitle expectation returns the wrong type, resulting in a compile-time error.

After correcting this, and adding the missing Article property, the test passed - leading me to think that the core of your problem has somehow become lost in translation as you wrote it up on SO.

However, given you are reporting a problem, I thought I'd mention an approach that will get Moq itself to help you identify your issue.

The fact you are getting a null _articleManager.Article is almost certainly because there is no matching expectation .GetByTitle. In other words, the one that you do specify is not matching.

By switching your mock to strict mode, Moq will raise an error the moment a call is made that has no matching expectation. More importantly, it will give you full information on what the unmatched call was, including the value of any arguments. With this information you should be able to immediately identify why your expectation is not matching.

Try running the test with the "failing" mock set as strict and see if it gives you the information you need to solve the problem.

Here is a rewrite of your test, with the mock as strict (collapsed into a single method to save space):

[Test]
public void load_article_by_title()
{
    var article1 = new Mock<Article>();
    var mockArticleDao = new Mock<ArticleDAO>(MockBehavior.Strict); //mock set up as strict
    var mockDbFactory = new Mock<IDBFactory>(MockBehavior.Strict); //mock set up as strict

    mockDbFactory.Setup(x => x.GetArticleDAO()).Returns(mockArticleDao.Object);
    mockArticleDao.Setup(x => x.GetByTitle(It.IsAny<string>())).Returns(article1.Object);

    var articleManager = new ArticleManager(mockDbFactory.Object);
    articleManager.LoadArticle("some title");

    Assert.IsNotNull(articleManager.Article);
}