Unit Testing; Expect vs AssertWasCalled

Although I have been writing Unit Tests for quite some time now, recently I have been practicing with the Test Driven Development (TDD) approach. Although earlier I was afraid that it will be a lot of overhead, but in due course I have really started liking it. It's a great way of writing good quality code.

With TDD, as the number of Unit Tests increases it also becomes important to write Unit Tests that are maintainable and readable. As a perfectionist (at least I try to be one ;)), I look for the best ways of writing Unit Test cases. In this process, one of the points I got stuck on was the best way to test the dependencies.

1. Should I use Expect/VerifyAll() or Stub/AssertWasCalled?
2. Should there be any expectations of the dependencies, when the Unit Test focus on the testing of the parent method?

In this article I will share how I previously wrote test cases and how and I why I have changed my approach. In my team we use Rhino Mock for providing the Mocking framework and in this article I will be taking the example from that only.

Let's take a simple method. Although in TDD, we write the test case first and then the code but for the sake of this explanation, I will show the method first and then share the test case I wrote.

public class SaveDraftCommandExecutor : IExecuteCommand<SaveDraftCommand>
{
private readonly IConfigurationRepository _configurationRepository;
private readonly IDraftRepository _draftRepository;
public SaveDraftCommandExecutor(

IConfigurationRepository configurationRepository, IDraftRepository draftRepository)
{
_configurationRepository = configurationRepository;
_draftRepository = draftRepository;
}
public void Execute(SaveDraftCommand command)
{
int configurationId;
var configuration = _configurationRepository.Get(configurationId);
var draft = new Draft(configuration.Name, command.Draft);
_draftRepository.Save(draft);
}
}
}

The preceding represents a class SaveDraftCommandExecutor with an Execute method. I left out some validation to keep this blog focussed. The class takes two repositories using Constructor injection. We use Ninject for doing the Dependency Injection. The method first retrieves the configuration using the ConfigurationRepository and then uses the ConfigurationName to create the draft object, that is then saved using the draft repository.

I will first share how I previously wrote test cases.

The following is my Test class:

private IDraftRepository _draftRepository;
private IConfigurationRepository _configurationRepository;
[SetUp]
public void SetUp()
{
_draftRepository = MockRepository.GenerateMock<IDraftRepository>();
_configurationRepository =
MockRepository.GenerateMock<IConfigurationRepository>();
}
private SaveDraftCommandExecutor GetCommandExecutor()
{
return new SaveDraftCommandExecutor(_configurationRepository,
_draftRepository);
}
public void GivenValidValuesWhenExecuteThenDraftSaved()
{
// Arrange
var configuration = new Configuration() {Id = 1, Name = "Config"};
_configurationRepository.Expect(x => x.Get(1)).Return(configuration);
string draft = "{draft: 123}";
_draftRepository.Expect(x => x.Save(Arg<Draft>.Matches(
d.ConfigurationName == “Config” && d.Content == draft)));
var saveCommand = new SaveDraftCommand("1", draft);
// Act
GetCommandExecutor().Execute(saveCommand);
// Assert
_adjustmentConfigurationRepository.VerifyAllExpectations();
_draftRepository.VerifyAllExpectations();
}

Although the preceding test was able to test that the Save method of the DraftRepository was called, it suffers from the following 2 problems:

1. The Test checks if the ConfigurationRepository was called to Get the configurationId using the Expect and VerifyAll functionality of RhinoMock. In my opinion this test case is doing a bit too much. The test case should not be concerned with the internals of a method. The test case should only be concerned with the outcome. Here we are bothered if the Save method of the _draft repository is called with the right parameters. If Save is called with the correct Configuration name then the Test case is working as expected. There should not be any need to create an Expect and Verify for all calls made by the method internally. Although it's just one dependency here with one call, if the test case contains multiple dependencies one can easily get lost on what to Assert and what not to.
    
2. VerifyAllExpectation is not that clear on what Expectations are being identified. I have been in situations where a dependency calls various methods in the method being tested. If we use VerifyAllExpectations then we do not know which call gave a problem.

With the preceding 2 points I rewrote the test case to be as in the following:

public void GivenValidValuesWhenExecuteThenDraftSaved()
{
// Arrange
var configuration = new Configuration() {Id = 1, Name = "Config"};
_configurationRepository.Stub(x => x.Get(1)).Return(configuration);
string draft = "{draft: 123}";
var saveCommand = new SaveDraftCommand("1", new AdjustmentDto());
// Act
GetCommandExecutor().Execute(saveCommand);
// Assert
_draftRepository.AssertWasCalled((x => x.Save(Arg<Draft>.Matches(d =>
d.ConfigurationName == “Config” &&
d.Content == draft))));
}

There are 2 major changes in the preceding test case:

1. I replaced the Expect with Stub and removed the VerifyAll() method for the ConfigurationRepository. If the draft Save method is called with the correct Config name then we know that the method is calling its dependency correctly.
    
2. _draftRepository.VerifyAll() has been replaced with _draftRepository.AssertWasCalled. This method clearly identifies what method was expected to be called. This becomes quite useful if there are multiple calls expected for the same repository.