Should I test a method that calls a method that is already tested?

Question

I have a method that does something like this

public void addFunds(Account account, int price) {
   int credits = account.getCredits()
   account.setCredits(credits + price)
   saveOrUpdate(account)
}

the method saveOrUpdate is already tested, so my question is: should I test the method add funds? I'm trying to follow the best practices.

Answer 1

TL;DR

"Dogmatically" speaking: Yes.

Practically speaking: It's up to you and your team. If I were on your team, I'd strongly argue that the tests are valuable, since they help to define and enforce the method's complete contract, including its behavior (normal, edge-case, and otherwise).

---

If I were writing this method:

I would give it a FakeAccount and verify that setCredits is called with the right value. This validates both that it's first getting the current value and properly determining the new value. Test your edge cases. Test negatives. Test Int.Max. Test Int.Min. But write the tests like a consumer -- what, as the caller, do you want this function to do in these cases? Even if the current behavior matches your expectations, define the behaviors explicitly in your test suite.

I'd do some integration testing. I'd create a new Account, addFunds(), clear caches, retrieve the account, assert on the balance, add funds again, clear the cache, retrieve it again, and assert on the value.

I'd try to think of other edge cases too. What if the account is invalid in some way? Maybe as your writing test code you realize it makes no sense to add funds to an account with no name or account number --maybe those are assumptions you should assert on. Maybe I want meaningful exceptions for those things.

Etc.

---

Interestingly, if you were TDD-ing this, it's entirely probable that addFunds() would be tested before saveOrUpdate() even exists. The tests for saveOrUpdate() would be written once some outer method's failing tests naturally demanded them (like those for addFunds()). Only then would you write saveOrUpdate().

If you're not doing TDD, shooting for 100% coverage, or worried about delivering fussy code, what you should do is entirely up to your comfort level. Or your team's comfort level. If no one on your team sees value in the tests, it's entirely possible they add no value.

Though, it's also entirely possible that skipping the tests on a method like this could cost your company millions someday ...

Answer 2

IMHO you are asking the wrong question. The fact saveOrUpdate is tested obviously does not guarantee the calculation logic in addFunds to be correct. So it is pretty clear the logic has to be tested somehow. So the question you need to ask here is, how to accomplish this?

1. Does one need an automated test, or is manually testing enough?

2. Do you need a real unit test, for testing the calculation logic in isolation, apart from saveOrUpdate?

3. And if the answer to the latter is "yes", should one write an additional (integration) test for addFund then?

Point 1 can only be answered by knowing the life cycle of your program and if the program or the function in stake will be subject of maintenance over a longer period, maybe years. If that's the case, test automation is typically worth it. (Obviously, it also matters if your program has a GUI which allows manual testing.)

Point 2 means one has to refactor the code, putting the logic (without the save operation) into a method of its own. If it is required depends on the desired distinction between unit and integration tests in your system. Does the call to saveOrUpdate may slow your unit tests suite too much down? Is it simpler to write unit tests when no call to saveOrUpdate is involved? That would be a strong indication for writing real unit tests.

Point 3 depends on how much effort it will mean in your system's context to set up stable, reproducable tests with a database involved (which I guess it is what happens when saveOrUpdate is called), and if that costs will be balanced by the benefits of an automated integration test. If not, making the unit test automated and apply the integration test manually can be perfectly fine.

Answer 3

When you (unit) test addFunds, you should not be testing whether saveOrUpdate works. You should be testing whether saveOrUpdate gets called (in the right circumstances) and leave it at that.

If you're going to unit test, any lower dependencies (such as the saveOrUpdate method) should be mocked. You are, after all, only trying to test the addFunds method. What you're suggesting in your question is that you test both addFunds and saveOrUpdate, which means you're no longer unit testing.

Never lose track of what you're testing.

Whenever you want to write a test, first ask yourself what you seek to prove with the test's results. If you can't think of anything meaningful, then you shouldn't be writing a test.

I don't think you need to test this method. As it is, all you're really going to be able to test is if the orchestration of the underlying methods (getCredits, setCredits, saveOrUpdate) is being called as it should.

But this test is superfluous. The orchestration of the three methods is trivially readable from simply looking at the code. The method doesn't have diverging logical paths (e.g. an if else structure) that causes the logical flow to be less than obvious.

Some counterexamples of methods that do warrant testing:

public void addFunds1(Account account, int price) {
    if(!account.IsLocked)
    {
        int credits = account.getCredits();
        account.setCredits(credits + positivePrice);
        saveOrUpdate(account);
    }
}

Here, there is a meaningful test: does the method correctly refuse to update accounts that are locked?

public void addFunds2(Account account, int price, Currency currency) {
    if(currency != account.Currency)
    {
         price = ConvertCurrency(currency, account.Currency, price);
    }

    int credits = account.getCredits();
    account.setCredits(credits + positivePrice);
    saveOrUpdate(account);
}

Here, there is a meaningful test: are we able to add any kind of currency to an account?

Take note that we are NOT testing:

• If the currency conversion is correct.
• If the equality check between currencies is implemented correctly.
• If the account update logic works.

What we ARE testing:

• If the method converts to the correct currency when the price currency is different from the account currency.

public void addFunds3(Account account, int price) {
    bool shouldUpdate = false;        

    if(!account.IsLocked)
    {
        shouldUpdate = account.Currency.Code.StartsWith("Z");
    }
    else
    {
        int foo = account.AgeInDays + account.getCredits() % 97;
        string fooResult = externalRepo.CalculateResult(foo);
        shouldUpdate = foo.Length > 3;
    }

    if(shouldUpdate)
    {
        int credits = account.getCredits();
        account.setCredits(credits + price);
        saveOrUpdate(account);
    }
}

I know this is a silly example, but it's intended as an example of complex and not at all obvious logic. There are many valid things to test here. A generalized test intention could be described as:

Since there are reasons to only update the account in certain situations, does the method correctly update the account in all needed circumstances where an update is required, and not update the account in all needed circumstances where an update is expressly prohibited? (This will be many different tests)

Answer 4

IMHO, whether saveOrUpdate has been tested has nothing to do with testing addFunds. Normally, when testing addFunds, you'd mock saveOrUpdate and check if it's called with correct arguments.

So yes, you should test it.