How to unit test a method whose side effect is to call other method?

Question

Here is my example:

void doneWithCurrentState(State state) {
    switch (state) {
        case State.Normal:
            // this method is never actually called with State.Normal
            break;
        case State.Editing:
            controller.updateViewState(State.Normal);
            database.updateObjectWithDetails(controller.getObjectDetailsFromViews())
            break;
        case State.Focus:
            controller.updateViewState(State.Editing);
            break;
    }
}

My controller calls the doneWithCurrentState when a specific button is pressed. The states are different positions on screen that the views for that controller can assume.

If the current state is Normal, the button will be hidden.

If the button is pressed with the current state as Editing, the doneWithCurrentState method (I say method because it is actually inside a class ``) will be called and it should change the controller's views state to Normal and update the Object in the database using the ObjectDetails (which is just a struct with data that will be used to update the Object) that should be retrieved from the controller's views (i.e., text fields, checkboxes, etc).

If the button is pressed with the current state as Focus, it should just send back to the Editing state.

I am unit testing it like this:

void testDoneWithCurrentStateEditing() {
    mockController.objectDetails = ...;

    myClass.doneWithCurrentState(State.Editing);

    AssertEqual(mockController.viewState, State.Normal, "controller state should change to Normal");

    AssertTrue(mockDatabase.updateObjectWithDetailsWasCalled, "updateObjectWithDetails should be called");
    AssertEqual(mockDatabase.updatedWithObjectDetail, mockController.objectDetails, "database should be updated with corresponding objectDetails");
}

void testDoneWithCurrentStateFocus() {
    myClass.doneWithCurrentState(State.Focus);

    AssertEqual(mockController.viewState, State.Editing, "controller state should change to Editing");

    AssertFalse(mockDatabase.updateObjectWithDetailsWasCalled, "updateObjectWithDetails should not be called");
}

But it seems wrong, it seems like I'm asserting a method call is made and then I'm making the call... it's just like asserting setter and getter methods.

What would be the right way of testing that doneWithCurrentState method? As part of the answer, I do accept something like "first you should refactor the method to better separate these concerns...".

Thank you.

Answer 1

I think Paul is spot on: put the state changes based on the incoming state into a state machine, i.e. an objects whose repsonsibility is to determine what comes next. This may sound dumb, because you kind of move the same code to another object, but at least this puts the controller on a diet. It shouldn't worry about too much details itself to be maintainable.

I worry about updateViewState, though. Why does it take the same kind of parameter as the controller's callback for user interaction? Can you model this differently? It's hard to tell you anything specific without looking at the flow of information (a detailed sequence diagram with comments might help), because usually the real insight into problems like these lies multiple levels deeper in the call stack. Without knowledge about the meaning of all this, it's hard to come up with a canned solution that fits.

Questions that might help:

• if State represents 3 (?) user interactions which all go through the same tunnel, can you model the actions to take as Strategy or Command?
• if doneWithCurrentState represents finishing one of many interaction modes, do you really need to use a shared doneWithCurrentState method? Couldn't you use three different callbacks? Maybe this is the wrong kind of abstraction. ("Don't Repeat Yourself" isn't about code but about things that change (in)dependently)

Answer 2

First of all, please consider using state machine for your state transitions, you will get out of switch statement branching business, which will result in a great simplification of your tests.

Next, treat your tests as a potential source for code and design smells. If it is hard to write a test for a piece of code - probably the code is lacking quality (breaking SRP, too coupled, etc.) and can be simplified/improved.

void doneWithCurrentState(State state) {
     State nextState = this.stateMachine.GetNextState(state);
     controller.updateViewState(nextState);

     if(nextState == State.Editing) 
         database.updateObjectWithDetails(controller.getObjectDetailsFromViews());
}

Then you can notice that you can pull out the call to the state machine of of the method and pass in the nextState.

//whoever calls this method should get nextState from state machine.
void doneWithCurrentState(State nextState) {
     controller.updateViewState(nextState);

     if(nextState == State.Editing) 
         database.updateObjectWithDetails(controller.getObjectDetailsFromViews());
}

and so forth.. you will write simple tests for state transitions in your state machine tests.. your overall code complexity gets down and all is goodness!? Well, there is hardly a limit to the level of goodness you can achieve and I can see multiple ways the code can be cleaned up even further.

As per your original question, how to test that code of your class makes a call on 'database' or 'controller' with proper parameters with specific state is passed in. You are doing it "right", that what mocks are meant to do. However, there are better ways. Consider event-based design. What if your controller could fire the events like "NextState" and your 'database' object can just subscribe to it? Then all your test needs to test is that the proper event is fired and not include anything about database (eliminating dependencies :))

Answer 3

If you wrote this not test-first, an obvious way to write it would be to write one case, then copy-paste into the next case. An easy mistake to make in that case would be to forget to update the parameter to updateViewState(). So (for instance) you might find yourself going from State.Focus to State.Normal. The test you've written, although it may seem weak to you, protects against mistakes of that nature. So I think it's doing what it should.