Test Anti-Pattern: Proving The Code is Written Like the Code Is Written

Testing is suppose to verify that code does what we want. However, I have seen many “unit” tests that spend a lot of time verifying that code works how they wrote it, rather than verifying that it achieves the desired result. The worst cases of this I have seen involve the use of mock objects. This is an real example (I changed the names to protect the guilty) of that anti-pattern (using Java and EasyMock):


public void testDoSomthingUseful()
{
  MockControl mockObjToTestControl = MockClassControl.createControl(ClassToTest.class);
  ClassToTest mockObjToTest = (ClassToTest)mockObjToTestControl.getMock();
  
  mockObjToTest.doSubtask1();
  mockObjToTestControl.setReturnValue("abc");
  
  mockObjToTest.doSubtask2();
  mockObjToTestControl.setReturnValue(1);

  ClassToTest objToTest = new ClassToTest()
  {
    String doSubtask1()
    {
      return mockObjToTest.doSubtask1();
    }

    String doSubtask2()
    {
      return mockObjToTest.doSubtask2();
    }
  }

   mockObjToTest.replay();

   objToTest.doSomethingUseful();

   mockObjToTest.verify();
}

As you can see this code proves that the doSomethingUseful() calls do doSubtask1() and doSubtask2(). The problem is that this is a total useless test. The only thing it proves is that the code is written the way the code is written. There are not even any assertions that the desired result as achieved. Even if there were some assertions made about the the results of doing something useful you still have a test which is completely and absolutely tied to the current implementation of the doSomethingUseful() method.

Tests should check for the desired behavior1 and avoid checking implementation details. Our tests should not care if next week the doSomethingUseful() method is refactored such that it does not call doSubtask1() as long as it still achieves the same result. The above example is an extreme case and very few people write tests as pathologically broken as that. However, a disturbing number of people write tests with little thought to whether their are verifying desired behavior or implementation details.

A key way to encourage this behavior oriented approach is to only use an objects, globally, public interface in the tests. One objection I have heard to this approach is that you will miss bugs because you correctness checks will, inevitably, be made at a higher level. This argument is just wrong. Any bug that is undetectable from the public interface does not matter. Of course, detecting some bugs via the public interface might be a bit more difficult than immediately reaching into the guts of your objects but it will be better because you will be testing the intended behavior of those objects, not their implementations. As a side benefit you will be implicitly documenting the public interface with is a Good Thing.

As with any rule there are a few situation in which I do write tests against non-public parts of an objects interface. For example, if you have a non-public method that you suspect is likely to break (e.g. it embodies a non-trivial algorithm or involves non-trivial interaction with other components) and it is difficult to diagnose it’s failures from the public interface. However, I consider this confluence of situations to be extremely suspect. It will most often result from sub-optimal design, better solved by refactoring than than by writing tests against the non-public interface.

1. Dave Astels is working on something he calls BDD (these slides are good too). I think he has got it just about right and I am looking forward to trying out RSpec real soon now.

Comments 10

  1. Andrew Cowper wrote:

    What if a method doesn’t return any results, or just returns success or failure? I can’t just call it and check that it returns ‘true’, that doesn’t prove much.

    For instance if a method has to insert a row in the DB and returns void I need to mock the DB and assert that the methods are called as I expect. This will look just like your example above.

    There are many simalr examples where a method has to do something correctly but not return any results.

    Posted 03 Nov 2005 at 5:54 am
  2. Adrian Mouat wrote:

    Not sure I entirely agree with this.

    A unit test should be exactly that – it should test a “unit” of code.

    Whether this code is public or private should be irrelevant. I certainly can think of cases in my own code where I would *like* to test a private function but it is difficult via junit.

    Why shouln’t I test that my algorithm (say a sort or lcs algorithm) returns the correct results directly? I don’t believe that it is private due to bad design.

    I agree that tests which show the intended use of the software are better, but I don’t believe they are necessarily sufficient.

    Posted 03 Nov 2005 at 7:07 am
  3. Peter Seymour wrote:

    If you can write a test for a method and the method is called from your user code then that means there are at least two distinct ways to use the method. Perhaps this means it is logically complete and maybe even generic enough to be a unit of its own.

    Posted 03 Nov 2005 at 7:54 am
  4. Adrian Mouat wrote:

    I was thinking it’s possible to test via reflection, which is another way to use methods, but not a very nice one.

    For another viewpoint on all this, see:

    http://www.artima.com/suiterunner/private.html

    Posted 03 Nov 2005 at 9:56 am
  5. Peter Williams wrote:

    Andrew,

    I did not mean to imply that tests should only assert things about the results of a method. Rather that if a “bug” in the code is not detectable from a public interface of the target class, or some closely related class, it probably does not matter. When you have methods that do not return a result that is sufficient for verification you can usually call one of more other methods in the public interface to verify that the behavior of the object in question changed in an appropriate way.

    Interactions with external systems, like databases, are generally difficult to deal with in unit testing, I think. However, for that class of problems I usually have the object to be tested operate on a mock database connection rather mocking and subclassing the class to be tested.

    Posted 03 Nov 2005 at 3:33 pm
  6. Peter Williams wrote:

    Adrian,

    I do not think that “unit” is a very useful way to think about what to test. Is a unit a single control structure, a method, a class, all the classes/mixins in package/namespace (or logical equivelent), etc? A unit is all of those things as one time or another. Which one just depends on your point of view. I think that what we should be testing is not “units” but behavior. I think that verifying that all of the required behavior is provided is sufficient. After you have proved that what else is there?

    I am not really arguing that testing private methods is intrinsically evil. Just that, in the general case, it is far more expensive than it is worth. There are certainly exceptions to this rule, I even gave a few of them in my post, but I think that testing non-globally public methods is a design smell.

    Posted 03 Nov 2005 at 4:22 pm
  7. Adrian Mouat wrote:

    >I think that verifying that all of the required behavior is provided is >sufficient. After you have proved that what else is there?

    Proving that the behaviour is correct? Making the code easier to port/maintain by avoiding future bugs?

    A bug in class may produce sub-optimal or almost correct behaviour that is difficult to reproduce easily or directly from public interfaces.

    Also, when a bug is found, it will probably seem logical and easier to make the regression test as close to the method as possible.

    It may, in general, be better to avoid testing private methods but I’d be wary of giving others any rules about this.

    Posted 04 Nov 2005 at 2:43 am
  8. Peter Williams wrote:

    Adrian, I do not believe that proving correctness is an reasonable goal for non-trival applications and testing for portability sets off my “your not going to need it” filter. :) There are certainly situations where testing private methods the most cost effective approach but I think that testing a private method should be presumed wrong. When there is sufficient cause to test private methods an affirmative defense can be used to show that it was the best workable approach.

    Posted 10 Nov 2005 at 11:20 am
  9. Robert Konigsberg wrote:

    I’m not sure I understand the flow of your unit tests — your syntax looks wrong. Perhaps you’re assuming I see something where precise syntax is not necessary, but I don’t see it.

    Posted 12 Nov 2005 at 10:41 am
  10. Peter Williams wrote:

    Basically the unit test creates a mock version of the object to be tested. Then it subclass the class to be tested and overrides the protected methods that the method being tested will call with calls to the mock object.

    This results in a test which verifies that the method being tested really does call the protected methods with the correct arguments.

    I do not think there are any major syntax errors in that code, but I have not compiled it so there might be a couple of small ones.

    Posted 12 Nov 2005 at 2:27 pm

Trackbacks & Pingbacks 1

  1. From Peter Williams » Blog Archive » Mocking on 13 Jun 2007 at 10:07 pm

    […] Mr Whitney recently posted an article in which he described mock objects as “bug aggregators”. I once held a similar point of view. Back then my belief was that test doubles (mock, stub, etc) should only be used when real objects would not work, either because they were too hard to setup or because they were too slow. Recently, however, my thinking towards mocks has become a bit more nuanced1. […]