Where is the line between unit testing application logic and distrusting language constructs?

Question

Consider a function like this:

function savePeople(dataStore, people) {
    people.forEach(person => dataStore.savePerson(person));
}

It might be used like this:

myDataStore = new Store('some connection string', 'password');
myPeople = ['Joe', 'Maggie', 'John'];
savePeople(myDataStore, myPeople);

Let us assume that Store has its own unit tests, or is vendor-provided. In any case, we trust Store. And let us further assume that error handling -- eg, of database disconnection errors -- is not the responsibility of savePeople. Indeed, let us assume that the store itself is magical database that cannot possibly error in any way. Given these assumptions, the question is:

Should savePeople() be unit tested, or would such tests amount to testing the built-in forEach language construct?

We could, of course, pass in a mock dataStore and assert that dataStore.savePerson() is called once for each person. You could certainly make the argument that such a test provides security against implementation changes: eg, if we decided to replace forEach with a traditional for loop, or some other method of iteration. So the test is not entirely trivial. And yet it seems awfully close...

---

Here's another example that may be more fruitful. Consider a function that does nothing but coordinate other objects or functions. For example:

function bakeCookies(dough, pan, oven) {
    panWithRawCookies = pan.add(dough);
    oven.addPan(panWithRawCookies);
    oven.bakeCookies();
    oven.removePan();
}

How should a function like this be unit tested, assuming you think it should? It's hard for me to imagine any kind of unit test that doesn't simply mock dough, pan, and oven, and then assert that methods are called on them. But such a test is doing nothing more than duplicating the exact implementation of the function.

Does this inability to test the function in a meaningful black box way indicate a design flaw with the function itself? If so, how could it be improved?

---

To give even more clarity to the question motivating the bakeCookies example, I'll add a more realistic scenario, which is one I've encountered when attempting to add tests to and refactor legacy code.

When a user creates a new account, a number of things need to happen: 1) a new user record needs to be created in the database 2) a welcome email needs to be sent 3) the user's IP address needs to be recorded for fraud purposes.

So we want to create a method that ties together all the "new user" steps:

function createNewUser(validatedUserData, emailService, dataStore) {
  userId = dataStore.insertUserRecord(validateduserData);
  emailService.sendWelcomeEmail(validatedUserData);
  dataStore.recordIpAddress(userId, validatedUserData.ip);
}

Note that if any of these methods throws an error, we want the error to bubble up to the calling code, so that it can handle the error as it sees fit. If it's being called by the API code, it may translate the error into an appropriate http response code. If it's being called by a web interface, it may translate the error into an appropriate message to be displayed to the user, and so on. The point is this function doesn't know how to handle the errors that may be thrown.

The essence of my confusion is that to unit test such a function it seems necessary to repeat the exact implementation in the test itself (by specifying that methods are called on mocks in a certain order) and that seems wrong.

Answer 1

Should savePeople() be unit tested? Yes. You aren't testing that dataStore.savePerson works, or that the db connection works, or even that the foreach works. You are testing that savePeople fulfills the promise it makes through its contract.

Imagine this scenario: someone does a big refactor of the code base, and accidentally removes the forEach part of the implementation so that it always only saves the first item. Wouldn't you want a unit test to catch that?

Answer 2

Usually this kind of question comes up when people do "test-after" development. Approach this problem from the point of view of TDD, where tests come before the implementation, and ask yourself this question again as an exercise.

At least in my application of TDD, which is usually outside-in, I'd not be implementing a function like savePeople after having implemented savePerson. The savePeople and savePerson functions would start as one and being test-driven from the same unit tests; the separation between the two would arise after a few tests, in the refactoring step. This mode of work would also pose the question of where the function savePeople ought to be - whether it is a free function or part of the dataStore.

In the end, the tests would not only check if you can correctly save a Person in the Store, but also many persons. This would also lead me to question if other checks are necessary, for instance, "Do I need to make sure that the savePeople function is atomic, either saving all or none?", "Can it just somehow return errors for the people that couldn't be saved? How would those errors look like?", and so on. All this amounts to much more than just checking for the use of a forEach or other forms of iteration.

Though, if the requirement of saving more than one person at once came only after savePerson was already delivered, then I'd update the existing tests of savePerson to run through the new function savePeople, making sure it still can save one person by simply delegating at first, then test-drive the behavior for more than one person through new tests, thinking if it would be necessary to make the behavior atomic or not.

Answer 3

Should savePeople() be unit tested

Yes, it should. But try to write your test conditions in a way that is independent from the implementation. For example, turning your usage example into a unit test:

function testSavePeople() {
    myDataStore = new Store('some connection string', 'password');
    myPeople = ['Joe', 'Maggie', 'John'];
    savePeople(myDataStore, myPeople);
    assert(myDataStore.containsPerson('Joe'));
    assert(myDataStore.containsPerson('Maggie'));
    assert(myDataStore.containsPerson('John'));
}

This test does multiple things:

• it verifies the contract of the function savePeople()
• it does not care about the implementation of savePeople()
• it documents the example usage of savePeople()

Take note that you can still mock/stub/fake the data store. In this case I wouldn't check for explicit function calls, but for the result of the operation. This way my test is prepared for future changes/refactors.

For example, your data store implementation might provide a saveBulkPerson() method in the future - now a change to the implementation of savePeople() to use saveBulkPerson() would not break the unit test as long as saveBulkPerson() works as expected. And if saveBulkPerson() somehow does not work as expected, your unit test will catch that.

or would such tests amount to testing the built-in forEach language construct?

As said, try to test for expected results and the function interface, not for the implementation (unless you are doing integration tests - then catching specific function calls might be of use). If there are multiple ways to implement a function, all of them should work with your unit test.

Regarding your update of the question:

Test for state changes! E.g. some of the dough will be used. According to your implementation, assert that the amount of used dough fits into pan or assert that the dough is used up. Assert that the pan contains cookies after the function call. Assert that the oven is empty/in the same state as before.

For additional tests, verify edge cases: What happens if the oven is not empty before the call? What happens if there isn't enough dough? If the pan is already full?

You should be able to deduce all the required data for these tests from the dough, pan and oven objects themselves. No need to capture the function calls. Treat the function as if its implementation would not be available to you!

In fact, most TDD users write their tests before they write the function so they are not dependent on the actual implementation.

---

For your latest addition:

When a user creates a new account, a number of things need to happen: 1) a new user record needs to be created in the database 2) a welcome email needs to be sent 3) the user's IP address needs to be recorded for fraud purposes.

So we want to create a method that ties together all the "new user" steps:

function createNewUser(validatedUserData, emailService, dataStore) {
    userId = dataStore.insertUserRecord(validateduserData);
    emailService.sendWelcomeEmail(validatedUserData);
    dataStore.recordIpAddress(userId, validatedUserData.ip);
}

For a function like this i would mock/stub/fake (whatever seems more general) the dataStore and emailService parameters. This function does not do any state transitions on any parameter on its own, it delegates them to methods of some of them. I would try to verify that the call to the function did 4 things:

• it inserted a user into the data store
• it sent (or at least called the corresponding method) a welcome email
• it recorded the users IP into the data store
• it delegated any exception/error it encountered (if any)

The first 3 checks can be done with mocks, stubs or fakes of dataStore and emailService (you really don't want to send emails when testing). Since I had to look this up for some of the comments, these are the differences:

• A fake is an object that behaves the same as the original and is to a certain extent indistinguishable. Its code can normally be reused across tests. This can, for example, be a simple in-memory database for a database wrapper.
• A stub just implements as much as needed to fulfill the required operations of this test. In most cases, a stub is specific to a test or a group of tests requiring only a small set of the methods of the original. In this example, it could be a dataStore that just implements a suitable version of insertUserRecord() and recordIpAddress().
• A mock is an object that lets you verify how it is used (most often by letting you evaluate calls to its methods). I'd try to use them sparingly in unit tests since by using them you actually try to test the function implementation and not the adherence to its interface, but they still have their uses. Many mock frameworks exists to help you create just the mock you need.

Note that if any of these methods throws an error, we want the error to bubble up to the calling code, so that it can handle the error as it sees fit. If it's being called by the API code, it may translate the error into an appropriate HTTP response code. If it's being called by a web interface, it may translate the error into an appropriate message to be displayed to the user, and so on. The point is this function doesn't know how to handle the errors that may be thrown.

Expected exceptions/errors are valid test cases: You confirm, that, in case such an event happens, the function behaves the way you expect it would. This can be achieved by letting the corresponding mock/fake/stub object throw when desired.

The essence of my confusion is that to unit test such a function it seems necessary to repeat the exact implementation in the test itself (by specifying that methods are called on mocks in a certain order) and that seems wrong.

Sometimes this has to be done (though you mostly care about this in integration tests). More often, there are other ways to verify the expected side effects/state changes.

Verifying exact functions calls makes for rather brittle unit tests: Only small changes to the original function causes them to fail. This can be desired or not, but it requires a change to the corresponding unit test(s) whenever you change a function (be it refactoring, optimizing, bug fixing, ...).

Sadly, in that case the unit test loses some of its credibility: since it was changed, it does not confirm the function after the change behaves the same way as before.

For an example, consider someone adding a call to oven.preheat() (optimization!) in your cookie baking example:

• If you mocked the oven object, it won't expect that call and fail the test, although the observable behavior of the method did not change (you still have a pan of cookies, hopefully).
• A stub might or might not fail, depending on whether you only added the methods to be tested or the whole interface with some dummy methods.
• A fake should not fail, since it should implement the method (according to the interface)

In my unit tests, I try to be as general as possible: If the implementation changes, but the visible behavior (from the perspective of the caller) is still the same, my tests should pass. Ideally, the only case I need to change an existing unit test should be a bug fix (of the test, not the function under test).

Answer 4

The primary value such a test provides is that it makes your implementation refactorable.

I used to do a lot of performance optimizations in my career and often found problems with the exact pattern you demonstrated: to save N entities into the database, perform N inserts. It's usually more efficient to do a bulk insert using a single statement.

On the other hand, we don't want to prematurely optimize, either. If you typically only save 1 - 3 people at a time, then writing an optimized batch may be overkill.

With a proper unit test, you can write it the way you implemented it above, and if you find you need to optimize it, you are free to do so with the safety net of an automated test to catch any errors. Naturally, this varies based on the quality of the tests, so test liberally and test well.

The secondary advantage to unit testing this behavior is to serve as documentation for what its purpose is. This trivial example may be obvious, but given the next point below, it could be very important.

The third advantage, which others have pointed out, is that you can test under-the-covers details which are very difficult to test with integration or acceptance tests. For example, if there is a requirement that all users be saved atomically, then you can write a test case for that, which gives you a way to know it behaves as expected, and also serves as documentation for a requirement which may not be obvious to new developers.

I will add a thought which I got from a TDD instructor. Don't test the method. Test the behavior. In other words, you don't test that savePeople works, you are testing that multiple users can be saved in a single call.

I found my ability to do quality unit testing and TDD improve when I stopped thinking about unit tests as verifying that a program works, but rather, they verify that a unit of code does what I expect. Those are different. They don't verify it works, but they verify it does what I think it does. When I began thinking that way, my perspective changed.

Answer 5

Should bakeCookies() be tested? Yes.

How should a function like this be unit tested, assuming you think it should? It's hard for me to imagine any kind of unit test that doesn't simply mock dough, pan, and oven, and then assert that methods are called on them.

Not really. Look closely at WHAT the function is supposed to do - it is supposed to set the oven object to a specific state. Looking at the code it appears that the states of the pan and dough objects does not really matter much. So you should pass an oven object (or mock it) and assert that it is in a particular state at the end of the function call.

In other words, you should assert that bakeCookies() baked the cookies.

For very short functions, unit tests may appear to be little more than tautology. But don't forget, your program is going to last a lot longer than the time you are employed writing it. That function may or may not change in the future.

Unit tests serves two functions:

1. It tests that everything works. This is the least useful function unit tests serves and it appears that you seem to only consider this functionality when asking the question.

2. It checks to see that future modifications of the program does not break functionality that was previously implemented. This is the most useful function of unit tests and it prevents introduction of bugs into large programs. It is useful in normal coding when adding features to the program but it is more useful in refactoring and optimisations where the core algorithms implementing the program are dramatically changed without changing any observable behaviour of the program.

Do not test the code inside the function. Instead test that the function does what it says it does. When you look at unit tests this way (testing functions, not code) then you will realise that you never test language constructs or even application logic. You are testing an API.

Answer 6

Should savePeople() be unit tested, or would such tests amount to testing the built-in forEach language construct?

Yes. But you could do it in a way that would just retest the construct.

The thing to note here is how does this function behave when a savePerson fails half way through? How is it supposed to work?

That is the sort of subtle behavior that the function provides that you should enforce with unit tests.

Answer 7

The key here is your perspective on a particular function as trivial. Most of programming is trivial: assign a value, do some math, make a decision: if this then that, continue a loop until... In isolation, all trivial. You just got through the first 5 chapters of any book teaching a programming language.

The fact that writing a test is so easy should be a sign that your design is not that bad. Would you prefer a design that is not easy to test?

"That will never change." is how most failed projects start out. A unit test only determines if the unit works as expected under a certain set of circumstances. Get it to pass and then you can forget about its implementation details and just use it. Use that brain space for the next task.

Knowing things work as expected is very important and not trivial in large projects and especially large teams. If there's one thing programmers have in common, is the fact we've all had to deal with someone else's terrible code. The least we can do is have some tests. When in doubt, write a test and move on.

Answer 8

I think your question boils down to:

How do I unit test a void function without it being an integration test?

If we change your cookie baking function to return cookies for example it becomes immediately obvious what the test should be.

If we have to call pan.GetCookies after calling the function though we can question whether its 'really an integration test' or 'but arent we just testing the pan object?'

I think you are correct in that having unit tests with everything mocked and just checking functions x y and z were called lack value.

But! I would argue that in these case you should refactor your void functions to return a testable result OR use real objects and make an integration test

--- Update for the createNewUser example

• a new user record needs to be created in the database
• a welcome email needs to be sent
• the user's IP address needs to be recorded for fraud purposes.

OK so this time the result of the function is not easily returned. We want to change the state of the parameters.

This is where I get slightly controversial. I create concrete mock implementations for the stateful parameters

please, dear readers, try and control your rage!

so...

var validatedUserData = new UserData(); //we can use the real object for this
var emailService = new MockEmailService(); //a simple mock which saves sentEmails to a List<string>
var dataStore = new MockDataStore(); //a simple mock which saves ips to a List<string>

//run the test
target.createNewUser(validatedUserData, emailService, dataStore);

//check the results
Assert.AreEqual(1, emailService.EmailsSent.Count());
Assert.AreEqual(1, dataStore.IpsRecorded.Count());
Assert.AreEqual(1, dataStore.UsersSaved.Count());

This separates the implementation detail of the method under test from the desired behavior. An alternate implementation :

function createNewUser(validatedUserData, emailService, dataStore) {
  userId = dataStore.bulkInsedrtUserRecords(new [] {validateduserData});
  emailService.addEmailToQueue(validatedUserData);
  emailService.ProcessQueue();
  dataStore.recordIpAddress(userId, validatedUserData.ip);
}

Will still pass the unit test. Plus you have the advantage of being able to reuse the mock objects across tests and also inject them into your application for UI or Integration Tests.

Answer 9

Should savePeople() be unit tested, or would such tests amount to testing the built-in forEach language construct?

This has already been answered by @BryanOakley, but I have some extra arguments (I guess):

First a unit test is for testing the fulfillment of a contract, not the implementation of an API; the test should set preconditions then call, then check for effects, side effects, any invariants and post conditions. When you decide what to test, the implementation of the API doesn't (and shouldn't) matter.

Second, your test will be there to check the invariants when the function changes. The fact it doesn't change now doesn't mean you shouldn't have the test.

Third, there is value in having implemented a trivial test, both in a TDD approach (which mandates it) and outside of it.

When writing C++, for my classes, I tend to write a trivial test that instantiates an object and checks invariants (assignable, regular, etc). I found it surprising how many times this test is broken during development (by - for example - adding a non-movable member in a class, by mistake).

Answer 10

You should also test bakeCookies - what would/should e..g bakeCookies(egg, pan, oven) yield? Fried egg or an exception? On their own, neither pan nor oven will care about the actual ingredients, since none of them are supposed to, but bakeCookies should usually yield cookies. More generally it can depend on how dough is obtained and whether there is any chance of it becoming mere egg or e.g. water instead.