86

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.

  • 43
    After it runs. Do you have cookies – Ewan Jun 22 '16 at 6:15
  • 6
    regarding your update: why would you ever want to mock a pan? or dough? these sound like simple in-memory objects that should be trivial to create, and so there's no reason why you shouldn't test them as one unit. remember, the "unit" in "unit testing" doesn't mean "a single class". it means "the smallest possible unit of code that is used to get something done". a pan is probably nothing more than a container for dough objects, so it'd be contrived to test it in isolation instead of just test-driving the bakeCookies method from the outside in. – sara Jun 22 '16 at 7:25
  • 11
    At the end of the day, the fundamental principle at work here is that you write enough tests to assure yourself that the code works, and that it's an adequate "canary in the coal mine" when someone changes something. That's it. There are no magical incantations, formulaic suppositions or dogmatic assertions, which is why 85% to 90% code coverage (not 100%) is widely considered excellent. – Robert Harvey Jun 22 '16 at 15:17
  • 5
    @RobertHarvey unfortunately formulaic platitudes and TDD sound bites, while sure to earn you enthusiastic nods of agreement, don't help solve real-world problems. for that you need to get your hands dirty and risk answering an actual question – Jonah Jun 22 '16 at 15:25
  • 3
    Unit test in order of decreasing cyclomatic complexity. Trust me, you'll run out of time before you get to this function – Neil McGuigan Jun 23 '16 at 23:50

10 Answers 10

118

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?

  • 20
    @RobertHarvey: There's a lot of gray area, and making the distinction, IMO, isn't important. You are right though -- it's not really important to test that "it calls the right functions" but rather "it does the right thing" regardless of how it does it. What is important is to test that, given a specific set of inputs to the function you get a specific set of outputs. However, I can see how that last sentence can be confusing, so I removed it. – Bryan Oakley Jun 22 '16 at 2:25
  • 64
    "You are testing that savePeople fulfills the promise it makes through its contract." This. So much this. – Lovis Jun 22 '16 at 7:24
  • 2
    Unless you have a "end to end" system test that covers it. – Ian Jun 22 '16 at 11:32
  • 6
    @Ian End to end tests do not replace unit tests, they are complimentary. Just because you may have an end to end test that ensures you cave save a list of people does not mean you shouldn't have a unit test to also cover it. – Vincent Savard Jun 22 '16 at 12:43
  • 4
    @VincentSavard, but the cost/benefit of a unit test is reduce if the risk is control in anther way. – Ian Jun 22 '16 at 14:22
37

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.

  • 4
    Basically test the interface, not the implementation. – Snoop Jun 22 '16 at 2:15
  • 8
    Fair and insightful points. Yet I feel somehow my real question is being dodged :) Your answer is saying, "In the real world, in a well designed system, I don't think this simplified version of your problem would exist." Again, fair, but I specifically created this simplified version to highlight the essence of a more general problem. If you can't get past the artificial nature of the example, you could perhaps imagine another example in which you did have a good reason for a similar function, which did only iteration and delegation. Or perhaps you think that it is simply impossible? – Jonah Jun 22 '16 at 2:16
  • @Jonah updated. I hope it answers your question a bit better. This is all very opinion-based and may be against the goal for this site, but it is certainly a very interesting discussion to have. By the way, I tried to answer from the point of view of professional work, where we must strive to leave unit tests for all application behavior, regardless of how trivial the implementation may be, because we have the duty to build a well tested and documented system for new maintainers if we leave. For personal or, say, non-critical (money is also critical) projects, I have a very different opinion. – MichelHenrich Jun 22 '16 at 2:54
  • Thanks for the update. How exactly would you test savePeople? As I described in the last paragraph of OP or some other way? – Jonah Jun 22 '16 at 3:11
  • 1
    Sorry, I didn't make myself clear with the "no mocks involved" part. I meant I wouldn't use a mock for the savePerson function like you suggested, instead I'd test it through the more general savePeople. The unit tests for Store would be changed to run through savePeople rather than directly call savePerson, so for this, no mocks are used. But the database of course should not be present, as we'd like to isolate coding problems from the various integration problems that occur with actual databases, so here we still have a mock. – MichelHenrich Jun 22 '16 at 3:53
21

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).

  • 1
    The problem is that as soon as you write myDataStore.containsPerson('Joe') you are assuming the existence of a functional test database. Once you do that, you are writing an integration test and not a unit test. – Jonah Jun 22 '16 at 14:06
  • I assume that I can rely on having a test data store (i don't care if that's a real or a mock one) and that everything works as set up (since I should have unit tests for those cases already). The only thing the test wants to test is that savePeople() actually adds those people to whatever data store you provide as long as that data store implements the expected interface. An integration test would be, for example, checking that my database wrapper actually does the right database calls for a method call. – hoffmale Jun 22 '16 at 15:13
  • To clarify, if you are using a mock, all you can do is assert that a method on that mock was called, perhaps with some specific parameter. You cannot assert on the state of the mock afterwards. So if you want to make assertions on the state of the database after calling the method under test, as in myDataStore.containsPerson('Joe'), you have to use a functional db of some kind. Once you take that step it's no longer a unit test. – Jonah Jun 22 '16 at 15:52
  • 1
    it does not have to be a real database - just an object that implements the same interface as the real data store (read: it passes the relevant unit tests for the data store interface). I'd still consider this a mock. Let the mock store everything that gets added by any method for doing so in an array and check whether the test data (elements of myPeople) are in the array. IMHO a mock should still have the same observable behavior that is expected from a real object, otherwise you are testing for compliance with the mock interface, not the real interface. – hoffmale Jun 22 '16 at 16:40
  • "Let the mock store everything that gets added by any method for doing so in an array and check whether the test data (elements of myPeople) are in the array" -- that's still a "real" database, just an ad-hoc, in-memory one you have built. "IMHO a mock should still have the same observable behavior that is expected from a real object" -- I suppose you can advocate for that, but that's not what "mock" means in the testing literature or in any of the popular libraries I've seen. A mock simply verifies that expected methods are called with expected parameters. – Jonah Jun 22 '16 at 17:03
13

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.

  • The bulk insert refactoring example is a good one. The possible unit test I suggested in the OP -- that a mock of dataStore has savePerson called on it for each person in the list -- would break with a bulk insert refactoring, however. Which to me indicates that it's a poor unit test. However, I don't see an alternative one that would pass both the bulk and one-save-per-person implementations, without using an actual test database and asserting against that, which seems wrong. Could you provide a test that works for both implementations? – Jonah Jun 22 '16 at 2:45
  • 1
    @jpmc26 What about a test that tests that the people were saved...? – immibis Jun 22 '16 at 23:48
  • 1
    @immibis I don't understand what that means. Presumably, the real store is backed by a database, so you'd have to mock or stub it for a unit test. So at that point, you would be testing that your mock or stub can store objects. That's utterly useless. The best you could do is assert that the savePerson method was called for each input, and if you replaced the loop with a bulk insert, you wouldn't be calling that method anymore. So your test would break. If you have something else in mind, I'm open to it, but I don't see it yet. (And not seeing it was my point.) – jpmc26 Jun 23 '16 at 0:16
  • 1
    @immibis I don't consider that a useful test. Using the fake data store doesn't give me any confidence it will work with the real thing. How do I know my fake works like the real thing? I would rather just let a suite of integration tests cover it. (I should probably clarify that I meant "any unit test" in my first comment here.) – jpmc26 Jun 23 '16 at 2:20
  • 1
    @immibis I'm actually reconsidering my position. I've been skeptical about the value of unit tests because of problems like this, but maybe I'm underestimating the value even if you fake out an input. I do know that input/output testing tends to be much more useful than mock heavy tests, but maybe a refusal to replace an input with a fake is actually part of the problem here. – jpmc26 Jun 23 '16 at 16:28
6

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.

  • Hi, thanks for you reply. Would you mind looking at my 2nd update and giving your thoughts about how to unit test the function in that example? – Jonah Jun 22 '16 at 13:56
  • I find that this can be effective when you can either use a real oven or a fake oven, but is significantly less effective with a mock oven. Mocks (by the Meszaros defintions) don't have any state, other than a record of the methods that have been called on them. My experience is that when functions like bakeCookies are tested in this way, they tend to break during refactors that would not impact the observable behaviour of the application. – James_pic Jun 22 '16 at 16:55
  • @James_pic, exactly. And yes, that is the mock definition I am using. So given your comment, what do you do in a case like this? Forgo the test? Write the brittle, implementation-repeating test anyway? Something else? – Jonah Jun 22 '16 at 21:02
  • @Jonah I'll generally either look at testing that component with integration tests (I've found the warnings about it being harder to debug to be overblown, possibly due to the quality of modern tooling), or go to the trouble of creating a semi-convincing fake. – James_pic Jun 23 '16 at 8:44
3

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.

  • Yes, I agree that subtle error conditions should be tested, but imo that is not an interesting question -- the answer is clear. Hence the reason I specifically stated that, for purposes of my question, savePeople should not be responsible for error handling. To clarify again, assuming that savePeople is responsible only for iterating through the list and delegating the saving of each item to another method, should it still be tested? – Jonah Jun 22 '16 at 2:06
  • @Jonah: If you're going to insist on confining your unit test solely to the foreach construct, and not any conditions, side effects or behaviors outside of it, then you're right; the new unit test is really not all that interesting. – Robert Harvey Jun 22 '16 at 2:27
  • 1
    @jonah - should it iterate through and save as many as possible or stop on error? The single save can't decide that, since it can't know how it's being used. – Telastyn Jun 22 '16 at 2:44
  • 1
    @jonah - welcome to the site. One of the key components of our Q&A format is that we're not here to help you. Your question helps you of course, but it also helps many other people who come to the site looking for answers to their questions. I answered the question you asked. It's not my fault if you don't like the answer or would rather shift the goalposts. And frankly, it looks like the other answers say the same basic thing, albeit more eloquently. – Telastyn Jun 22 '16 at 13:26
  • 1
    @Telastyn, I'm trying to gain insight about unit testing. My initial question was not clear enough, so I'm adding clarifications to steer the conversation toward my real question. You are choosing to interpret that as me somehow cheating you in the game of "being right." I have spent hundreds of hours answering questions on code review and SO. My purpose is always to help the people I'm answering. If yours isn't, that's your choice. You don't have to answer my questions. – Jonah Jun 22 '16 at 13:35
3

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.

  • Thanks for you feedback. Good points. The question I really want answered (I just added yet another update to clarify) is the appropriate way to test functions that do nothing more than call a sequence of other services through delegation. In such cases, it seems the unit tests appropriate to "document the contract" are just a restatement of the function implementation, asserting that methods are called on various mocks. Yet the test being identical to the implementation in those cases feels wrong.... – Jonah Jun 22 '16 at 13:45
1

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).

1

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.

  • it's not an integration test simply because you mention the names of 2 concrete classes ... integration tests are about testing integrations with external systems, like disk IO, the DB, external web services and so on.calling pan.getCookies() is in-memory, fast, checks for the thing we're interested in etc. I agree that having the method return the cookies directly feels like a better design though. – sara Jun 22 '16 at 7:30
  • 3
    Wait. For all we know pan.getcookies sends an email to a cook asking them to take the cookies out of the oven when they get a chance – Ewan Jun 22 '16 at 7:49
  • I guess it's theoretically possible, but it'd be a pretty misleading name. who ever heard of oven equipment that sent emails? but I see you point, it depends. I assume these collaborating classes are leaf objects or just plain in-memory things, but if they do sneaky stuff, then caution is needed. I think email sending definitely should be done at a higher level than this though. this seems like the down and dirty business logic in the entities. – sara Jun 22 '16 at 8:16
  • 2
    It was a rhetorical question, but: "who ever heard of oven equipment that sent emails?" venturebeat.com/2016/03/08/… – clacke Jun 22 '16 at 12:38
  • Hi Ewan, I think this answer is coming closest so far to what I'm really asking. I think your point about bakeCookies returning the baked cookies is spot on, and I had some thought after posting it. So I think it's yet again not a great example. I added one more update that hopefully provides a more realistic example of what's motivating my question. I'd appreciate your input. – Jonah Jun 22 '16 at 13:40
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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