CH7: ItemRepositoryTests, valid item test is wrong

olerass · June 3rd, 2013, 06:44 PM

In Chapter 7 we write our first unit tests for the ItemTypeRepository. Either I am missing something or there is something wrong with the test for saving valid item types (then_a_valid_item_type_id_should_be_returned).
Look at the setup for this test:

Code:

public class and_saving_a_valid_item_type : when_working_with_the_item_type_repository
{
  private int _result;
  private ItemType _testItemType;
  private int _itemTypeId;

  protected override void Establish_context()
  {
     base.Establish_context();

     var randomNumberGenerator = new Random();
     _itemTypeId = randomNumberGenerator.Next(32000);

     _session.Setup(s => s.Save(_testItemType)).Returns(_itemTypeId);            
   }
   ...

The Save method on session is passed the _testItemType argument, but at this point _testItemType hasn't been initialized - it's null. Actually _testItemType is never initialized. Thus what the test is really saying is that when Save is called with null it should return a valid item id. In other words when Save is called with an invalid item it should work, which contradicts the test itself. The following production code would pass this test:

Code:

public int Save(ItemType itemType)
{
    int id;
    using (var session = sessionFactory.OpenSession())
    {
        id = (int)session.Save(null);
        session.Flush();
    }
    return id;
}

I've checked both the code in the book (p. 165) and the downloadable source code. Both has the problem.

This is a good example on the danger of using mocks. I don't know if the author mentions this in the book (I have only read up to ch. 7), but I believe it's an important point to make.

When using mocks you are basically telling the collaborators of the UUT how to behave. The catch is this: if you tell the collaborators to behave in a way that they wouldn't do in the real world it's the tests won't tell you this. In this case the issue is that the ISession collaborator is told to return a valid id when Save is called with null. On a real session object this would throw an exception (I hope), as hinted at by the other test in this class. Still, all the tests pass with flying colors, even on the clearly invalid implementation above. The sad thing is there are no easy solution to this problem. In this case integration tests would probably discover the bug, but in some cases it's not that easy. I've brought up the problem on the discussion group for another TDD book called "Growing Object Oriented Software - Guided by Tests". Some people there have come up with somewhat nice ways to minimize the problem (i.e. contract tests). Take a look if you're interested.

Regards,
Ole

schweeneh · June 29th, 2013, 02:46 AM

I think the reason is this; you don't care what is passed to the Save method of the object implementing the ISession interface, only that it is of type ItemType because you're not testing the Save method of NHibernate's ISession interface.

You're testing the Save method of the ItemTypeRepository. You've removed the dependency on NHibernate so you want it to do as little as possible (i.e. take whatever you give it and give you back something good in return). Hence the name of the test "and_saving_a_valid_item_type" which is really saying two things being: we assume we're getting a valid ItemType AND we assume we're getting a valid id back from NHibernate.

The code you're actually testing is mind blowingly simple, and that may be what's causing the confusion.

Code:

        public int Save(ItemType itemType)
        {
            int id;
            using (var session = _sessionFactory.OpenSession())
            {
                id = (int) session.Save(itemType); <-- we don't care what this                                                                                
                                                       does, just give us back a
                                                       good id and make sure the
                                                       code surrounding it does
                                                       what it should, in this case 
                                                       return the id.                
                session.Flush();
            }
            return id;
        }