Don’t Use Side-Effects to Verify Behaviour

I’m sure no one goes out of their way to do it, but it’s not uncommon to find tests that verify the behaviour by relying on a side-effect of the operation rather then sensing the actual operation itself. It’s easy to see how the rationale probably goes:

• I need to test that an operation has happened.
• The code under test logs when the operation occurs.
• So, I can use the presence of the log message as proof.

Here is the kind of test that results from applying that logic:

[Test]
public void manually_refreshing_requests_an_update()
{
  var cache = new Cache();
  Assume.That(Log.Messages.Count, Is.EqualTo(0));

  cache.Refresh();

  Assert.That(Log.Messages.Count, Is.EqualTo(1));
  Assert.That(Log.Messages[0].Text,
              Contains(“cache updated”));
}

This example has come up numerous times in real life and is a prime example of the confusion between correlation and causation. In essence the test is saying that the http message was sent due to the log message being written, which is clearly nonsense.

The log message or event is probably highly coupled to the action that is under test and therefore is highly correlated with it, but that does not make it any more right to use it to sense behaviour. When developers complain about brittle tests this is exactly the kind of thing they dislike because a change in non-functional behaviour can cause a test for functional behaviour to break.

The test should should make it abundantly clear what interaction is expected to be taking place. If the side-effect is behaviour that needs verifying too, then it should be done separately. This is another problem I often see with tests – one test verifying two independent behaviours. I wouldn’t want to suggest it’s down to laziness per-se but “killing two birds with one stone” might seem an easier path to take in the short term.

Consequently I’d end up with two different tests – one to sense the interaction with the HTTP client and another to describe the side-effect.

[Test]
public void manually_refreshing_requests_an_update()
{
  var httpClient = new MockHttpClient();
  var cache = new Cache(httpClient);

  cache.Refresh();

  Assert.That(httpClient.Requests.Count,
              Is.EqualTo(1));
}

[Test]
public void an_event_is_logged_when_the_cache_updates()
{
  var cache = new Cache();
  Assume.That(Log.Messages.Count, Is.EqualTo(0));

  cache.Refresh();

  Assert.That(Log.Messages.Count, Is.EqualTo(1));
  Assert.That(Log.Messages[0].Text,
              Contains(“cache updated”));
}

You may notice that the original test made no reference to the HTTP Client, which may be because it’s a singleton mocked out elsewhere. As we didn’t interact with it directly in the test it wasn’t mentioned, but in my rewritten test I’ve brought the dependency front-and-centre to make it obvious what the interaction is with.

The second one is just the original test but with a better name to more actually describe the behaviour we are interested in. Now we can change the behaviours a little more easily. Yes, the second is still coupled to the behaviour in the first, but the first is no longer tied to the behaviour in the second.