Moving Closer
Experience, thinking, and a few words with GeePaw suggest some moves. I try some things, roll back, but think we’re on a decent path.
We have a broken test, which I kept as a memento of the occasion:
def test_step(self):
world = World(20, 20)
bot = Bot(10, 10)
world.add(bot)
connection = DirectConnection(world)
connection.step(bot)
assert bot.location == Location(11, 10)
assert connection.knowledge.location == Location(11, 10)
assert world.map.entity_at(11, 10) == bot
It’s failing on the first assertion. the bot’s location has not changed, because we are now copying the Knowledge object on the call from connection to world, in my first sketch at a network message send. Yesterday, I was thinking that we would change that by changing what we stored in Map, although I do not remember the details of that idea. In any case, it was not a good one.
Yesterday in a few Slack messages, GeePaw reminded me that we should really not be sending a client-side data structure to the World, that it would be better just to send the id of the bot we want to affect, and the command we want done. (Plus, of course, any parameters that that command might allow.) This is unquestionably true and maybe it’s time to do it. Let’s think about it.
On the surface, it would be easy enough to do that in Connection and the time is arguably right:
class DirectConnection:
def __init__(self, world):
self.world = world
def step(self, bot):
self.knowledge = self.copy_knowledge(bot)
self.world.step(self.knowledge)
bot._knowledge = copy.copy(self.knowledge)
def copy_knowledge(self, bot):
return copy.copy(bot._knowledge)
Instead of copying the Knowledge over, we might just do something like the following. I’m going to type it into the actual code: it can’t break much, this is a new class and it’s not used in prod.
def step(self, bot):
self.world.command('step', bot.id)
result_knowledge = self.world.fetch(bot.id)
bot._knowledge = copy.copy(result_knowledge)
We posit that we will send simple text commands, and (I’m guessing) they will have various numbers and types of arguments, so that the command operation will have a little language to sort out, but just now, we only have the one to deal with. The fetch represents the notion that the DC will send one or more commands, perhaps even for more than one bot, and that when the command sequence is done, it’ll request the results be sent back.
This isn’t quite right … I need to work a bit with sockets and such to be confident about details, but it has approximately the right sequence of events.
What I’m assuming here is that World, as soon as we do this, becomes the owner of the Knowledge of all the bots, which it will store in the Map, and that it will send that information back in response to the fetch.
This does not work, of course, because World does not understand command and fetch. Let’s fix that.
class World:
def command(self, action, parameter):
if action == 'step':
bot = self.map.at_id(parameter)
self.step(bot)
else:
raise Exception('Unknown command')
class Map:
def at_id(self, id):
return self.contents[id]
I think this should get us to failing on the call to fetch. And it does.
def fetch(self, entity_id):
return self.map.at_id(entity_id)._knowledge
Now we fail on the penultimate assert of the test:
assert connection.knowledge.location == Location(11, 10)
The connection does not have a knowledge attribute. We had one, I think, but now we don’t use it. We do, however, know that the connection updated our bot correctly. Let’s belay that line and take a look at what we’ve done:
def test_step(self):
world = World(20, 20)
bot = Bot(10, 10)
world.add(bot)
connection = DirectConnection(world)
connection.step(bot)
assert bot.location == Location(11, 10)
assert world.map.entity_at(11, 10) == bot
Note that last line, we’ll come back to that.
class DirectConnection:
def __init__(self, world):
self.world = world
def step(self, bot):
self.world.command('step', bot.id)
result = self.world.fetch(bot.id)
bot._knowledge = copy.copy(result)
class World:
def command(self, action, parameter):
if action == 'step':
bot = self.map.at_id(parameter)
self.step(bot)
else:
raise Exception('Unknown command')
def fetch(self, entity_id):
return self.map.at_id(entity_id)._knowledge
We are green, by the way, Let’s commit and push. Commit: DirectConnection sends command string and bot id.
Reflection
I mentioned that last line of the test, asserting that the world map has the Bot. That won’t do, the bot belongs to the client side. In case it isn’t obvious, we are moving toward a Knowledge instance, or something much like it, belonging to World. The client can use the class for its own purposes, if it wants, but we’re going to have the fundamental knowledge preserved on the server side, never read in from the client side. We just accept commands, update our knowledge, and report on it when asked to.
We’re not there yet.
Hm. Recall that we have identified different chunks of Knowledge as pertaining to different objects:
def __init__(self, location, direction):
# shared info
self._direction = direction
# world write / client read only
self._entity = None
self._id = None
self._location = location
self._scent = 0
self._vision = Vision([], self.location, self.direction)
# local info client
self._energy = self.energy_threshold
self._old_location = None
I think that even that is not quite right. I think that id, direction, and location are clearly things that are owned and maintained by the World. Direction cannot be “shared”, because we’re not sharing client data into the world. We’ll need a command to change direction.
As for scent and vision, however, I’m not so sure. They certainly don’t seem as primitive as the first three.
Be that as it may, we need to change world so that instead of holding a Bot or Block instance, it holds the World’s knowledge about that entity.
I think that for now we can ignore this concern. With the connection, we have begun to break the link.
Here is the fundamental problem:
class World:
def add(self, entity):
entity.world = self
World.next_id += 1
entity.id = World.next_id
self.map.place(entity)
class Map:
def place(self, entity):
self.contents[entity.id] = entity
That assignment gives world direct access to the client object. (And that object might be a Bot, which has a _knowledge, or a Block, which has not.)
We have rafts of tests that very likely rely on this. Our code is pushed. Let’s try something, just as an experiment.
class Map:
def place(self, entity):
if entity.name == 'R':
knowledge = Knowledge(entity.direction, entity.location)
knowledge.id = entity.id
self.contents[entity.id] = knowledge
else:
self.contents[entity.id] = entity
If we’re dealing with a Bot (their name is ‘R’), we create a new knowledge and store that. Otherwise we just store the entity, which will be a Block.
Twelve tests fail. Let’s see how hard they’ll be to fix.
One of the tests wants the name. Let’s patch it in, this is Python. Twelve still fail but we’ll see what’s up.
Knowledge does not have x and y attributes. Add them as properties referencing location. Still twelve fail.
fetch no longer needs to reach in to get knowledge. This is getting slow, we may have to roll this experiment back.
Ah. The new test needs its last line changed, since we no longer have a bot in Map:
def test_step(self):
world = World(20, 20)
bot = Bot(10, 10)
world.add(bot)
connection = DirectConnection(world)
connection.step(bot)
assert bot.location == Location(11, 10)
assert world.map.at_xy(11, 10) is not bot
Now let’s see what’s up with some of the remaining 11 tests. With luck, they all get cured with the same couple of changes.
def test_map_successful_move(self):
map = Map(10, 10)
bot = Bot(5, 5)
map.place(bot)
map.attempt_move(bot.id, Location(6, 6))
assert bot.location == Location(6, 6)
This test is relying on the Map holding a bot. We need to check the map itself.
def test_map_successful_move(self):
map = Map(10, 10)
bot = Bot(5, 5)
map.place(bot)
map.attempt_move(bot.id, Location(6, 6))
assert map.at_id(bot.id)._location == Location(6, 6)
There’s a similar test passing for the wrong reason, and I’ll fix it also.
I spend a little time confused by the fact that I created the knowledge with its two parameters reversed. Type checking would have been nice.
OK, I’ve learned enough from this experiment. Roll back.
It’s those darn tests that we all hate. Well, I don’t hate them, they’ve served well so far, keeping things working as they should, guiding new changes as they should. However … well, just look at this easy one:
def test_step(self):
world = World(10, 10)
bot = Bot(5, 5)
world.add(bot)
location = bot.location
bot.direction = Direction.NORTH
bot.step()
assert bot.location == location + Direction.NORTH
This old test is much like this new one:
def test_step(self):
world = World(20, 20)
bot = Bot(10, 10)
world.add(bot)
connection = DirectConnection(world)
connection.step(bot)
assert bot.location == Location(11, 10)
assert world.map.at_xy(11, 10) == bot
Let me retrofit the old one to the new scheme.
def test_step(self):
world = World(10, 10)
bot = Bot(5, 5)
world.add(bot)
location = bot.location
bot.direction = Direction.NORTH
connection = DirectConnection(world)
connection.step(bot)
assert bot.location == location + Direction.NORTH
It runs. Maybe I should move it over to TestConnection. Yes. Done, renamed to test_step_north.
Reflection
We need to break the direct object access between the Map and the client objects Bot and Block. We want the map to contain its own notion of what is stored there. That notion will include what we now have in Knowledge, but probably additional information, such as the entity name and type. That object should belong to world. Probably trying to use Knowledge isn’t ideal: we can match up what the world has to what the client has in the connection.
We have a lot of tests that ask the world to do something and then check a bot to see whether it happened.
That’s too remote. If we’re trying to test what the world does, we should test that. If we want to test the bot, we should be testing things like its state machine.
If we want to test the map’s behavior, we should test the map.
And, of course, we should be moving toward more command access like what we’ve done for step, rather than telling bots to step or whatever else we tell them.
I think that to make the changes easy, I might do well to improve the tests first. It will be tedious, perhaps, but it shouldn’t be difficult … and if it is, it’ll be telling us something about the design.
Let’s just check some of the tests in TestBot. I’ll bet they need work.
class TestBot:
# Seems to have redundant tests, needs cleanup
def test_create_bot(self):
bot = Bot(10, 10)
assert isinstance(bot, Bot)
def test_starting_location(self):
bot = Bot(10, 10)
assert bot.location == Location(10, 10)
def test_bot_in_world(self):
World.next_id = 100
bot = Bot(10, 10)
world = World(10, 10)
world.add(bot)
point = Location(10, 10)
assert bot.id == 101
assert bot.location == point
That comment doesn’t seem to have fixed the problem with these tests. Maybe if it had been a TODO it would have worked better. Perhaps not.
I can forgive the first two tests as setting up the class and init. What’s that third one about?
Is it about the world assigning an id? Why is it checking the location again? (Because I was thinking that some time in the indefinite future, you might not be able to assign a bot’s location, because the world wouldn’t allow it.) How does that test line tell us that or help us? It doesn’t. Rename and revise:
def test_bot_in_world_gets_next_id(self):
World.next_id = 100
bot = Bot(10, 10)
assert bot.id is None
world = World(10, 10)
world.add(bot)
assert bot.id == 101
This is surely going to change, because we are surely going to do additions over the Connection but it’ll do for now and is closer to correct. When we update adding, we’ll need to update this but it should still get that id.
I’m trying to think of ways to learn what these tests learn but differently, more directly. This one tells us that the id is incremented and that it gets assigned to the bot. Would we do well to test those two notions separately? One that we increment the id when we add things, and one that the id gets to the added object? Probably. I don’t know see how I would want to do that. Another test:
def test_bot_facing_north_block(self):
map = Map(10, 10)
bot = Bot(5, 5)
bot.direction = Direction.NORTH
map.place(bot)
bot.vision = map.vision_at(bot.location)
assert not bot.can_take()
map.place(Block(5,4))
bot.vision = map.vision_at(bot.location)
assert bot.can_take()
Note the following chain:
class Bot:
def can_take(self):
return self._knowledge.can_take
class Knowledge:
@property
def can_take(self):
is_scent_ok = self._scent <= self.take_threshold
return is_scent_ok and self.vision.match_forward_and_one_side('B', '_')
I think this test is a test of Knowledge, and possibly, vision’s interaction with direction.
Let’s see if we can write that more directly.
def test_knowledge_cannot_take_if_no_block(self):
map = Map(10, 10)
location = Location(5, 5)
direction = Direction.NORTH
knowledge = Knowledge(location, direction)
knowledge._scent = map.scent_at(location)
knowledge._vision = Vision(map.vision_at(location), location, direction)
assert not knowledge.can_take
def test_knowledge_can_take_if_block(self):
map = Map(10, 10)
map.place(Block(5,4))
location = Location(5, 5)
direction = Direction.NORTH
knowledge = Knowledge(location, direction)
knowledge._scent = map.scent_at(location)
knowledge._vision = Vision(map.vision_at(location), location, direction)
assert knowledge.can_take
I find that convincing enough to remove these tests:
def test_bot_facing_north_block(self):
map = Map(10, 10)
bot = Bot(5, 5)
bot.direction = Direction.NORTH
map.place(bot)
bot.vision = map.vision_at(bot.location)
assert not bot.can_take()
map.place(Block(5,4))
bot.vision = map.vision_at(bot.location)
assert bot.can_take()
def test_bot_facing_east_block(self):
map = Map(10, 10)
bot = Bot(5, 5)
bot.direction = Direction.EAST
map.place(bot)
bot.vision = map.vision_at(bot.location)
assert not bot.can_take()
map.place(Block(6,5))
bot.vision = map.vision_at(bot.location)
assert bot.can_take()
Commit that: replace bot tests with knowledge tests.
Now those two tests might want to move over to knowledge. Huh. When I look over there I find even better tests that do essentially the same thing. Almost wish I had looked, but the experience was useful. Remove those two new tests.
I think that’s enough for the morning. I might get a session this afternoon with GeePaw. Let’s sum up.
Summary
We began by fixing our broken test, with the result that via the DirectConnection, the World Map no longer has a pointer to the live Bot or Block in the client. In that mode, the Map contains a Knowledge instance, not an Entity.
That’s a step in the right direction and we’ll keep it moving. I think we’ll create our own World-side equivalent of Knowledge (or Entity) that contains what the World wants to know about things. The info will get copied back to the client on the new method fetch, which may or may not stand in that form, but which does imply that after issuing commands, the client requests and receives the results of the commands.
We have more commands to do before that scheme can be considered to be working.
We briefly tried breaking the World’s direct reference to the Bot and Block in our older code, and that breaks a few more tests than I was prepared to deal with just now. I think that when we get the Connection working, it’ll be easier to move over to it.
Meanwhile, we do have a lot of tests that are too indirect, really checking something that happens down at the Knowledge or Map level, but doing the test by throwing things into the World and watching them, instead of exercising the right objects more directly. We improved that situation by the clever process of replacing two tests and then removing the replacements as redundant. You’d think that could have been done more efficiently … but in fact writing those tests as replacements for the bot-referencing ones gave me confidence that they were good enough. If I had just looked at the existing ones that we wound up retaining, I don’t think I’d have been sure we should just remove the original bot tests.
I do think there is an outside chance that during upcoming changes, something might not get copied over to the Bot, and maybe there is no test that will catch it. I’m not too worried about that, because I think at least some of our connection tests will deal with that concern. We’ll see: it’s possible that I’ve torn a small hold in the net of tests.
I am hopeful that I’ll get a session with GeePaw this afternoon: I think his insights will be helpful. He often sees things from a different angle and that’s usually helpful.
Either way, I think we need some test revision and whether we push first on more messages over the Connection, or on the tests, either way will be good. We’ll probably push for more capability, leaving me the more tedious test revision. But we’ll see.
We’re inching toward the goal. I predicted a couple of days of work yesterday. While I think I have a better picture of things today, there hasn’t been much real progress. We’ll see if I was too optimistic.
See you next time!