Dungeon 304
Today I’m going to try to learn something. I wonder if I’ll learn what I set out to learn, or something else.
Yesterday was Tuesday, so as frequent readers may recall, the Friday Night Coding Slack group held their weekly Zoom meeting. I raised the testing topics that I wronte about yesterday. Today I’ll briefly comment on an idea that I got from GeePaw Hill, and then I want to learn better ways of testing my interacting objects.
I raised my theory that interacting objects need more mocking and fakery than hierarchic objects, and as an intro raised an example of a hierarchic situation, because I wanted to understand how GeePaw would test it. I was thinking I had a case where he’d probably have to set up more than one object for testing. But no, he said he’d do it differently. I understood the idea pretty much immediately.
The example was an income tax prep program, where some form, 1040, uses data calculated by another form, Schedule D. I asked how he’d test 1040 alone, since it needs to ask Schedule D for some of its result data. I was envisioning a scheme where 1040 would say “Schedule D, give me your line 45”.
Hill demurred. He said, no, he’d have Schedule D produce an export package of pure numbers, a data package with no calculations in it. In essence, a dictionary of values. I suppose it might be a dictionary from line number to value. Something like that, anyway.
The thing with that is that now, we can test 1040 independently of Schedule D, because we can create actual export packages in our tests, give them to the 1040 and watch it calculate.
Simple, clear, and now 1040 and D have no direct dependency. They do share understanding of the export package: they are necessarily coupled by the shared information, of course.
We might find that idea useful in the dungeon program, though I haven’t any particular idea about that. We’ll keep it in mind.
The thing I want to learn about is better ways of testing my interacting objects. The Decor/Loot test from yesterday is what I want to think about:
Decor / Loot Behavior
When there is a visible object in the dungeon, it is typically Decor (or a Monster). Decor might be a skeleton, or a box, or a chest. Decor can do damage, and may contain Loot.
The player interacts with everything in the dungeon by bumping into it, trying to step onto the same tile as the thing. (That’s because we have very few controls on an iPad.) I’ve been calling that attempt to move onto a thing “bumping”.
When the player enters a room with a Decor, the Decor is the only thing on the tile. Oh! A chest! How interesting!
If you bump the Decor the first time, three things need to happen:
- Change appearance. Most decor doesn’t do this, but chests change from being closed to being open.
- If there is Loot in the Decor, move it onto the same tile as the decor, so that it appears. Oh, look! A huge rock!
- If the Decor is rigged to do damage, a CombatRound is built to apply the damage and put information into the crawl.
Now, you’re still on the tile you were on, because the Decor didn’t let you into the tile, but you now see an open chest and the Loot, a Mysterious Rock of Dullness.
When you bump again, both the chest and the loot will interact with you. The Loot will give itself to you, and disappear. We’re not here to worry about that.
The chest interaction this time should be to ignore you, so you see an open chest and the Loot is going to your inventory.
The third time you bump the chest, it sets itself to open, which is the signal that you can step onto it. We do that so that Decor can never accidentally block the door.
The code for this is simple and clear, because it uses an ActionSequence instance to call each of the three action methods (in sequence). We could have kept a counter and used if statements. The ActionSequence seems better to me. ActionSequence is well-tested, by the way.
Our code is this:
function Decor:initActionSequence()
local actionTable = {
self.firstAction,
self.secondAction,
self.thirdAction
}
self.sequence = ActionSequence(self, actionTable)
end
function Decor:actionWith(aPlayer)
self.sequence:action(aPlayer)
end
function Decor:firstAction(aPlayer)
self:changeAppearance()
self:showLoot()
self:applyDamage(aPlayer)
end
function Decor:secondAction()
-- nothing
end
function Decor:thirdAction()
self.open = true
end
It’s clear by inspection that this works, or so I claim. But we should have a test that locks in any complex behavior, and yesterday we wrote that test:
_:test("Decor shows Loot only once.", function()
local tile = FakeTile()
local item = FakeItem()
local decor = Decor(tile,item)
decor.danger = decor.doNothing
_:expect(tile:getContentsCount(), "Decor not added").is(1)
decor:actionWith()
_:expect(tile:getContentsCount(), "Did not show loot").is(2)
decor:actionWith()
_:expect(tile:getContentsCount(), "Showed loot twice").is(2)
end)
This “simple” test uses two fake objects, FakeTile and FakeItem. It monkey-patches the Decor danger method to ensure that it does nothing, which just lets me be sure that a CombatRound isn’t going to get invoked. We’re not set up to allow that.
Our FakeTile includes a count of the times something is added to it:
function FakeTile:init()
self.contentsCount = 0
end
function FakeTile:addContents(something)
self.contentsCount = self.contentsCount + 1
self.added = something
Bus:publish("moveObjectToTile", something, self)
end
function FakeTile:getContentsCount()
return self.contentsCount
end
So our test just checks to be sure that we add two things, the Decor and then the Loot, and that we do not add the loot again, which assures that we only did showLoot once, as intended.
Now, I think this test is notably more intricate than the thing it’s testing. That’s not good. It took me two passes to write the test yesterday. The first version was even more intricate. I’m not going to jump to write tests if it’s going to be that tricky.
As Jerry Weinberg put it, things are as they are because they got that way, and our code is the way it is because it got that way. Now, much of how it is, I like. I like that we have these little objects that all sort of act on their own, making decisions about where to go, and so on.
But there are things that I don’t like. I don’t like that we treat the tiles as intelligent objects: I’ve come to believe that they should be no more than a token representing a location. We used to store the things that are “on” a tile inside the tile. We no longer do that: we moved the representation of what’s on a tile to DungeonContents, around 100 or so articles back. But we sill have many connections to the tile in the code.
We’re not here to solve the big problem. We’re here to learn about the big problem by addressing this small one.
Ah. I’m getting a part of an idea. Here’s showLoot:
function Decor:showLoot()
self:currentTile():moveObject(self.loot)
end
Here we are, a Decor, finding our tile (possibly ok) and then telling the tile to move our loot onto itself. Here’s what really happens:
function Tile:moveObject(thing)
self:addContents(thing)
end
function Tile:addContents(aDungeonObject)
Bus:publish("moveObjectToTile", aDungeonObject, self)
end
It turns out that DungeonCollection is the subscriber to that message, and it does whatever magic it does. We don’t care just now: the Loot appears where it should.
But suppose that our showLoot was this:
function Decor:showLoot()
Bus:publish("moveObjectToMe", self, self.loot)
end
If that were the case, we could just check the Bus to see how many times that message was sent. That might lead to a better test.
Let’s make it so.
Here’s how the move to tile is done:
function DungeonContentsCollection:moveObjectToTile(object,tile)
--requires DungeonObject to be defined way forward in tabs.
if object then
self.contentMap[object] = tile
self:dirty()
end
end
We’ll need a new subscription here, and of course a method.
function DungeonContentsCollection:init()
self.contentMap = {}
self:dirty()
Bus:subscribe(self, "drawDungeonContents")
Bus:subscribe(self, "getTileContents")
Bus:subscribe(self, "moveObjectToMe") -- <===
Bus:subscribe(self, "moveObjectToTile")
Bus:subscribe(self, "removeObject")
Bus:subscribe(self, "tileContaining")
end
This should error without the method.
EventBus:143: event moveObjectToMe must be implemented
As planned. Implement:
function DungeonContentsCollection:moveObjectToMe(targetObject, object)
local targetTile = self:tileContaining(targetObject)
self:moveObjectToTile(object,targetTile)
end
That seems easy. I think I might want to reverse the order of the parameters, but this should work. Let’s find out.
My loot test fails, not surprised there. I’m going to test in game. Game works a treat. Examine the test:
6: Decor shows Loot only once. Did not show loot -- Actual: 1, Expected: 2
6: Decor shows Loot only once. Showed loot twice -- Actual: 1, Expected: 2
The FakeTile is no longer called at all, as intended. The test itself is:
_:test("Decor shows Loot only once.", function()
local tile = FakeTile()
local item = FakeItem()
local decor = Decor(tile,item)
decor.danger = decor.doNothing
_:expect(tile:getContentsCount(), "Decor not added").is(1)
decor:actionWith()
_:expect(tile:getContentsCount(), "Did not show loot").is(2)
decor:actionWith()
_:expect(tile:getContentsCount(), "Showed loot twice").is(2)
end)
What we need to know is whether the Bus was asked for “moveObjectToMe”, and how many times it happened.
I note that we do have a 1 in the FakeTile, because of course we did this in Decor:init:
function Decor:init(tile, loot, kind)
self.kind = kind or Decor:randomKind()
self.sprite = DecorSprites[self.kind]
if not self.sprite then
self.kind = "Skeleton2"
self.sprite = DecorSprites[self.kind]
end
self.loot = loot
tile:moveObject(self) -- <===
self.scaleX = ScaleX[math.random(1,2)]
local dt = {self.doNothing, self.doNothing, self.castLethargy, self.castWeakness}
self.danger = dt[math.random(1,#dt)]
self:initActionSequence()
self.open = false
end
We can legit do this:
function Decor:init(tile, loot, kind)
self.kind = kind or Decor:randomKind()
self.sprite = DecorSprites[self.kind]
if not self.sprite then
self.kind = "Skeleton2"
self.sprite = DecorSprites[self.kind]
end
self.loot = loot
Bus:publish("moveObjectToTile", self, tile)
self.scaleX = ScaleX[math.random(1,2)]
local dt = {self.doNothing, self.doNothing, self.castLethargy, self.castWeakness}
self.danger = dt[math.random(1,#dt)]
self:initActionSequence()
self.open = false
end
Now I think the game works but the test says 0s rather than 1s.
6: Decor shows Loot only once. Decor not added -- Actual: 0, Expected: 1
Etc. As expected. Now I do need a new fake object, but I think it’s going to be generally useful. Should I TDD it? I think yes.
What I think I’ll do is build a specialized subclass of EventBus, TestEventBus, that works just like EventBus, but records useful information. What is useful information? Well, I think it certainly includes a list of the things published and how many times each was published.
Let’s go over to EventBus and work on it. This is a bit iffy: I’ve got this other test broken. Let’s ignore it. The yellow line will remind me to fix it.
RecordingEventBus
I decide to call it RecordingEventBus rather than Test....
function testRecordingEventBus()
CodeaUnit.detailed = false
_:describe("RecordingEventBus publish/subscribe", function()
_:before(function()
OldBus = Bus
Bus = RecordingEventBus()
end)
_:after(function()
Bus = OldBus
end)
_:test("hookup", function()
_:expect(RecordingEventBus).isnt(nil)
end)
end)
end
This should fail demanding the class.
EventBus:80: attempt to call a nil value (global 'RecordingEventBus')
No big surprise. Code:
RecordingEventBus = class(EventBus)
Test better pass. Yes. Make a real test:
_:test("REB counts publishes", function()
local msg = "someMethod"
_:expect(Bus:count(msg)).is(0)
end)
This’ll fail for want of count.
2: REB counts publishes -- EventBus:93: attempt to call a nil value (method 'count')
And …
function RecordingEventBus:init()
self.counts = {}
end
function RecordingEventBus:publish(event, ...)
self.counts[event] = (self.counts[event] or 0) + 1
end
function RecordingEventBus:count(event)
return self.counts[event] or 0
end
I expect my test to pass. It does. Refactor:
function RecordingEventBus:publish(event, ...)
self.counts[event] = self:count(event) + 1
end
I expect it to continue to run. It does. Extend the test:
_:test("REB counts publishes", function()
local msg = "someMethod"
_:expect(Bus:count(msg)).is(0)
Bus:publish(msg)
_:expect(Bus:count(msg)).is(1)
end)
I expect this to run as well. It does. But the REB isn’t actually publishing. We’d better test that.
Let’s adopt a test from EventBus:
_:test("query returns result, publish does not", function()
local answer
local lis1 = TimesListener("provideAnswer", 15)
local mul = 2
answer = Bus:publish("provideAnswer", mul)
_:expect(answer, "publish returned something").is(nil)
answer = Bus:query("provideAnswer", mul)
_:expect(answer).is(30)
end)
This should fail nicely.
3: query returns result, publish does not -- EventBus:186: attempt to index a nil value (field 'events')
We aren’t initializing our superclass. I forgot that, though I am expecting the super call issue to arise in query and publish.
function RecordingEventBus:init()
Bus.init(self)
self.counts = {}
end
The test should fail differently now.
Weirdly, it passes. I am surprised. Let’s enhance the test to check some counts.
_:test("query returns result, publish does not", function()
local answer
local lis1 = TimesListener("provideAnswer", 15)
local mul = 2
answer = Bus:publish("provideAnswer", mul)
_:expect(answer, "publish returned something").is(nil)
_:expect(Bus:count("provideAnswer")).is(1)
answer = Bus:query("provideAnswer", mul)
_:expect(answer).is(30)
_:expect(Bus:count("provideAnswer")).is(2)
end)
Test:
3: query returns result, publish does not -- Actual: 1, Expected: 2
We need to implement query, and we also need to call super in the publish, tho my test doesn’t require that. First things first:
function RecordingEventBus:publish(event, ...)
self:tick(event)
end
function RecordingEventBus:query(event, ...)
self:tick(event)
Bus.query(self, event, ...)
end
function RecordingEventBus:tick(event)
self.counts[event] = self:count(event) + 1
end
The query method now intercepts, so that the count will be right, and it also calls the superclass. But I forgot to add in the return, so the test will probably fail.
Hm, this was not quite my plan:
3: query returns result, publish does not -- EventBus:224: stack overflow
Ah. I’m calling Bus. Should be EventBus:
function RecordingEventBus:init()
EventBus.init(self)
self.counts = {}
end
function RecordingEventBus:publish(event, ...)
self:tick(event)
end
function RecordingEventBus:query(event, ...)
self:tick(event)
return EventBus.query(self, event, ...)
end
This should be better. In fact the test runs. But it’s still not testing publish doing actual publishes.
I’ll adopt another test from the EventBus suite:
_:test("can subscribe to a message", function()
local listener = FakeListener("itHappened")
Bus:publish("itHappened", info)
_:expect(itHappened).has(listener)
end)
That should fail in the REB tests. The failure is odd:
3: can subscribe to a message -- Actual: table: 0x2825b2a80, Expected: table: 0x2825b2ac0
I’m going to go ahead and cause the publish to happen and see if the test runs.
function RecordingEventBus:publish(event, ...)
self:tick(event)
return EventBus.publish(self, event, ...)
end
I expect success. I receive success. Commit: RecordingEventBus implemented. (I just commit the EventBus class. The other code and ignored test are pending.
Back to the ignored test.
_:ignore("Decor shows Loot only once.", function()
local tile = FakeTile()
local item = FakeItem()
local decor = Decor(tile,item)
decor.danger = decor.doNothing
_:expect(tile:getContentsCount(), "Decor not added").is(1)
decor:actionWith()
_:expect(tile:getContentsCount(), "Did not show loot").is(2)
decor:actionWith()
_:expect(tile:getContentsCount(), "Showed loot twice").is(2)
end)
My plan here is to use the new RecordingEvenBus to track that the calls occur.
function testDecor()
CodeaUnit.detailed = false
_:before(function()
bus = Bus
Bus = RecordingEventBus()
end)
No harm in using it throughout.
_:ignore("Decor shows Loot only once.", function()
local tile = FakeTile()
local item = FakeItem()
local decor = Decor(tile,item)
decor.danger = decor.doNothing
_:expect(Bus:count("moveObjectToTile"), "Decor not added").is(1)
decor:actionWith()
_:expect(Bus:count("moveObjectToMe"), "Did not show loot").is(1)
decor:actionWith()
_:expect(Bus:count("moveObjectToMe"), "Showed loot twice").is(1)
end)
Test runs. Commit: Replace two Decor tile refs with direct use of Bus. Modify test to use RecordingEventBus.
No! Wait! I didn’t change the ignore back to test Test fails:
6: Decor shows Loot only once. -- Decor:333: attempt to call a nil value (method 'count')
Huh? Ah. I had the before and after in the wrong place. Tests run. Recommit: Decor had mistakenly ignored test.
Let’s reflect and sum up.
Reflection
It seems to me that if we have to find out whether an object has sent a message the right number of times, we’‘re stuck with some kind of Mock / Fake / Test Double. Given that, it seems to me that using the Bus to send the messages, and the REB to count them, is pretty clean.
I freely grant that this is a pretty London School move, but while I am a Detroit School kind of guy, I’ll take a good idea from anywhere. And, if you have a better idea for this situation, please do let me know.
We still have the monkey patch to bug us, and we’re using the FakeItem and FakeTile. We might be able to do without the fakes. Let’s try it.
_:test("Decor shows Loot only once.", function()
local tile = "tile"
local item = "item"
local decor = Decor(tile,item)
decor.danger = decor.doNothing
_:expect(Bus:count("moveObjectToTile"), "Decor not added").is(1)
decor:actionWith()
_:expect(Bus:count("moveObjectToMe"), "Did not show loot").is(1)
decor:actionWith()
_:expect(Bus:count("moveObjectToMe"), "Showed loot twice").is(1)
end)
OK, good. The Decor pays essentially no attention to its tile or item. (Of course, if we had subscribers to the messages we publish, they might care. But we haven’t.)
I’ll go thru and remove those fakes elsewhere. Some of them probably are needed. We’ll see. Yes. Replaced them all. Commit: Remove FakeItem class and all refs to FakeItem and FakeTile in Decor.
So this seems good. Let’s sum up.
Summary
I’m not sure whether we can claim that RecordingEventBus is a true Mock Object or not, since it’s actually functional. Be that as it may, it allows us to determine which messages an object publishes, and how many times it publishes them, while carrying out the actual function of the EventBus. So that’s a good thing.
The tests for Decor are now simplified by needing no other Fakes. That’s not entirely due to what we did this morning (and afternoon, it turns out), but it does come from our being in there improving things.
Curiously enough, I think I’ve actually accomplished what I set out to do, to learn better ways of testing my interacting objects. It does involve a supporting something like a Fake or Mock, but that’s fine by me. The code is improved. I’m not here to say Detroit Yes, London No, I’m here to find what works best in a given situation.
I continue to suspect that Detroit style works best with objects that return results, and that London may work better with objects that interact via more complex message passing, but I’m not prepared to die on that hill yet. Or any other, for that matter.
My little dungeon world is a slightly better place. I hope we can do the same in the larger world. Please try.
And do stop by next time!