15

In order to fix a bug in an application, I modified a method named postLogin by adding a call to an existing method named getShoppingCart.

Code

protected void postLogin() {
  getShoppingCart();
}

However, I'm not sure what the best way to write a unit test for postLogin is.

Approach 1

Use verify from Mockito to simply verify that the method was called.

verify(mock).getShoppingCart();

Approach 2

Test the side effect of the method call by fetching the value of the user's shopping cart.

AssertNotNull(user.getShoppingCart());

Is one approach better than the other?

A_B
  • 409
  • 1
  • 3
  • 10
  • 1
    whichever makes the test easier to understand, and keeps the code clean. If you are unsure of the test design, that _COULD_ also be a sign that the code design is off. Make sure you are asking these questions: "**WHY** does adding that method call fix the bug? Is this the **RIGHT** way to fix this bug?" – Caleb Nov 25 '17 at 20:06
  • 9
    Unless your `getShoppingCart()` method has side-effects, you don't need to test that it's called. If it does have side effects, you should really change its name because `getXXX()` methods conventionally should be idempotent. – Jules Nov 26 '17 at 02:41
  • @Jules `getNextValue`? Arguably, someone could say "Don't call it a getter; change the name to `nextValue`", but I have seen `getNext` used before. Perhaps a better example would be an object representing an electron; what happens when I call `getPosition`? Or worse, `getPosition(); getVelocity();` – Aaron Nov 30 '17 at 22:14

5 Answers5

18

I would usually prefer method 2.

Why? Because, you want postLogin to change some state of your system, but how it accomplishes this (and which methods it calls internally for this) is merely an implementation detail, nothing your unit test should make any assumptions about. So better make your test just verifying the final state.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
4

I would change getShoppingCart to something like initializeShoppingCart, the purpose of the method should be clear to whoever reads it without the need to check what the method does and side effects like this can cause some surprising behavior for users of the method.

If getShoppingCart is in another class and it's already unit tested I would use approach 1 - no need to test again what's already tested. In this case we are sure that getShoppingCart works properly and we only want to assure that it's called from postLogin so if someone in the future removes this call the test will fail.

If getShoppingCart is a private method which can not be tested by itself, then I would use approach 2, to make sure that when postLogin is called the desired functionality of getShoppingCart is performed as expected.

Nastya S
  • 41
  • 1
1

When testing a function call (void or not) that has a side effect, it’s most complete to test that the side effect not only occurs, but to check that the side effect (system output or or state change) is the one desired.

hotpaw2
  • 7,938
  • 4
  • 21
  • 47
  • 1
    While this is true, it is also worth considering that the *details* of the side effect that occurs could be part of the internal state of some other module, and that by checking those details you'd be coupling your test to not only the module that it is testing but also that other module, which could lead to brittle tests if those details are likely to change. Mocking the interface between modules helps prevent this problem. – Jules Nov 26 '17 at 02:44
0

I won't discuss your design, but in your case I would go for the first approach because unit test is for testing what methods do technically regardless of their job in the domain, that is, what your method postLogin does? Technically it calls getShoppingCard so you have to test that is really is calling getShoppingCard, I would also create another test for getShoppingCard to test what it does and if it has side effects I will check it inside that new test.

La VloZ Merrill
  • 29
  • 3
0

You have a bug in postLogin. So the first thing you should do is create a unit test which upon calling postLogin without the expected set of information will "fail".

From the above idea, another alternative from the 2 proposed is to inject the information about the shopping cart as a parameter. If you don't have the correct information, you throw an unchecked exception. This will make it clear that without the correct details, your method is doomed.

This will require a little change where the client calling the postLogin right now is required to also pass the shopping cart info. To me this is still coherent now that you see they are coupled. This coupling will be done by the caller.

Then you wouldn't even need to test getShoppingCart inside postLogin because the real method under test is postLogin. It is the one that has the bug and the only one that requires proper fix and validation. With the injected dependency, you will be able to test it easily under different condition and confirm that no error is thrown.

gumol
  • 154
  • 4