23 Mar 2011

Stop Using Mocks - A Rebuttal

This morning, I opened my Twitter client to find a cheerful tweet posted by Karl Seguin (@karlseguin), who stated:

"I'm pretty close to the opinion that if you are testing with Moq, you simply aren't testing properly."

Yeah, I know it's bait, but I still responded.  After some talk with me and others, Karl graciously wrote a blog post explaining what he meant.  In his post titled "Stop Using Mocks," he wrote about the "plague" of which the .Net community suffers -- a lack of understanding of what stubs and mocks are.  And since he thinks that...

"The fact though is that if you aren't testing properly, you might as well not test at all."

... he took the time provided a clear example of what he meant, with a code sample, a test that the average .Net developer might write (incorrectly), and a set of tests that he thought were properly written.

Before I get into this, I want to say that I think this is an important discussion to have because it gets down to some fundamental approaches to testing and design.  This is even a discussion I've had recently with peers over a set of tests that look eerily-similar to the ones Karl provided.  And since I had already had that debate, I knew how to break Karl's tests as soon as I read them.  

Test Driven Design

Karl provided the simple example

Here's my first disagreement with him. If you're practicing TDD, what you'll want is to produce tests that mold the production code to that which will produce the expected results.  If you're not practicing TDD, you'll look at production code and make assumptions about how best to write tests for it.  

Yes, I know that TDD isn't the subject of his post, but I think it's what shapes the entire discussion.  He took a piece of pre-written code and stated that there were FIVE things that he wanted to test about that code.  If we were going test-first, we'd have to take our requirements and then figure out what tests to write to satisfy them.

For example, given what I know about his sample, it seems that what we need is a class that will return a user if a user/pass match.  If they don't, we get no user.  So based on that, here's how I'd start my tests:

  1. Test that if the username and password match a user in the system, return that user.
  2. Test that if the username matches but the password does not, we return no user.
  3. Test that if the username does not match, we return no user.

That's about as simple as it can be.  I see three tests, and they are all tied to actual business requirements -- not preexisting code, and not even dependencies or any existing code.  I state what I need, I write the failing tests, and now the question is:  How do I make them pass?

Testing Internals

The danger in writing tests after-the-fact is that it's sometimes easy to see the need for tests for code-based things that have no tie to business requirements.  Take his "LoadsTheUserFromTheDataStore" test: 

[Test]
public void LoadsTheUserFromTheDataStore()
{
  var store = A.Fake<IDataStore>();
  new UserRepository(store, A.Fake<IEncryption>()).FindByCredentials("Leto", null);

  A.CallTo(() => store.FindOneByNamedQuery("FindUserByUserName", "Leto")).MustHaveHappened(); 
}

This test asserts that the username was passed to this particular data store method.  What happened to the results?  Don't know.  

It could be said that making sure that result is returned is the responsibility of another test, which Karl also provides:

[Test]
public void ReturnTheValidUser()
{
  var store = A.Fake<IDataStore>();
  var encryption = A.Fake<IEncryption>();
  var expected = new User();

  Any.CallTo(store).WithReturnType<User>().Returns(expected);
  Any.CallTo(encryption).WithReturnType<bool>().Returns(true);
  var user = new UserRepository(store, encryption).FindByCredentials(null, null);

  Assert.AreSame(expected, user);
}

So here's the test that shows that the user is returned when... any username and password is used?  

You could say the two, combined, will work.  You'd be wrong, but let's say you're right.  What will these tests mean to another programmer who is approaching the problem from the point-of-view of a contract the code is supposed to satisfy?  This programmer needs to be able to see the username and password go in, and the user returned.  That simple fact is nowhere to be seen in the code, and instead has to be derived by looking at multiple tests and the production code itself.

Complex Tests Don't Work

Karl explains why he doesn't like how the average .Net developer would test this, but I think that .Net developer has one thing going for him -- his tests throws exceptions for obvious errors.  I said earlier that I knew how to break his code, so here it is:

This code still passes Karl's tests, despite the obvious problems.  

The tests still fail because what they're testing for is still happening.  Does the class check for a matching password?  Yes.  Does the class pull an user out of the data store?  Yes.  Does it call the data store with the username passed in?  Yes.  

Does it ever check that all of these micro-unit tests are ever run together, as a single unit, to test the business requirement?  No.  And since it doesn't test for that, it's not hard to modify the production code in such a way that violates the requirement but still passes the other tests.

I don't mean to poke at Karl's examples, especially since they were written in a blog post instead of Visual Studio (yet I could still copy-paste them into VS and run them, kudos Karl).  However, he's is stating that much of the .Net community is doing it wrong, that we don't understand testing, and that this is how we should do it.  If we're the ones that are wrong, why is it that the "wrong" example test he provides will fail when the production code breaks yet his won't?

Hit Me Back

If I'm going to tear down something, it's only fair that I offer an alternative.  Now, normally I'd use some combo of SpecFlow with MSpec, but since Karl's examples were NUnit tests I'll show you my TDD'd result.  I'm using AutoMoq, a small auto-mocking container I wrote that hides away some of the ugliness of passing around stubs/mocks/fakes/whatever.  I also love to refactor my unit tests into something that reads more like English and less like lambda-e.  But all of my personal preferences aside, I think these tests are aligned more with the intent and would be easier to maintain and understand in the long-term.

And if you don't like them, please write a long blog post bashing them -- I can take it.  Hey, this is how we all get better, right?