Unit tests for a simple composite

Question

I do a lot of unit testing(tdd) and would like some comments about my style. Here is a simple example from a composite.

Is there anything I could improve?

public class ChangedClaimsServiceComposite : IChangedClaimsService
{
    private readonly IChangedClaimsService[] changedClaimsServices;

    public ChangedClaimsServiceComposite(params IChangedClaimsService[] changedClaimsServices)
    {
        if (changedClaimsServices == null) throw new ArgumentNullException("changedClaimsServices");
        this.changedClaimsServices = changedClaimsServices;
    }

    public List<IChangedClaim> GetChangedClaims()
    {
        return changedClaimsServices.SelectMany(x => x.GetChangedClaims()).ToList();
    }

    public void SetClaimUpdated(IChangedClaim changedClaim)
    {
        foreach (var changedClaimsService in changedClaimsServices)
        {
            changedClaimsService.SetClaimUpdated(changedClaim);
        }
    }
}

[TestFixture]
public class ChangedClaimsServiceCompositeTests
{
    [Test]
    public void GetChangedClaimsSouldCombineResultsFromDependedServices()
    {
        //Arrange
        var changedClaimService1 = new Mock<IChangedClaimsService>();
        var changedClaimService2 = new Mock<IChangedClaimsService>();

        var changedClaim1 = CreateAnyChangedClaim();
        var changedClaim2 = CreateAnyChangedClaim();
        var changedClaim3 = CreateAnyChangedClaim();
        var changedClaim4 = CreateAnyChangedClaim();

        changedClaimService1.Setup(x => x.GetChangedClaims())
            .Returns(new List<IChangedClaim>() {changedClaim1, changedClaim2});
        changedClaimService2.Setup(x => x.GetChangedClaims())
            .Returns(new List<IChangedClaim>() { changedClaim3, changedClaim4 });

        var sut = new ChangedClaimsServiceComposite(
            changedClaimService1.Object, 
            changedClaimService2.Object);

        //Act
        var result = sut.GetChangedClaims();

        //Assert
        var expectedResult = new List<IChangedClaim>()
        {
            changedClaim1,
            changedClaim2,
            changedClaim3,
            changedClaim4
        };
        Assert.That(result, Is.EqualTo(expectedResult), "result");
    }

    [Test]
    public void SetClaimUpdatedShouldCallSameMethodOnDependedServices()
    {
        //Arrange
        var changedClaimService1 = new Mock<IChangedClaimsService>();
        var changedClaimService2 = new Mock<IChangedClaimsService>();

        var changedClaim = CreateAnyChangedClaim();

        var sut = new ChangedClaimsServiceComposite(
            changedClaimService1.Object,
            changedClaimService2.Object);

        //Act
        sut.SetClaimUpdated(changedClaim);

        //Assert
        changedClaimService1.Verify(x => x.SetClaimUpdated(changedClaim), "changedClaimService1");
        changedClaimService2.Verify(x => x.SetClaimUpdated(changedClaim), "changedClaimService2");
    }

    private IChangedClaim CreateAnyChangedClaim()
    {
        return new Mock<IChangedClaim>().Object;
    }
}

Answer 1

If you rename these:

    var changedClaim1 = CreateAnyChangedClaim();
    var changedClaim2 = CreateAnyChangedClaim();
    var changedClaim3 = CreateAnyChangedClaim();
    var changedClaim4 = CreateAnyChangedClaim();

Like this:

    var service1changedClaim1 = CreateAnyChangedClaim();
    var service1changedClaim2 = CreateAnyChangedClaim();
    var service2changedClaim1 = CreateAnyChangedClaim();
    var service2changedClaim1 = CreateAnyChangedClaim();

Then it will be more obvious that these will be used in two different services. These names will carry more meaning than just an arbitrary list of IChangedClaim objects.

---

What is "sut" ? A more meaningful name would be better.

---

In this assert:

    Assert.That(result, Is.EqualTo(expectedResult), "result");

The comment message is not very useful. Either make it something more meaningful, or delete it.

Answer 2

Overall, it looks good, but sometimes you have to ask yourself why you're testing something. What benefit does a test provide. You don't seem to really be testing your code, but the .Net framework itself. I don't see any benefit to having this test beyond making yourself feel good when it turns green.

The only other thing I would mention that @Janos didn't already is that this method doesn't really do anything.

    private IChangedClaim CreateAnyChangedClaim()
    {
        return new Mock<IChangedClaim>().Object;
    }

Do you expect the code for returning a new mock with no setup to change? I'd just chalk it up to rounding out an exercise in futility and ego stroking.

But yes, the code looks nice. You have a good style.

Answer 3

I like naming my tests with the pattern:

MethodBeingTested_State_Result

Something like:

GetChangedClaims_CreateSeperateClaims_ClaimsCombined

This makes it more obvious what the tests are performing.

Other than the other comments, the code looks fine.