Is it bad practice, if I have to fake something that is not relevant to my test?

Question

I have a method called Buy, when called it does the following:

• It calls a factory to create an order
• Calls pay on a payment object
• Calls empty on the customer's cart
• Calls the dao to save the order

This is what my sequence diagram says should happen, however, when I write my unit test for testing that it calls Empty on the cart, it fails (null pointers) unless I specify that the factory returns an order.

So I end up faking behavior that isn't relevant, just to test if Empty has been called on the cart.

Is this a recognised code smell? When you recognise this in your code, do you change it? I could obviously split up the empty cart functionality, but I do like the clean API of Buy. Not to mention that splitting it up requires Empty to be called first or last, which in this case isn't a problem, but in some situations might be.

This clutters my tests quite a bit, and it definitely smells like my method does more than it should, so any pointers to clean solutions would be appreciated.

//Implementation
public void Buy()
{
    var order = CreateOrder();
    Pay();
    EmptyCart();
    SaveChanges(order);
}

//Test
[TestMethod]
public void Buy_WhenCalled_CallsEmptyOnCart()
{
    var cart = CreateFakeCartThatExpectsCallToEmpty();
    var order = CreateFakeOrder().Object;
    var orderFactory = CreateFakeOrderFactoryThatReturnsOrder(order).Object;
    Cashier cashier = new Cashier(orderFactory, cart);

    cashier.Buy();

    cart.Verify();
}

Answer 1

No it is not a code smell. Only if you needed to fake dependencies which are unrelated to the specified behavior of the method under test, then it would be a code smell. But it a part of the specified behavior that the Buy()-method causes an order to be created, so you would expect to supply either a real order or a fake.

Actually if the method didn't fail on receiving a null instead of an order, it could be considered a code smell.

I understand you only want to test a particular part of the methods behavior, but you should still expect the method to do everything it is supposed to do.

Answer 2

Yes. In my opinion, there does seem to be a problem with coupling. For instance, the creation of the order within the Buy method seems to violate the single responsibility principle, as well the call to SaveChanges. The reason for this, it seems to me, is that

I would solve this with some combination of events/delegation and object (dependency) injection.

Events

This allows for a pretty clean solution, and one which I prefer to use in an Object Oriented system. By firing specific events during the process, you can configure listeners in a nicely decoupled way (say, the Cart object listens for the PurchaseProcessComplete event, and then empties itself), as well as allowing you to very simply add/remove behaviors based on the events that you fire. You can also create these events and populate them with additional information as you need, which can be a bonus (for instance, a purchase order number that you can display on your UI).

In this case, testing becomes a matter of checking whether the proper events are fired (via equality). Also, you can fire the events during different stages in the process to ensure things happen in a given order.

Object injection

This is important to avoid unnecessary dependencies. Without further knowledge of your system I can't provide better details, but usually, the Cart and Order are built before the Checkout process. So, pass them in as arguments. The Cashier doesn't, intuitively, seem the right place to create orders (and it also seems weird that the OrderFactory requires the Order... shouldn't an OrderFactory create Orders?). Perhaps the Cashier should exist independently of the Cart and Order, and they should be passed in to Buy. Perhaps what you need is a CheckoutProcess object, which takes Cart and Order.

In summary, there are many ways this could go - and it's really up to you to decide which to use. I would advice to use a more indirect way of controlling the process, so you can have simpler objects and methods with single responsibilities. As always, YMMV.

Good luck!

Answer 3

It seems that your issue is that you have to write a lot of scaffolding code that doesn't deal with your test at hand. That is the smell that you are detecting.

Your issue is a common friction in unit testing. I'd suggest picking up XUnit Test Patterns, which can provide some advanced tools for unit testing. Particularly, I would look at the Object Mother pattern, which will give you consistent objects so you don't have to repetitively write code that doesn't deal with your test at hand.