Is “AssertWasCalled” an Anti-pattern?

One style of unit testing is to verify that methods were called on passed in mocks. In Rhino Mocks the method is “AssertWasCalled”. In this blog post, I’m putting forward my reasoning why these tests are unnecessary, creating more work and indirection — with no value in return.

It started off as internal discussion at 7digital, with probably an equal number of people taking the for and against opinion. It was an interesting and good-natured debate — one of the reasons its rewarding and enjoyable to be a dev at 7digital (mention I referred you if you apply J) — but there was no clear outcome. Importantly, I couldn’t see the other side of the argument.

So here’s my side of the story, and my request for you to show me what I am missing — what value do these kind of tests give me?False Security

Here’s one of those tests that verifies some collaborator was called. It probably means that if the credentials are bad the mp3 shouldn’t be given to the user, or if they are good then vice versa.

[Test]

public void Permission_checker_verifies_credentials_when_getting_an_mp3()

{

var checker = MockRepository.GenerateMock<IPermissionChecker>();

var retriever = MockRepository.GenerateMock<IAssetRetriever>();

var provider = new AssetProvider(retriever, checker);

string assetCode = "blahAssetCode";

string userCode = "blahUserCode";

provider.GetMp3Asset(assetCode, userCode);

checker.AssertWasCalled(c => c.CanVerifyCredentialsFor(assetCode, userCode));

}

But how do I make it pass, doing the simplest possible thing?….

public Mp3Asset GetMp3Asset(string assetCode, string userCode)

{

permissionChecker.CanVerifyCredentialsFor(assetCode, userCode);

return assetRetriever.GetMp3(assetCode);

}

My test console is lit up with the finest shade of bright green jetbrains can provide, and my heart is warm knowing that no underserving-fiend will be listening to my digital copy of Lady gaga. But wait… the call to permission checker may as well not be there because it contributes in no way to this behaviour.

All we had to do to get the test to pass was call that method anywhere, in any order — and that’s it.

We can fix that

[Test]

public void Returns_mp3_asset_from_provider_when_permission_checker_verifies_credentials()

{

string assetCode = "blahAssetCode";

string userCode = "blahUserCode";

var asset = new Mp3Asset("Lady gaga");

checker.Stub(p => p.CanVerifyCredentialsFor(assetCode, userCode)).Return(true);

assetRetriever.Stub(a => a.GetMp3(assetCode)).Return(asset);

var result = provider.GetMp3Asset(assetCode, userCode);

Assert.That(result, Is.EqualTo(asset));

}

[Test]

public void Blows_up_with_unauthorized_exception_when_permission_checker_cannot_verify_credentials()

{

string assetCode = "R2D2";

string userCode = "C3P0";

checker.Stub(p => p.CanVerifyCredentialsFor(assetCode, userCode)).Return(false);

Assert.Throws<UnAuthorizedException>(() => provider.GetMp3Asset(assetCode, userCode));

}

Now I have two more tests that describe the real behaviours of this asset provider — users with permission get their Lady gaga fix, while the fiends are kept wanting. With the simplest possible solution, it forces my production code to take this shape:

public Mp3Asset GetMp3Asset(string assetCode, string userCode)

{

var hasPermission = permissionChecker.CanVerifyCredentialsFor(assetCode, userCode);

if(hasPermission) return assetRetriever.GetMp3(assetCode);

throw new UnAuthorizedException();

}

What value is the “AssertWasCalled” test giving me now? What value did it give me in the first place? I can’t think of one reason why I needed it in the first place or why I should keep it.Maintenance Costs

I’ve argued the test provides me no value. If in doubt, however, why not keep it, in case there is some hidden value visible above my level of intellect? I’m about to show you that it has costs — costs in addition to those inherent of creating and maintaining any code.

Let’s make a change

Google has now purchased the asset manager and they make all their money from ads; now they give away the mp3s for free. All they want is a log stating that you took it for free, just incase the patent trolls drain their bank account, forcing Google to retrospectively recover the fee from you at a later date. Don’t worry, this is all covered in the Google terms and conditions you agreed to.

Here’s Google’s new asset business model:

public Mp3Asset GetMp3Asset(string assetCode, string userCode)

{

var mp3 = assetRetriever.GetMp3(assetCode);

Logger.LogFreeGiveaway(assetCode, userCode, mp3);

return mp3;

}

Bethany, the developer at Google changing the code, decides to be a good developer and run the tests before checking in:

She laughs out loud at the unauthorized exception, thinking there is no such thing as a free lunch yet Google is somehow giving stuff away for free. That test is soon deleted in line with the new business policy.

Cost 1: Dangerous Miscommunication

But what about the “AssertWasCalled” test? Some previous developer explicitly stated this method MUST be called — it was part of the behaviours and requirements for this feature. Does it have some unknown side effect? Is it because of a bug in the system where permission checker does some background job that allows the asset retriever to get the mp3s? She doesn’t know — but whoever wrote that test, explicitly stated — you MUST call this method, whether you do anything with the return value or not.

Because it is Google we don’t care. Imagine this is your code, or imagine you have to change this code: can you be certain it is safe to delete the “AssertWasCalled” test? You’ll probably have to spend time digging around just to make sure it is safe. But that’s ok, the value of this test is worth it, right?

Cost 2: Refactoring Baggage

 Imagine if almost every time you make a small refactoring, you have to go and fix 2 broken tests. Take the example of moving a responsibility to a new class. You have to move the behavioural test on to the new object, and cart around this “AssertWasCalled” as well

When we’re doing lots of small successive refactorings to code, do we really want double the amount of broken tests every time? Do want more things clogging up our mind as we try and clean up the code?

Cost 3: Slows down your test suite

With all these extra tests just to assert something was called, it will take time to execute them. For a couple it may be negligible, but as the test suite grows the time lost on these tests will become more noticeable if you’ve got them everywhere.Sweet Spot

AssertWasCalled is not always bad…. sometimes a collaborator will make “cross-boundary” communication. I use this term to mean anything that isn’t synchronous or feeding back into the current execution of the code. Let’s take this example:

public void OrderBreakfast(IEnumerable items)

{

 var address = GetCafeEmailAddress();

 var body = BuildBreakfastOrderEmailBodyFor(items);

 emailSender.SendBreakfastOrder(address, body);

}

A behaviour of this method is that it will email the café the breakfast order. The only way we can be certain is to verify that it carries out this behaviour is to assert the email sender sent the breakfast order email.

I am not being critical of these use cases, and encourage them on the few occasions they are needed.State of Play

I’ve shown you why I don’t like AssertWasCalled test in nearly all cases — I believe they provide no value, yet require extra work to create, maintain, and asses the validity of. I’ve shown how they break your flow and concentration by having to attend to more breaking tests, more things to refactor, and a slower test suite.

However, I invite you girls and boys to show me what I am missing — show me what value they add, and bring some balance to this argument with clear examples. If there is humble pie to be eaten, I shall indulge.

Actually I was quite hungry and made a start already…