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
- I need to cast the inner
ViewResult, as I want to inspect its
- But the
Resultcould 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
ViewResultis 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.