The importance of verifying method arguments - arcdev/engram404 GitHub Wiki

originally posted 2016-10-11 at https://engram404.net/the-importance-of-verifying-method-arguments/

Alternate Title: The importance of strict mocks

Let's do an example. (Yeah, it'll be contrived, but hopefully you'll see the idea)

Here are the business & API requirements:

  1. Payday is the 15th of each month
  2. I need a way to determine if I get paid on a given date

Let's create a couple interfaces:

public interface IDayChecker
{
	bool IsItPayday(int dateNumber);
}

public interface IPayroll
{
	bool DoIGetPaid(int dateNumber);
}

Now let's implement them:

public class DayChecker : IDayChecker
{
	// we get paid on the 15th of each month
	public bool IsItPayday(int dateNumber)
	{
		return dateNumber == 15;
	}
}

public class Payroll : IPayroll
{
	private readonly IDayChecker _dayChecker;

	public Payroll(IDayChecker dayChecker)
	{
		_dayChecker = dayChecker;
	}

	public bool DoIGetPaid(int dateNumber)
	{
		if (dayChecker.IsItPayday(1))
		{
			return true;
		}
		return false;
	}
}

Next, let's write a test method – assuming today is the 1st.
(Note: for this purposes of this article, I'm using MSUnit and Moq).

[TestMethod]
public void DoIGetPaid_on_the_1st()
{
	const int dateNumber = 1;

	var mockDayChecker = new Mock<IDayChecker>();
	mockDayChecker.Setup(dayChecker => dayChecker.IsItPayday(dateNumber)).Returns(false);

	var payrollInstance = new Payroll(mockDayChecker.Object);
	var actual = payrollInstance.DoIGetPaid(dateNumber);

	Assert.IsFalse(actual);
}

Simple – and it works.
Notice that on line 7 we're using the '"right'" dateNumber that the call should return false.

Ok. Let's write a test for the 15th.

[TestMethod]
public void DoIGetPaid_on_the_15th()
{
	const int dateNumber = 15;

	var mockDayChecker = new Mock<IDayChecker>();
	mockDayChecker.Setup(dayChecker => dayChecker.IsItPayday(dateNumber)).Returns(false);

	var payrollInstance = new Payroll(mockDayChecker.Object);
	var actual = payrollInstance.DoIGetPaid(dateNumber);

	Assert.IsFalse(actual);
}

And…surprise, surprise. This works, too.
Great! This means we have unit tests! And, if you check the coverage metrics, you'll see that it's fully covered. Sweet – no bugs, right?
Let's test it out in the '"real world.'"

I'm going to throw another test together to truly exercise all 31 possible days:

[TestMethod]
public void RealExecution()
{
	var payrollInstance = new Payroll(new DayChecker());
	for (var i = 1; i <= 31; i++)
	{
		var result = payrollInstance.DoIGetPaid(i);
		Debug.WriteLine($"payrollInstance.DoIGetPaid({i:00}); // returns {result}");
	}
}

Here's the output:

payrollInstance.DoIGetPaid(01); // returns False
payrollInstance.DoIGetPaid(02); // returns False
...
payrollInstance.DoIGetPaid(14); // returns False
payrollInstance.DoIGetPaid(15); // returns False
payrollInstance.DoIGetPaid(16); // returns False
...

Wha!? While you employer probably loves this new code, we don't – because we're never getting paid!

Sanity check. Something can't possibly be right. If nothing else, we've failed the first business requirement.

If you read the implementation of DoIGetPaid closely, then you noticed this line (that's line 21 in the second code block above):

if (_dayChecker.IsItPayday(1))

It always passes in 1.

That tells me, we didn't write our unit tests properly. (By the way, this is also one of the pitfalls of writing unit tests after writing the code. Test-Driven Development (TDD) should never have allowed this to happen.)

I'm sure you've already guessed that we needed to verify the arguments. (That was the title of the article. ;-))

Let's rewrite our test for the 15th – since that's the one that's failing.
This time, let's use a strict mock. (See line 7)

[TestMethod]
public void DoIGetPaid_on_the_15th_strict()
{
	const int dateNumber = 15;

	var mockDayChecker = new Mock<IDayChecker>(
					MockBehavior.Strict
				);
	mockDayChecker.Setup(dayChecker => dayChecker.IsItPayday(dateNumber)).Returns(false);

	var payrollInstance = new Payroll(mockDayChecker.Object);
	var actual = payrollInstance.DoIGetPaid(dateNumber);

	Assert.IsFalse(actual);
}

When running this test, we get:

Moq.MockException: IDayChecker.IsItPayday(1) invocation failed with mock behavior Strict.

Ah-ha! There's our bug! We're always passing 1 into IsItPayday. Let's fix that to so the test passes.
(By the way, does that sound to anyone like Test-Driven Development? Find a bug; write a unit test to prove the bug exists; fix the bug; re-run the unit test and show the bug is fixed.)

public bool DoIGetPaid(int dateNumber)
{
	if (_dayChecker.IsItPayday(dateNumber))
	{
		return true;
	}
	return false;
}

We re-run our tests, and… success! They all pass!
Let's double-check our '"real world'" execution, too:

payrollInstance.DoIGetPaid(01); // returns False
payrollInstance.DoIGetPaid(02); // returns False
...
payrollInstance.DoIGetPaid(14); // returns False
payrollInstance.DoIGetPaid(15); // returns True
payrollInstance.DoIGetPaid(16); // returns False
...

Whew – now we're getting paid!
And that's how it should be.

What's the moral of the story?

We must verify the values of the parameters passed into the methods we fake/mock. If we ignore them (which is quite easy to do), we risk missing a bug.

Sure, this was a contrived example, but the principle holds.

What about using a Verify statement?

This is worth covering briefly.
We could have changed our unit test to do a verify at the end of the test like so (see line 13):

[TestMethod]
public void DoIGetPaid_on_the_15th_verify()
{
	const int dateNumber = 15;

	var mockDayChecker = new Mock<IDayChecker>();
	mockDayChecker.Setup(dayChecker => dayChecker.IsItPayday(dateNumber)).Returns(false);

	var payrollInstance = new Payroll(mockDayChecker.Object);
	var actual = payrollInstance.DoIGetPaid(dateNumber);

	Assert.IsFalse(actual);
	mockDayChecker.VerifyAll();
}

This is perfectly functional, but notice the error message:

Moq.MockVerificationException: The following setups were not matched:
IDayChecker dayChecker => dayChecker.IsItPayday(15)

It tells us what we expected to happen, but not what actually happened.
I know what I expected because it's right there in the unit test. What I don't know is what actually happened. This is what's going to help me work out what's broken.

Ok, you say, but I can swap in a specific verify statement like this (see line 13):

[TestMethod]
public void DoIGetPaid_on_the_15th_verify()
{
	const int dateNumber = 15;

	var mockDayChecker = new Mock<IDayChecker>();
	mockDayChecker.Setup(dayChecker => dayChecker.IsItPayday(dateNumber)).Returns(false);

	var payrollInstance = new Payroll(mockDayChecker.Object);
	var actual = payrollInstance.DoIGetPaid(dateNumber);

	Assert.IsFalse(actual);
	mockDayChecker.Verify(dayChecker => dayChecker.IsItPayday(dateNumber));
}

And see this error message which tells me everything:

Moq.MockException: 
Expected invocation on the mock at least once, but was never performed: dayChecker => dayChecker.IsItPayday(15)

Configured setups:
dayChecker => dayChecker.IsItPayday(15), Times.Never

Performed invocations:
IDayChecker.IsItPayday(1)

This is definitely good, but it requires 2 things:

  1. you must never forget to to verify every expectation/setup
  2. you've written the same code twice (lines 7 & 13) – which means double maintenance and twice the opportunity for mistakes

More importantly to me using a '"loose'" mock leaves more room for error. Meaning: nearly any other behavior can happen against the mock that you might have missed.
Sure, there are real, valid cases where you need this functionality, but I would personally prefer that all mocks be Strict by default, then I can '"back off'" the specificity when I need to.
(Note: Moq isn't the only mocking library that does this: TypeMock defaults to '"loose'" fakes as well.)