Test code needn’t be defensive

In a code review I encountered some test code that looked a bit like this:

 var result = await _controller.Resources() as ViewResult;
 result.Should().NotBeNull();
 // ReSharper disable once PossibleNullReferenceException
 result.Model.Should.BlahBlahBlah();
 

This is a typical defensive coding pattern whose reasoning goes like this:

• The return type of _controller.Resources() is Task<ActionResult>.
• I need to cast the inner Result of this Task to a ViewResult, as I want to inspect its Model attribute.
• But the Result could be a different subclass of ActionResult, so I had better use a safe cast, just in case.
• As I’m using a safe cast, I can’t guarantee that I’ll get any instance back, so I had better do a null check.
• Oh look! ReSharper is complaining when I try to access properties of this object. As I’ve already performed a null check, I’ll turn off the warnings with a comment.

Now, defensive coding styles are valuable when we don’t know what data we’ll be handling, but this is most likely to happen at the boundaries of a system, where it interacts with other systems or, even more importantly, humans.

But in the context of a unit test, things are different:

• We are in control of both sides of the contract: the test and class under test have an intimate and interdependent existence. A different type of response would be unexpected and invalid.
• An attempt to directly cast to an invalid type will throw a runtime error, and a runtime error is a meaningful event within a test. If _controller.Resources() returns any other subclass of ActionResult, then the fact that it cannot be cast to ViewResult is the very information I want to receive, as it tells me how my code is defective.

This means I can rewrite the code like this:

 
var result = (ViewResult) await _controller.Resources(); 
result.Model.Should.BlahBlahBlah();

By setting aside the defensive idiom, I’ve made the test clearer and more precise, without losing any of its value.