4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ GetChangedClaimsSouldCombineResultsFromDependedServices: "Sould" --> "Should". Other than that, this looks good. \$\endgroup\$ – Carl Manaster Jul 30 '15 at 18:41
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Sut is software under test and is used a lot in unit testing see xunit book and open source projects such as autofixture and more. \$\endgroup\$ – Jakob Jul 30 '15 at 21:06
  • \$\begingroup\$ Pretty sure he knows that @jakob. I think he was making a point. \$\endgroup\$ – RubberDuck Jul 30 '15 at 23:16
  • \$\begingroup\$ I didn't "know", but I suspected it means something. I'm glad to know about places where this is widely used, as Jakob commented. But I don't get yet the compelling reason to circumvent general good practices in naming. I'd welcome some links that make a good point in favor of sut and its brothers cut, mut, aut... (and NOT in example code) \$\endgroup\$ – janos Jul 31 '15 at 9:09
3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.