Which Unit Test is correct? They both test the same thing in different ways

Question

Below both Unit Tests pass and they both test the same thing. Which is the production code uses the right error codes.

The short question is which test is correctly Unit Testing expected behaviour and why?

Please see the code sample below.

System Under Test (SUT)

public interface IRepository {
    string GetParameter(int id);
}

public class Repository {
    public string GetParameter(int id) {
        return "foo";
    }
}

public class ErrorInfo{
    public string ErrorMessage { get; set; }
}

public interface IErrorProvider {
    ErrorInfo BuildErrorMessage(string errorCodes);
}

public class ErrorProvider {
    public ErrorInfo BuildErrorMessage(string errorCodes)
    {
        return new ErrorInfo { ErrorMessage = errorCodes };
    }
}

public class DomainClass {

    private readonly IRepository _repository;
    private readonly IErrorProvider _errorProvider;

    public DomainClass(IRepository repository, IErrorProvider errorProvider) {
        _repository = repository;
        _errorProvider = errorProvider;
    }

    public List<ErrorInfo> GetErrorList(int id) {

        var errorList = new List<ErrorInfo>();
        string paramName = _repository.GetParameter(id);

        if (string.IsNullOrEmpty(paramName))
        {
            string errorCodes = string.Format("{0}, {1}", 200, 201);
            var error = _errorProvider.BuildErrorMessage(errorCodes);
            errorList.Add(error);
        }

        return errorList;
    }
}

UNIT TESTS

    //test 1
    [TestMethod] 
    public void GetErrorList_WhenParameterIsEmpty_ReturnsExpectedErrorCodes()
    {
        //Arrange
        var stubRepo = new Mock<IRepository>();
        stubRepo.Setup(x => x.GetParameter(It.IsAny<int>())).Returns(string.Empty);
        const string expectedErrorCodes = "200, 201";
        var stubErrorRepo = new Mock<IErrorProvider>();
        stubErrorRepo.Setup(e => e.BuildErrorMessage(expectedErrorCodes)).Returns(new ErrorInfo() { ErrorMessage = expectedErrorCodes + " some error message" });
        const int parameterId = 5;
        var sut = new DomainClass(stubRepo.Object, stubErrorRepo.Object);

        //Act
        var result = sut.GetErrorList(parameterId);

        //Assert
        Assert.IsTrue(result.Any(info => info.ErrorMessage.Contains(expectedErrorCodes)));
    }

    //test 2
    [TestMethod]
    public void GetErrorList_WhenParameterIsEmpty_VerifyTheCorrectErrorCodeBeingUsed()
    {
        //Arrange
        var stubRepo = new Mock<IRepository>();
        stubRepo.Setup(x => x.GetParameter(It.IsAny<int>())).Returns(string.Empty);
        const string errorCodesPassedIn = "200, 201";
        var errorRepoMock = new Mock<IErrorProvider>();
        const int parameterId = 5;
        var sut = new DomainClass(stubRepo.Object, errorRepoMock.Object);

        //Act
        sut.GetErrorList(parameterId);

        //Assert
        errorRepoMock.Verify(x => x.BuildErrorMessage(errorCodesPassedIn));
    }

The short question is which test is correctly Unit Testing the expected behaviour and why?

More details: I don't seems to have the confidence on the test1, "GetErrorList_WhenParameterIsEmpty_ReturnsExpectedErrorCodes" It seems like the stub dictates what the production code should returns.

For example, stubErrorRepo has the setup:

     stubErrorRepo.Setup(e => e.BuildErrorMessage(expectedErrorCodes)).Returns(new ErrorInfo() { ErrorMessage = expectedErrorCodes + " some error message" });

This causes production code to return the expected error codes. Then we Assert against the error codes to ensure the result message contains the expected error codes. If I change the value of the "expectedErrorCodes" variable to some other value, the test fails. Note that it is not the production error codes cause the test to fail, it is what have stubbed out "expectedErrorCodes". This make me think that this is not a true Unit test. I believe the stubs should just provide canned answers to the test, and should not dictate the final outcome of the test. Can you call it a mock? I don't know.

But I also think test describes what the production code should return. If you think test is as specification. For example, if the "expectedErrorCodes" codes are not returned, the test fails. Seems like a valid argument too. And of course if I change the production error codes, the test fails as well. But there is something fishy I cannot rule out why this test is not right.

The test2, which is GetErrorList_WhenParameterIsEmpty_VerifyTheCorrectErrorCodeBeingUsed uses the mock.Verify to ensure the correct parameters being used, which seems like a reasonable approach. But I cannot fault the first test either.

Would you prefer test1, test2 or both?

Any thoughts greatly appreciated.

Answer 1

This question demonstrates why Test-Driven Development (TDD) is a valuable discipline. If TDD had been strictly followed, the answer would have almost presented itself.

To demonstrate that, I copied the tests only (not the 'production code') to a code base, and attempted to figure out what the implementation should be in order to pass each test. Keep in mind that an important principle in TDD is that you write only enough code to make the tests pass.

Initial state

In order to compile, the initial implementation of DomainClass looks like this:

public class DomainClass
{
    public DomainClass(IRepository repository, IErrorProvider errorProvider)
    {
    }

    public IEnumerable<ErrorInfo> GetErrorList(int parameterId)
    {
        yield break;
    }
}

Obviously this doesn't pass the tests, but at least the code compiles so that you can run the tests.

Since the goal of this answer is to explore which test is most correct, I'll now isolate each test. I'll first delete test 2 so that only test 1 is left and see where that goes, and then the other way around. After that, I'll compare the two implementations to figure out which one is best.

Test 1

If test 1 is the only test of DomainClass, then this implementation passes:

public class DomainClass
{
    private readonly IErrorProvider errorProvider;

    public DomainClass(IRepository repository, IErrorProvider errorProvider)
    {
        this.errorProvider = errorProvider;
    }

    public IEnumerable<ErrorInfo> GetErrorList(int id)
    {
        yield return this.errorProvider.BuildErrorMessage("200, 201");
    }
}

Compared to the 'production code' in the OP, this is obviously not the full implementation of GetErrorList, but it does pass the test(s).

Test 2

If test 2 is the only test of DomainClass, then this implementation passes:

public class DomainClass
{
    private IErrorProvider errorProvider;

    public DomainClass(IRepository repository, IErrorProvider errorProvider)
    {
        this.errorProvider = errorProvider;
    }

    public IEnumerable<ErrorInfo> GetErrorList(int parameterId)
    {
        this.errorProvider.BuildErrorMessage("200, 201");
        return new List<ErrorInfo>();
    }
}

This implementation passes the test(s), but doesn't look correct to me. The BuildErrorMessage method is called, but the return value is ignored. That's probably not what was intended.

Stubs and Mocks

From GOOS comes this rule of thumb (paraphrased):

Use Stubs for Queries and Mocks for Commands.

Since the BuildErrorMessage method is a Query this rules states that a Stub should be used for testing. That indicates that test 1 is more correct, which the two alternative implementations also seem to indicate.

Driving the Happy Path

Obviously, neither alternative implementation is correct as is, but if test 1 is used as a foundation, a second unit test can exercise the 'Happy Path':

[Fact]
public void HappyPath()
{
    // Fixture setup
    var repositoryStub = new Mock<IRepository>();
    repositoryStub.Setup(r => r.GetParameter(1)).Returns("foo");

    var sut = new DomainClass(
        repositoryStub.Object,
        new Mock<IErrorProvider>().Object);
    // Exercise system
    var result = sut.GetErrorList(1);
    // Verify outcome
    Assert.False(result.Any());
    // Teardown
}

Which, together with test 1, forces you to write code like this:

public class DomainClass
{
    private readonly IRepository repository;
    private readonly IErrorProvider errorProvider;

    public DomainClass(IRepository repository, IErrorProvider errorProvider)
    {
        this.repository = repository;
        this.errorProvider = errorProvider;
    }

    public IEnumerable<ErrorInfo> GetErrorList(int id)
    {
        if (string.IsNullOrEmpty(this.repository.GetParameter(1)))
            yield return this.errorProvider.BuildErrorMessage("200, 201");
        yield break;
    }
}

You may, however, notice from the hard-coded 1 parameter that this can't be the final implementation yet. However, all tests are green at this point, so you must add more test cases.

Here's the Happy Path test expanded as a Parameterized Test:

[Theory]
[InlineData(1)]
[InlineData(2)]
[InlineData(3)]
public void HappyPath(int id)
{
    // Fixture setup
    var repositoryStub = new Mock<IRepository>();
    repositoryStub.Setup(r => r.GetParameter(id)).Returns("foo");

    var sut = new DomainClass(
        repositoryStub.Object,
        new Mock<IErrorProvider>().Object);
    // Exercise system
    var result = sut.GetErrorList(id);
    // Verify outcome
    Assert.False(result.Any());
    // Teardown
}

This drives the implementation to this point in order to pass all tests:

public class DomainClass
{
    private readonly IRepository repository;
    private readonly IErrorProvider errorProvider;

    public DomainClass(IRepository repository, IErrorProvider errorProvider)
    {
        this.repository = repository;
        this.errorProvider = errorProvider;
    }

    public IEnumerable<ErrorInfo> GetErrorList(int id)
    {
        if (string.IsNullOrEmpty(this.repository.GetParameter(id)))
            yield return this.errorProvider.BuildErrorMessage("200, 201");
        yield break;
    }
}

This is beginning to look more reasonable.

Summary

Test 1 is better because it specifies an expected data flow through the method. Since the BuildErrorMessage is a Query you must expect that it has no side effects. When that is the case, then why care that it was invoked?

You should care, however, about what it returns if it is invoked, and that's what the Stub specifies.

Answer 2

I think these two test test two different things. 1st test test correctness of error messages formatting, 2nd - that appropriate method was called. What I don't understand is why do you need expectedErrorCodes variable in tests in the fist place? 200, 201 error codes are just hard coded in your GetErrorList method. So assertion section of the 2nd test may look like

  errorRepoMock.Verify(x => x.BuildErrorMessage(It.IsAny<string>()));

Tests don't need to know such details.

Answer 3

They test the same "action". The semantic difference is:

The first test verifies the values returned by the method being tested.

The second verifies that it called a particular method.

//disclaimer// I'm certainly no UnitTesting expert; I'm still learning, but I that's how I read those two tests.