The Repo on GitHub

I promised to improve the state machine tests. I guess I’d better do that. There’s probably some good news in there. I learn to listen to myself.

The potential good news, besides being sure that the thing works, is that I plan to change it in two significant ways.

  • Return a list of commands to perform on the world, rather than sending messages to the bot to forward to the world;
  • Change the machine itself to a one class per state version.

I think there will be pretty good value to having decent tests, because both those changes will need to be well tested. And currently, I’ve found myself needing to watch the game to be sure things are working … and worse, at least twice, maybe three times, I found defects while watching the game. It’s one thing to watch it for fun or reassurance, but when you have to find defects that way … it’s a sign that your tests aren’t as helpful as they might be.

Here are the tests we have now:

class TestMethodObjectStateMachine:
    def test_starts_walking(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        assert machine._state == machine.walking

    def test_laden_goes_to_walking_if_no_block_in_inventory(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        machine._state = machine.laden
        machine.state()
        assert machine._state == machine.walking

    def test_looking_goes_to_laden_if_block_in_inventory(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        machine._state = machine.looking
        machine.state()
        assert machine._state == machine.looking
        bot.receive(Block(2, 2))
        machine.state()
        assert machine._state == machine.laden

    def test_laden_stays_laden_if_cannot_drop(self):
        # we call state() twice to ensure round trip update
        bot = Bot(5, 5)
        machine = Machine(bot)
        entity = Block(3, 3)
        bot.receive(entity)
        vision_list = [('R', 5, 5), ('B', 6, 5)]
        bot.vision = vision_list
        bot.direction = Direction.EAST
        machine._state = machine.laden
        machine.state()
        assert bot.has_block()
        assert machine._state == machine.laden
        machine.state()
        assert bot.has_block()
        assert machine._state == machine.laden

We should at least test some other transitions. The laden Bot should go to walking state if it succeeds in dropping the Block.

    def test_laden_goes_walkabout_after_drop(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        entity = Block(3, 3)
        bot.receive(entity)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        bot.direction = Direction.EAST
        machine._state = machine.laden
        machine.state()
        assert bot.has_block()
        assert machine._state == machine.laden
        bot.remove(entity)
        machine.state()
        assert not bot.has_block()
        assert machine._state == machine.walking

That passes. I am gratified but not surprised. However, these tests seem pretty gross, requiring a lot of setup.

Let’s look at the machine entry for laden and see whether we can see a simpler test.

class Machine:
    def laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._state = self.walking
            return
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

OK, the first thing I see is that check for tired. What we are trying to do with that value is to have the bot wander a bit before it thinks to take or drop something, so that it doesn’t just pick up something and drop it right back: that value makes it more likely that a block will be added to a different clump from where it was found.

But we currently have this:

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._state = self.laden
            return
        if self.bot.can_take():
            self.bot.take()

I think the machine could be better. I see at least two options:

  • Enter walking state from lookup, not laden, and condition the next state that walking chooses based on inventory;
  • Have two states, walking_empty and walking_laden with fixed transitions when the tired value counts down.

Let’s try the first.

    def walking(self):
        if self.tired <= 0:
            self._state = self.laden if self.bot.inventory else self.looking

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._state = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

That breaks a test, of course, the one about going to laden after getting a block. Let’s change the test, we made this change on purpose.

    def test_looking_goes_to_walking_then_laden_if_block_in_inventory(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        machine._state = machine.looking
        machine.state()
        assert machine._state == machine.looking
        bot.receive(Block(2, 2))
        machine.state()
        assert machine._state == machine.walking
        machine.tired = 0
        machine.state()
        assert machine._state == machine.laden

That passes. Nice. Commit: new tests and slight machine change to use walking after pick up and drop off.

We don’t have a test that shows us getting from walking to looking.

I’ll show you the test in a moment but it failed and I discovered this:

    def state(self):
        self.tired =- 1
        self._state()
        return self

We meant -= of course. I kind of wish PyCharm or python had flagged that.

    def test_unladen_goes_from_walking_to_looking(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        assert machine.tired == 10
        assert machine._state == machine.walking
        machine.state()
        assert machine.tired == 9
        assert machine._state == machine.walking
        machine.tired = 0
        machine.state()
        assert machine._state == machine.looking

That runs green now. I don’t entirely love these tests. Commit: new test. Found defect in decrementing tired.

Now I want to watch the game. It looks much the same, perhaps a bit less action, which it could be, since the tired flag was allowing pickup and drop off sooner.

Time to reflect.

Reflection

I’d like these tests to be simpler. It’s possible that we could provide a fake Bot and control its answers to things like can_drop(), which might make things easier.

However. I think these tests are going to need to change substantially, because I plan to refactor the Machine into several separate objects, one class per machine state, and because we’re going to want to return lists of commands to the bot, rather than do the things directly.

I just walked down the hall, as we say euphemistically, and along the way I had a thought: We don’t really want to pass the whole bot to the machine, just because we don’t like loops in our object pointers, but we do want the state machine to make all the decisions for our bots. (We could do otherwise: we just want that. I just want that. It seems right to me.)

So … what we need in the machine is the inventory, and the vision, and then we the can_take and can_drop are local to the machine and the take and drop are done by the bot, ultimately from a list of things to do that we return to it.

Let’s use the vision in the machine. Our current tests are probably good enough.

Ow. Looking at this, it’s not as easy as we might wish. There’s a lot of code involved in can_take and can_drop:

class Bot:
    def can_take(self):
        return self.forward_name() == 'B' and (self.forward_left_name() == '_' or self.forward_right_name() == '_')

    def forward_name(self):
        forward = self.location.forward(self.direction)
        return self.vision.name_at(forward)

    def forward_left_name(self):
        forward_left = self.location.forward_left(self.direction)
        return self.vision.name_at(forward_left)

    def forward_right_name(self):
        forward_right = self.location.forward_right(self.direction)
        return self.vision.name_at(forward_right)

    def can_drop(self):
        return self.forward_name() == '_' and (self.forward_left_name() == 'B' or self.forward_right_name() == 'B')

That said, those methods don’t look like they necessarily belong on Bot, do they? If Vision knew a bit more, maybe it could be a bit more helpful.

class Vision:
    def __init__(self, vision_list):
        self.vision_list = vision_list

    def __iter__(self):
        return iter(self.vision_list)

    def name_at(self, location: Location):
        return self._find_name_at(location.x, location.y)

    def _find_name_at(self, x, y):
        for name, vx, vy in self.vision_list:
            if vx == x and vy == y:
                return name
        return '_'

Let’s do some refactoring, trusting our tests to save us if we do anything bad. Let’s create Vision with our location and direction baked in. and provide those name methods there:

class Vision:
    def __init__(self, vision_list, location, direction):
        self.location = location
        self.direction = direction
        self.vision_list = vision_list

    def __iter__(self):
        return iter(self.vision_list)

    def forward_name(self):
        forward = self.location.forward(self.direction)
        return self.name_at(forward)

    def forward_left_name(self):
        forward_left = self.location.forward_left(self.direction)
        return self.name_at(forward_left)

    def forward_right_name(self):
        forward_right = self.location.forward_right(self.direction)
        return self.name_at(forward_right)

    def name_at(self, location: Location):
        return self._find_name_at(location.x, location.y)

    def _find_name_at(self, x, y):
        for name, vx, vy in self.vision_list:
            if vx == x and vy == y:
                return name
        return '_'

I’ve converted the bot to use the vision this way, and removed the forward_etc methods:

class Bot:
    def can_take(self):
        return self.vision.forward_name() == 'B' and (self.vision.forward_left_name() == '_' or self.vision.forward_right_name() == '_')

    def can_drop(self):
        return self.vision.forward_name() == '_' and (self.vision.forward_left_name() == 'B' or self.vision.forward_right_name() == 'B')

We are green and solid. I think we can commit: move pattern recognition methods forward_etc to Vision.

I do not love three messages to Vision here. We can have a single method that does all three. Like this:

class Bot:
    def can_take(self):
        return self.vision.match_forward_and_one_side('B', '_')

    def can_drop(self):
        return self.vision.match_forward_and_one_side('_', 'B')

class Vision:
    def match_forward_and_one_side(self, center, side):
        return self.forward_name() == center and (self.forward_left_name() == side or self.forward_right_name() == side)

I think I rather like that. All seems good, tests are green, game still looks right. Commit: move more capability to Vision with good effect in Bot.

I think that’ll do for this morning. Let’s sum up.

Summary

We improved our state machine testing substantially, with most of the important transitions checked. We modified the machine a bit, so that it returns to walking state after either picking up or dropping off a block. Along the way we found a very subtle defect, where we had self.tired =- 1 instead of self.tired -= 1. A test actually found that.

We set the goal of passing less than a full bot to the state machine, and as a step toward that, pulled the vision out of the bot as a separate member of Machine:

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._state = self.walking

The plan had been to use the vision and implement can_take and can_drop in Machine, but that looked like too big a bite, so first we refactored Vision to know the location and direction where the picture was taken, and then to implement all the necessary code to give us this fine method that covers the situation for both can_take and can_drop:

class Vision:
    def match_forward_and_one_side(self, center, side):
        return self.forward_name() == center and (self.forward_left_name() == side or self.forward_right_name() == side)

With that in place, we could in fact change the machine to use that method to implement the two can methods, but I am tried and even though it is trivial … oh heck, it’s easy enough, let’s go for it.

One test fails. It’s a bit tricky to fix. I was right when I said not to try it. Roll back, save for another day.

I should listen to myself when I say I’m tired.

At least part of the issue is that we’re going to need to pass the current vision and other such things to the machine on every call to state, since the bot’s status can and will change. So we need a bit more design before we can switch to the narrower usage. That said, a quick fix is probably to go back to referencing bot.vision in the machine.

But that is for another session. I have proven that I am tired and that makes it time to stop.

We have good progress, and I think behavior is moving toward objects that are lower down in the chain, and that’s a good thing.

See you next time!