Testing Codes Smells¶

For when you wake up. I wanted to discuss the approach to event dispatching a little more and try to explain a bit better what I am trying to get at.

The problem isn't the tests themselves - they are (for the most part) fine. The multiple patches isn't wrong, but it suggest the code is harder to isolate for testing than it could be. That in turn is indicative of a potential smell in the underlying code.

In particular the piece of code that this all comes back to is the dispatch function - event_handler

def event_handler(sender, instance, created, **kwargs):
    if not created:
        return

    if instance.event_type == EventTypes.INITIAL_EVENT:
        make_it_so(instance)

    elif instance.event_type == EventTypes.ANOTHER_EVENT:
        undo_previous(instance)
        make_it_so(instance)
        skip_a_thing(instance)

    elif instance.event_type == EventTypes.TERTIARY_IS_A_NICE_WORD:
        undo_previous(instance)
        skip_a_thing(instance)

This function is neither complex or hard to understand. It therefore follows that it really shouldn't be all that hard to test either. It certainly shouldn't need every test touching it to be updated every time a new event type is added to the system. This is a hot piece of code that's going to see lots of change over time. Therefore it will pay us back later to make it easier to maintain and by extension, easier to test.

I initially started going down a bit of a hole by implementing a listener/subscriber type approach that could be configured dynamically via settings. I swiftly realised that this isn't supposed to be a re-usable app that anyone can install and configure. It's a specific implementation for this project alone, and therefore playing at being a architectural astronaut was pretty silly.

What it did teach me however is that the core issue here is that there is a missing layer (of abstraction?).

What we have is a series of functions which are called in various combinations depending on the event type. Each of those individual functions was being patched because some of them are called as the result of multiple event types. It's easier to just patch them all to normalise the pattern for those who come after us to maintain or expand the code.

Things get easier if we instead normalise the code itself by insisting that every event needs it's own handler. That handler can do whatever it needs to. It can still call all the underlying shared functions. The key thing is that it is now 1 layer removed from the dispatching. This allows use to separate out the testing into one set of tests that test the dispatcher itself, and a set to tests the individual handlers (or even the underlying re-usable functions).

We can make out life super easy on the testing front by refactoring the event_handler method as bellow:

EVENT_HANDLERS = {
    EventTypes.INITIAL_EVENT: on_initial_event,
    EventTypes.ANOTHER_EVENT: on_another_event,
    EventTypes.TERTIARY_IS_A_NICE_WORD: on_the_use_of_nice_words
}

def _unknown_event_handler(instance):
    log.error(f"{instance.event_type} does not have any registered handlers. (PK: {instance.pk})")

def dispatch_event(sender, instance, created, **kwargs):
    if not created:
        return
    handler = EVENT_HANDLERS.get(instance.event_type, _unknown_event_handler)
    handler(instance)

Testing of the dispatch_event method then becomes quite straight forward.

One test to show that when an event type not present in the dictionary of EVENT_HANDLERS is dispatched it will result in a error log entry (there is a pytest fixture that allows log capture). One test that demonstrates that known event type is dispatched to the expected handler - this one can be done with a single patch to override the EVENT_HANDLERS dict to specify a specific handler and event type. And a final test that demonstrates the functionality of the initial not created guard clause.

With those 3(ish) tests you have 100% test coverage and only 1 minor use of patch. The testing of this function is then done forever and need not be touch or updated again as new events are added.

The individual handler tests follow the same pattern - you may want to use patch to fully isolate them but that patching doesn't need to grow for every new event or handler added. Ideally not at all, and the full process is followed.

I hope that this make a little more sense of my thinking around the testing, and why I think the multiple patches are indicative of a code smell - not in the tests themselves, but in the underlying code they are testing.