How do you unit test functions split in smaller functions

Question

The problem is the following, suppose we have this functions:

from PIL import Image
from magiclibrary import perform_some_operation, stack_images


def load_image(path: str):
    if isfile(path):
        img = Image.open(path)
        perform_some_operation(img)
        return img
    else:
        throw FileNotFoundException
    
def stack_images(paths: List[str]):
    imgs = list()
    for path in paths:
        imgs.append(load_image(path))
    return stacks_images(imgs)

Suppose that checking that the path exists is the responsability of the function load_image. We could write two tests, one for a path to an existing image and one to a wrong path. The problem arises with our second function, stack_images.

My research has led me to think that the usual way to deal with this kind of problem is to mock the inner function, and check if it was called. I think that is a bad idea because doing that you are linking the tests to the implementation of the method, and not the use cases. If at any point in the future you want to change the function for whatever reason, the method may still act properly in use cases and in corner cases, but the tests will signal that there is a problem.

The other approach is giving stack_images a list where all paths lead to existing images, and another case where some or all the paths are wrong. IMHO, checking this should not be the responsability for this method, and we already tested that the inner method performs properly when this is the case.

My concern with this second approach is that if load_image wasn't a method, but was just written inline inside stack_images where the call is, stack_images would have the same 2 tests required but we would have 2 tests less, because since load_image is not a method anymore, we don't have to test it.

2 extra tests doesn't seem much, but what if this pattern is repeated across 5 diferent functions to enhance maintainability? Should we test the same 2 cases for the 5 functions? This seems inherently bad for me.

My question is, how I'm supposed to test stack_images?

Edit: I think this is not a duplicate of this question because they talk about mocking the data sources for the top function, but in this case we are not talking about I/O or web functions, we are talking about how to manage the responsabilities of inner functions in a general way, when testing a function that uses this functions in its implementation.

Answer 1

The method stack_images seems to have two concerns: loading images, and actually stacking them. I'd add a load_images method, and have stack_images take images instead of paths. That should make testing easier.

---

However, figuring out how to test code without modifying it has value (it is what we would do with legacy code, so we have the test as a safety net before we dare to modify the code). So, let us talk about your concerns:

My concern with this second approach is that if load_image wasn't a method, but was just written inline inside stack_images where the call is, stack_images would have the same 2 tests required but we would have 2 tests less, because since load_image is not a method anymore, we don't have to test it.

Yes, that is the way it is. The motivation I'd have to refactor load_image out is that I'd consider the files external, and thus I'd want to isolate and minimize the code that deals with them.

---

2 extra tests doesn't seem much, but what if this pattern is repeated across 5 diferent functions to enhance maintainability?

I don't know how repeating the code to load images enhances maintainability. I'd argue it degrades it, any defect found in it has to be fixed in multiple places.

Should we test the same 2 cases for the 5 functions?

If you need to test the system without modifying it, then yes you need to test all the methods that take paths handle not-existing paths correctly.

This seems inherently bad for me.

Yes, that is bad. You can consider it a hint to change the design.

---

After you have your tests, you would refactor that code to load images to a separate method (the tests would tell you if your refactoring broke something), and there would be tests for that separate method.

I would encourage to make methods that take images instead of paths. That is, I would make their responsibility to work with images only, not with files. Which suggest I need a load_images method (note the plural). These methods would not need tests for not-existing paths on all those methods, since they would not take paths at all.

---

If you insist in keeping methods that take paths, have them call the methods that take images. Which would make their implementation trivial. If you insist in testing them despite their trivial implementation over tested methods, you can have dependencies injected on initialization which you can mock. And use them to test they call the right methods. By the way, can you make parameterized tests? Specify what you actually call as a parameter for the test. That way, you can have the same test for multiple methods. This is all waste effort from my point of view.

Answer 2

My question is, how I'm supposed to test stack_images?

def stack_images(paths: List[str]):
    return stacks_images_v2(paths, load_image)

def stack_images_v2(paths: List[str], fn):
    imgs = list()
    for path in paths:
        imgs.append(fn(path))
    return stacks_images(imgs)

Written this way, stack_images is "so simple there are obviously no deficiencies". So long as we pull load_image out of the correct namespace, any problems that arise are going to be "somewhere else". Put whatever comments/annotations/tags on the code you need to indicate that this code requires human inspection when it changes, put your name and the data on it, and move on.

stack_images_v2 is supposed to work with any function, including both load_image and any substitute that produces an output with the right shape. So you can test it in isolation, by passing in a stable substitute function.

I'd give full credit for putting this function in the obviously no deficiencies category, especially if you were to refactor it like so:

def stack_images_v2(paths: List[str], fn):
    return stacks_images(
        [fn(path) for path in paths]
    )

---

I don't understand how this solves our problem, provided that stack_images_v2 and stack_images can still throw a FileNotFoundError with inexistent paths.

No, they propagate an exception that is thrown by load_image, or its substitute.

If you want to test that the exception propagates correctly, then you test stack_images_v2 by passing in a function that always throws, and measure that stack_images_v2 behaves correctly.

If you want to test that stack_images propagates an exception correctly, you look at it and sign off on it.

---

Your substitute function, used to measure the behavior of stack_images_v2 when an exception is thrown might look like:

def substitute_that_throws(path):
    throw FileNotFoundException

An important thing to notice is that substitute_that_throws doesn't need to change for domain related reasons - it doesn't have any "domain logic" that might change over time. It only changes for test reasons. Therefore, it is a much more stable than the "real" implementation, and your test is isolated from unrelated changes to the domain.

See Parnas 1971 on information hiding.

Of course, if load_image changes enough - in particular if the contract supported by load_image changes in a not-backwards-compatible way, then substitute_that_throws stops being a substitute for that implementation, and you may need to introduce new tests with new substitutes, and deprecate the old ones.