When having to do a design change, should I change the already working tests first and only then try to run the new one?

Question

I have a bunch of tests that assume that my Tetris class is composed by a Board class. I now feel that instead of having a Board inside of that Tetris class, what I will want is to have a Board inside a BoardEngine class that is inside the Tetris class. That is what this test is asking for:

    [TestMethod]
    public void Start_Game_And_Check_That_A_Red_Cube_Appears_And_Moves_Down_One_Pixel_On_First_Iteration() {
        Board board = new Board(10, 22);
        BoardEngine boardEngine = new BoardEngine(board);
        Tetris tetris = new Tetris(boardEngine);

        //test that the pixels are black/empty at first
        Assert.AreNotEqual(Color.Red, tetris.GetColorAt(0, 0));
        Assert.AreNotEqual(Color.Red, tetris.GetColorAt(1, 0));

        tetris.Start();
 //and that after running Start() the pixels are set to red
        Assert.AreEqual(Color.Red, tetris.GetColorAt(0, 0));
        Assert.AreEqual(Color.Red, tetris.GetColorAt(1, 0));
    }

So to run this code I had to first create a BoardEngine class. After that, I need an empty BoardEngine constructor that accepts a Board as argument. I then need to create an empty new constructor on Tetris that accepts a BoardEngine as argument.

If I try running the code, I will get a NullPointerException. Why? Because when trying to do

tetris.GetColorAt(0, 0)

since in this constructor I am using now I haven't set the Board in Tetris to anything, it will just blow up.

My first question arises here. What to do now? In one hand, I can make this don't crash by setting that Board to something. On the other hand, what I really want is not to set it to anything, what I'll want is to eventually get rid of it, so my Tetris class only has a BoardEngine attribute.

Am I supposed to go and refactor the tests themselves? I guess it's the only way of making this work. Am I missing something? At which time should I refactor the other tests? If I refactor them BEFORE trying to run this test, I then can mantain all those other tests green while this one is red. On the other hand, if I try to make this one be green asap, all the others are going turn to red :(

Here is an example of an old test:

[TestMethod]
public void ...() {
    Board board = new Board(10, 22);
    Tetris tetris = new Tetris(board);

    tetris.Start();
    Assert.AreEqual(Color.Red, board.GetColorAt(0, 0));
    Assert.AreEqual(Color.Red, board.GetColorAt(1, 0));
}

Thanks

Answer 1

Sounds like you should look at a mocking framework for testing purposes; try Mockito - http://mockito.org/

e.g.

Board board = mock(Board.class);
BoardEngine boardEngine = new BoardEngine(board);
Tetris tetris = new Tetris(boardEngine);
when(board.GetColorAt(0, 0)).thenReturn(Color.RED);
Assert.AreEqual(Color.Red, tetris.GetColorAt(0, 0));

It sounds as though you are trying to maintain tests as passing during a refactor (substituting board for boardEngine). If you are following Test Driven Development then you should update the tests so that all tests are failing; then update your code with the proposed changes.

In practice using a framework such as Mockito allows you to separate your concerns. I.e. you can test the expected behavior of the Tetris class without making the test depend on the behavior of the BoardEngine/Board classes.

Answer 2

From my pov this is not à problemen with the tests.

The method GetColorAt in the tetris class looks like à wrapper on the same method in the board class. The instance of the board-class has however moved from the tetris-class to the boardengine. Either expose the board from the engine as a property so the tetris class has access to it, or create a GetColorAt method on the engine to chain the call from tetris through the engine to the board.

Class Tetris{
    public Color GetColorAt(int x, int y){
        return  _engine.Board.GetColorAt(x,y);
    }
}

Or

Class Tetris{
    Public Color GetColorAt(int x, int y){
        return  _engine.GetColorAt(x,y);
    }
}

Class Boardengine{
    Public Color GetColorAt(int x, int y){
        return  _board.GetColorAt(x,y);
    }
}

Answer 3

In general, if you change the design, you will have to change your tests as well. In this case, because your constructor is evolving, and taking a new concrete type of dependency, you will need to refactor the tests.

Part of the pains here is that you are taking a class as a dependency; if you were taking an IBoard, you could both use mocking more easily, like Syntax is suggesting, and have a test that doesn't depend on a concrete implementation. You could also probably refactor more easily your code, because you could proceed in 2 phases: add the interface contract to the new class you want to use, and then progressively remove it from the old class. You could actually start this approach right now: first extract Board into an interface, then implement the interface on BoardManager and remove it from Board.

That being said, sometimes when you are on solid ground, it's just plain easier and faster to just have a bunch of tests go red together and fix them all at once!

Another remark: your issue might also indicate that you are not unit testing the right class. If GetColor is a function provided by Board or BoardManager, maybe your unit test should be on that class only, and the Tetris class should simply check that it has a BoardManager available to call, and that it is relaying the calls properly to it...

Answer 4

This code:

Board board = new Board(10, 22);
Tetris tetris = new Tetris(board);

will still work fine, if you create a constructor for Tetris that takes a Board, creates a BoardEngine around it, and calls the BoardEngine constructor. Then you can inline that constructor, and Poof! your Board disappears from the Tetris interface - and all your existing tests still work. This counts as refactoring while green, as far as I'm concerned.