Testing procedure with several nested yield statements in Unity3D

Question

I made a small game that does the following:

1. Displays a popup with the instructions of the game
2. Turns on the GUI
3. Start a list of trials for the player
4. For each trial:
   1. It shows a symbol
   2. Waits for a set amount of time
   3. Show another symbol
   4. Waits for the user to give an answer or for a timer to expire
   5. Indicates the feedback for the answer for a set amount of time
   6. Waits for a while before beginning the next trial
5. Turns off the GUI

I am going to show some Unity3d's C# code. If you don't know Unity3D's API, please stick with me, I will make it as generic as possible, and I will explain Unity3D's specificities.

The following was my original code:

public class Game: Component {
    ...

    public IEnumerator<float> PlayGame(
        IGameDefinition gameDefinition,
        ICourutineManager cm
        )
    {
        CountDown answerCountdown = new CountDown(gameDefinition.answerTimemout);
        Trial[] trials = GenerateTrials(gameDefinition);
        PlayerAnswer playerAnswer;

        yield return cm.WaitForCoroutine(_OpenInstructionsAndWait());

        TurnOnGUI();

        yield return cm.WaitForSeconds(gameDefinition.waitBeforeBegin);

        foreach (Trial trial in trials)
        {
            FirstCue firstCue = GenerateFirstCue(trial, gameDefinition);
            ShowCue(firstCue);

            yield return cm.WaitForSeconds(gameDefinition.waitToShutOfCue1);

            HideCue(firstCue);

            SecondCue secondCue = GenerateSecondCue(trial, gameDefinition);
            ShowCue(secondCue);

            ActivateInput(ref playerAnswer);
            playerAnswer = PlayerAnswer.Unanswered;
            answerCountdown.Start();

            yield return cm.WaitWhile(() =>
                    playerAnswer == PlayerAnswer.Unanswered
                    &&
                    !answerCountdown.HasFinished()
                );

            Result result;

            if(answerCountdown.HasFinished()) {
                result = Result.Late;
            } else {
                result = CheckAnswer(playerAnswer);
            }

            HideCue(secondCue);
            DisplayFeedback(result);

            yield return cm.WaitForSeconds(gameDefinition.interTrialWait);
        }

        TurnOffGUI();
    }

    ...
}

The ICourutineManager is meant to decouple with Unity3d Coroutine's engine. What it does is to defer the execution of the current procedure until a certain condition is meet.

For example cm.WaitForSeconds(float seconds) will defer the execution for seconds seconds. cm.WaitForCoroutine will defer the execution until another "Coroutine" finishes, this Coroutine can contain several yield statements. Coroutines are procedures that return an IEnumerator.

This code was rather long, in my opinion, ill-suited for Unit testing. Most of the functions that it calls are private or protected, so I could not test them. Most of the state is local because I wanted to make pure functions whenever it was possible, so I could not track the state of the coroutine after each yield.

After a lot of research, I decided to move all those functions and local state to a separate class. Now all those functions and glue logic is grouped in public procedures that I can test and the local state variables (like firstCue and playerAnswer) are members of the new class, some of them are public.

But I am very disappointed with the result, before, it was a very readable code, but now, it is hard to read, no other class will use the new procedures, and I have a lot of side effects on methods that will run in a particular sequence. Not to mention that I don't think that I have made any progress to make this code more SOLID.

My new code looks like this:

public class Game: Component {
    ...

    public IEnumerator<float> PlayGame(
        IGameDefinition gameDefinition,
        IGameController gameController,
        ICourutineManager cm
        )
    {
        gameController.InitializeGame(gameDefinition);

        yield return cm.WaitForCoroutine(_OpenInstructionsAndWait());

        gameController.TurnOnGUI();

        yield return cm.WaitForSeconds(gameDefinition.waitBeforeBegin);

        foreach (Trial trial in gameController.trials)
        {
            gameController.ShowFirstCue(trial);

            yield return cm.WaitForSeconds(gameDefinition.waitToShutOfCue1);

            gameController.ShowSecondCue(trial);

            gameController.ActivateUserInput();

            yield return cm.WaitWhile(() =>
                    !gameController.HasPlayerAnswer()
                    &&
                    !gameController.AnswerCountdownHasFinished()
                );

            gameController.CheckAnswer(trial);

            gameController.ShowFeedback();

            gameController.DeactivateUserInput();

            yield return cm.WaitForSeconds(gameDefinition.interTrialWait);
        }

        gameController.FinishGame();
    }

    ...
}

I made a really bad job making this code testable. Can you help me to make it more SOLID, readable and testable?

Answer 1

Before I start, just a small disclaimer: I've only ever written C# in the context of Unity, and I'm going to be providing a good deal of controversial opinions. I do hope that I can back them up with objective reasoning.

One small point before we start proper: I don't think you should have all the abstraction you have on top of communicating to Unity (e.g. the ICoroutineManager). Unity's coroutine API isn't perfect, and I'm sure we'd all prefer being able to use async/await, but unless you're planning to switch out your engine, it's almost certainly better to use the API as Unity exposes it, rather than wrap it in an additional level of indirection that doesn't gain you much expressiveness. (In fact, unless you're running your method in another level of compatibility wrapper, I don't think it will work, as Unity's coroutine implementation expects a CustomYieldInstruction to identify how to wait, not floats. Why are you yielding floats, anyway?)

Pure functions are good, but a Coroutine is not pure.

A coroutine (in Unity) is, by definition, an iterator run for its side effect, not its yielded values. In the "common" case (running code over multiple frames), you're generating a stream of nulls.

It's easy to test a pure function: you give it arguments and check the return value. It's harder to test a function executed for its side effects, but it's still possible: you just check the state before and after to make sure that it is consistent with what you expect.

I don't understand why this change is necessary.

The testability of this method does not change between versions. If you give it mocks of the interfaces it takes, the best you can really do is assert that it calls the right methods in order, but you can tell that it does that from just looking at the code; you don't gain anything from testing obvious truths.

If the methods were only extracted to an IGameController in order to give a reason for those methods being public, then IGameController is a meaningless abstraction. Unity already offers Monobehaviour.Invoke(methodName), so you can (probably, untested) use that to test internal APIs. But that goes against the concept of unit testing: you test the external API surface.

This potentially shouldn't even be tested.

I've yet to find a good strategy for unit testing in games. You don't unit test the algorithm, you unit test the result, and that's a very ethereal concept in graphic applications. What is the goal of this method? You could say the list that you have given, but I would argue that that is not the result of the method, but rather the method by which the method achieves its goal.

A single-responsibility method does one thing, and that one thing is what you would test. In this case, the one thing is a higher level concept: present a set of trials to the user for completion.

What would I do?

It's a hard question. I don't think I have an answer, only suggestions. The way I understand unit tests is that they are written the same way that a consumer would use your code. In games, most of the code is consumed by the engine, not by other code, and as such, unit tests aren't as applicable. Unit tests should cover behaviors that could have their implementation change but should retain the same behavior.

The other way I use unit tests is as a sort of regression test. If you find a deficit in your product because of a method behaving incorrectly, write a failing unit test for it if it's a testable behavior. You then fix it and are guaranteed that the error won't return. Some interactions just aren't unit testable though, because they rely on the engine powering it.

---

But that said, here's how I believe I would currently design a method to present a set of trials. I will be using Unity's coroutine surface. I assumed your FirstCue and SecondCue could be merged into one conceptual Cue.

public IEnumerator PresentTrials(Trial[] trials, Settings settings)
{
    PlayerResponse? playerResponse;
    Stopwatch timer = new Stopwatch();

    yield return StartCoroutine(PresentInstructionPopup());
    EnableGUI();
    yield return new WaitForSeconds(settings.delayAfterGUIInit);

    foreach (var trial in trials)
    {
        playerResponse = null;

        Display(Cue.from(trial.first, settings));
        yield return new WaitForSeconds(settings.delayBetweenTrialCues);
        Display(Cue.from(trial.second, settings));

        ActivateInput(ref playerResponse);
        timer.StartNew();
        yield return new WaitWhile(() =>
            playerResponse == null && timer.Elapsed < settings.answerTimeout
        );
        timer.Stop();
        DisableInput();

        Display(Result.from(playerResponse));
        yield return new WaitForSeconds(settings.delayBetweenTrials);
    }

    DisableGUI();
}

Then, as building blocks which could potentially be tested, you have:

• this.Display(Cue)
• this.Display(Result)
• this.EnableGUI()
• this.DisableGUI()
• this.ActivateInput(ref PlayerResponse?)
• this.DeactivateInput()
• Cue.from(CueInfo, Settings): Cue
• Result.from(PlayerResponse?): Result

The only methods on this are those that deal directly with IO, and thus aren't directly unit testable. The array of trials to present and the configuration object are passed in.

---

## Addendum

Testing of IO code

IO methods like e.g. Display can't really be unit tested. Their job is to talk to the game engine to do something, and that communication can't be tested -- the best you could do is assert that the function calls certain library functions, but then you're doing a test of implementation rather than interface.

You could set up an integration test (or whatever the "proper" name is) checking that what is displayed on screen matches some parameters, but that is fragile at best. Similarly, just requiring certain calls to library code is just as limiting to future changes. When code's contract is to convey something to the user, there is no objective way to test that with code.

Testing private code

My mantra has always been "don't make code public just to test it", and I typically work in languages where reflection to call private APIs is not as simple as Invoke(name, args).

You should always test a method on its contract, not its implementation. Most of the time, private methods are implementation details which shouldn't be tested. However, it isn't impossible that you would have a private API with enough complexity that it's worth testing: at that point you should either extract it to its own symbol and make it only as exposed as it needs to be to be used in your original class. (For example, a package-private service provider.) If you can't or don't want to do that, testing private APIs is fine, when those APIs are well defined in terms of input/outcome and that definition is what you are testing.