The Repo on GitHub

I said I’d spare you the tedium of improving the tests, but you deserve to see what kind of improvements I’m making. I do not know what sins make you deserving of that, but you can examine your own consciences.

Here’s a test we might still object to, even though it is better than it might be:

    def test_take_a_block(self):
        world = World(10, 10)
        connection = DirectConnection(world)
        client_bot = connection.add_bot(5, 5)
        world.add_block(6, 5)
        assert not world.is_empty(Location(6, 5))
        connection.take(client_bot)
        assert client_bot.has_block()
        assert world.is_empty(Location(6, 5))

I suppose it’s a question of what ne wants to know, but the most basic question we might be asking with this test is whether the world will pick up a block and give it to a bot if they are adjacent and the bot is looking at the block.

We might not be asking whether the connection between the client bot and the world bot works such that the block’s presence is detected all the way back at the client bot. Here’s a test that is less round trip and involves only world-side objects:

    def test_better_take_a_block(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        bot = world.from_id(bot_id)
        world.add_block(6, 5)
        assert not world.is_empty(Location(6, 5))
        world.take_forward(bot)
        assert world.is_empty(Location(6, 5))
        assert bot.has_block()

Perhaps that would be more clear like this:

    def test_better_take_a_block(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        world.add_block(6, 5)
        assert not world.is_empty(Location(6, 5))
        bot = world.from_id(bot_id)
        world.take_forward(bot)
        assert bot.has_block()
        assert world.is_empty(Location(6, 5))

Now the conversion from id to world entity is closer to where it is used. I created World.from_id to make that work:

class World:
    def from_id(self, bot_id):
        return self.map.at_id(bot_id)

Is from_id a good enough name? It might be better if it were named entity or entity_from_id. The latter, I thinkL

    def test_better_take_a_block(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        world.add_block(6, 5)
        assert not world.is_empty(Location(6, 5))
        bot = world.entity_from_id(bot_id)
        world.take_forward(bot)
        assert bot.has_block()
        assert world.is_empty(Location(6, 5))

I am tempted to do something a bit tricky. It would be nice, from the testing viewpoint, to be able to say something like this:

    def test_better_take_a_block(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        world.add_block(6, 5)
        assert not world.is_empty(Location(6, 5))
        world.take_forward(bot_id)
        assert bot_id.has_block()
        assert world.is_empty(Location(6, 5))

Or, optionally, to have world_add_bot return a world_entity. We only need that in testing but there are a lot of tests.

I think we’ll hold off and see what we actually need, if anything. I’ll remove that old DC test and stick with the simpler one. It’s more “micro”.

It’s also more of a world test than a bot test, but it is in the test_bot.py file. We have some world-related test files but no test_world per se. Let’s move this test to a new test_world.py file. PyCharm makes that easy.

Commit: improving tests to be more worldly and less like round-trip story tests.

Now I find four tests that are all basically alike except for direction:

    def test_bot_facing_north_takes_a_north_block(self):
        world = World(10, 10)
        bot = DirectConnection(world).add_bot(5, 5)
        world.add_block(5, 4)
        bot.direction = Direction.NORTH
        world.take_forward(bot)
        assert bot.has_block()

    def test_bot_facing_east_takes_an_east_block(self):
        ...

    def test_bot_facing_south_takes_a_south_block(self):
        ...

    def test_bot_facing_west_takes_a_west_block(self):
        ...

I suppose there is some merit to checking that we don’t always just take to the EAST. I’ll recast one and then let’s have a look. First move them to our new file.

I just noticed that PyCharm moved those tests to the top level if the file test_world.py. They work fine that way, of course. Our current “standard” is to put the tests into separate classes, but we generally make no use of anything about the class itself. Let’s let this ride and see what we think.

The tests now all look like this example:

def test_bot_facing_north_takes_a_north_block():
    world = World(10, 10)
    bot_id = world.add_bot(5, 5, Direction.NORTH)
    bot = world.entity_from_id(bot_id)
    world.add_block(5, 4)
    world.take_forward(bot)
    assert bot.has_block()

I think we could remove some duplication from these, but I generally prefer to see tests written out more longhand anyway, so we’ll let that go. There is surely some pytest way to make them into a batch with variable inputs but … well, we could do this:

First extract two variables and move them to the top, in one test. Well, more than two:

def test_bot_facing_north_takes_a_north_block():
    bot_y = 5
    bot_x = 5
    block_x = 5
    block_y = 4
    world = World(10, 10)
    bot_id = world.add_bot(bot_x, bot_y, Direction.NORTH)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

Then, let’s make the two pairs into two sequences.

def test_bot_facing_north_takes_a_north_block():
    bot_loc = (5, 5)
    block_loc = (5, 4)
    bot_x, bot_y = bot_loc
    block_x, block_y = block_loc
    world = World(10, 10)
    bot_id = world.add_bot(bot_x, bot_y, Direction.NORTH)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

I’m not entirely loving this, kind of wish I had committed earlier. The next step is to factor out the Direction:

def test_bot_facing_north_takes_a_north_block():
    bot_loc = (5, 5)
    block_loc = (5, 4)
    direction = Direction.NORTH
    bot_x, bot_y = bot_loc
    block_x, block_y = block_loc
    world = World(10, 10)
    bot_id = world.add_bot(bot_x, bot_y, direction)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

Then we can extract a method accepting all those nice parameters:

def test_bot_facing_north_takes_a_north_block():
    bot_loc = (5, 5)
    block_loc = (5, 4)
    direction = Direction.NORTH
    check_directional_take(bot_loc, block_loc, direction)


def check_directional_take(bot_loc, block_loc, direction):
    bot_x, bot_y = bot_loc
    block_x, block_y = block_loc
    world = World(10, 10)
    bot_id = world.add_bot(bot_x, bot_y, direction)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

And then inline the parameters:

def test_bot_facing_north_takes_a_north_block():
    check_directional_take((5, 5), (5, 4), Direction.NORTH)

And then change the other three tests:

def test_bot_facing_north_takes_a_north_block():
    check_directional_take((5, 5), (5, 4), Direction.NORTH)

def test_bot_facing_east_takes_an_east_block():
    check_directional_take((5, 5), (6,5), Direction.EAST)

def test_bot_facing_south_takes_a_south_block():
    check_directional_take((5, 5), (5, 6), Direction.SOUTH)

def test_bot_facing_west_takes_a_west_block():
    check_directional_take((5, 5), (4, 5), Direction.WEST)

def check_directional_take(bot_loc, block_loc, direction):
    bot_x, bot_y = bot_loc
    block_x, block_y = block_loc
    world = World(10, 10)
    bot_id = world.add_bot(bot_x, bot_y, direction)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

Is that better? Or is it worse? It’s more compact. I’ll leave it as it is, but I don’t think I’d do that again. Commit: moving some tests to test_world.

That’s enough FAFO for an afternoon session, I have more interesting things to do. Let’s sum up.

Improving those tests to be focused entirely on whether the world does the right thing was good. We already have tests that ensure that commands get processed and that fetch works to transfer information back to the client.

I think that making those tests more compact is at best partially better, partially worse. The good news is that once we understand the check_directional_take method, we understand all four tests, and they are then pretty straightforward. The less good news is that the check is a bi harder to understand, because of the unpacking.

I think it was worth learning how we might do such a thing, and that the test file, in this case, was not improved much if at all. On the other hand, if we encounter another occasion where we need to do several tests that are very similar … and there is one coming up that we might work on, this technique might come in handy.

We’ll see about that next time, but here are the tests I have in mind:

    def test_bot_cannot_drop_off_world_north(self):
        world = World(10, 10)
        bot: Bot = DirectConnection(world).add_bot(5, 0)
        block = Block(4, 4)
        bot.receive(block)
        bot.direction = Direction.NORTH
        world.drop_forward(bot, block)
        assert bot.has(block), 'drop should not happen'

    def test_bot_cannot_drop_off_world_east(self):
        ...

    def test_bot_cannot_drop_off_world_south(self):
        ...

    def test_bot_cannot_drop_off_world_west(self):
        ...

You can see how those might be parameterized to some good end. Maybe next time. See you then!

Added in Post

Hold on a second here. We always pass (5, 5) as the bot location in those tests. And we have no reason to change it: we’re confident that if the takes works there, it’ll work anywhere. So we can remove that parameter from the test, resulting in this:

def test_bot_facing_north_takes_a_north_block():
    check_directional_take((5, 4), Direction.NORTH)

def test_bot_facing_east_takes_an_east_block():
    check_directional_take((6, 5), Direction.EAST)

def test_bot_facing_south_takes_a_south_block():
    check_directional_take((5, 6), Direction.SOUTH)

def test_bot_facing_west_takes_a_west_block():
    check_directional_take((4, 5), Direction.WEST)

def check_directional_take(block_loc, direction):
    block_x, block_y = block_loc
    world = World(10, 10)
    bot_id = world.add_bot(5, 5, direction)
    bot = world.entity_from_id(bot_id)
    world.add_block(block_x, block_y)
    world.take_forward(bot)
    assert bot.has_block()

Simpler. Better. Or so it seems to me. Commit: simplify tests since all use same starting location.

See you next time!