5

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();
}
6
  • 5
    Unless I misunderstood your question, this is exactly what test stubs are about. Aug 26, 2016 at 7:18
  • 3
    Look at it this way. If your test is supposed to test A and you need B and C to perform that test you definitely want to fake B an C to make sure those cannot make your test of A fail. In the end you will need an integration test that tests A, B and C in conjunction. But while you are testing A, you want to test A only and nothing else. That is what "unit" in unit test says. Aug 26, 2016 at 7:58
  • @MainMa, That is not exactly what stubs are about. The factory works as a stub in some other tests, but in this, it is not relevant at all, yet, I have to fake it to make it. Aug 26, 2016 at 8:19
  • 2
    @MartinMaat, I know that I must fake B and C in order to make A work. My problem is, I want to know if it smells bad that test A requires B and C when B and C are irrelevant to A. I just want to test that it empties the cart (the unit under test), OrderFactory has no affect on this, and yet, it must be faked. It smells to me, doesn't it to you? Aug 26, 2016 at 8:26
  • 1
    OK, I see. The smell is not in your test, it is in Buy that does a bunch of other stuff in addition to what its name suggests. It should not empty the cart and it should not create an order, the order should be injected (passed as an argument). Aug 26, 2016 at 9:04

3 Answers 3

7

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.

5
  • 1
    I think the point of the question is that the OP is not having to set up mocks which aren't relevant to the point of the test. So having to mock a database for a test called Save_Persists_Updated_User_Details may be fine, but what about having to mock a database for a test called A_400_Is_Returned_When_The_Email_Format_Is_Incorrect? Aug 26, 2016 at 7:55
  • @BenAaronson, Exactly! Aug 26, 2016 at 8:18
  • @BenAaronson: OK I totally misunderstood the question.
    – JacquesB
    Aug 26, 2016 at 11:11
  • I have rewritten the answer after better understanding the question.
    – JacquesB
    Aug 26, 2016 at 13:40
  • @JacquesB, That is spot on, I hadn't considered the consequence of it "not failing" at all. So I shouldn't identify "a unit" as "empty cart", but rather as "buy"? :) Aug 28, 2016 at 14:08
3

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!

4
  • Decoration would be an additional way to clean it up.
    – RubberDuck
    Aug 27, 2016 at 10:48
  • Right, decorators would be another way in which you could model the checkout process. I'd love to see your take on it if you'd like to answer or expand on mine. Aug 27, 2016 at 16:53
  • Thank you very much for your answer. The events are a very good idea, I already have a Bought event which I use to update my UI, but I guess the utility didn't have to stop there. Regarding object injection, I simplified the example to focus on the testing aspect. SaveChanges calls an IWriter<Order>, and the factory does not take an order, that is just how I structure my test for ease of readability. The Cashier takes in the Cart as injection as well. Thank you very much for the detailed answer though. :) Aug 28, 2016 at 14:05
  • Awesome, I'm glad it helped. I love this kind of question; great to hear it might help. Aug 29, 2016 at 6:42
1

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.

3
  • Awesome, I'll look into that. :) Aug 28, 2016 at 13:59
  • 1
    Just read your link on the Object Mother pattern. That is exactly what I need, my problem is not my implementation, but my test set up. One of the reasons I thought this smelled was because of all the setup I had to do in my tests. Thank you. Aug 28, 2016 at 14:25
  • No problem glad to help :) Aug 28, 2016 at 23:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.