Dungeon 323
Apparently not Done-geon. Those missing tests for that trivial code bug me. There’s something to learn in here. LATER: A very nice outcome!
One is reminded of the little optimist child in the room full of horse manure, digging with great abandon: “There’s got to be a pony in here somewhere!”
The little story that I worked on yesterday was to cause the program to draw a line between an object in the dungeon, and the crawling text that is displayed when we query in its neighborhood. It took me two unreported tries, and probably two reported ones, to get it to work. I tried, I swear I tried to write tests for each case, to at least be sure they were all doing what I intended, but by the last few, I was so tired and frustrated by writing the tests that I just did the changes, tested in the game, and went home. Well, I was already home.
I was so frustrated that I forgot to finish yesterday’s hand-crafted iced chai latte. If you’d like to make them at home, I recommend Chaikhana Chai. Nice people, super product. They package it so well that even though the delivery people had clearly played soccer with the box, the chai inside was perfectly safe. But I digress.
The Issue(s)
I have at least two objects that are not running tests to ensure that they are providing the location information that’s needed to draw the little dialog line. They both work, but because I am a strong proponent of TDD and microtesting, I feel badly that they are untested..
More importantly, when things are hard to test, that tells us that there is probably something needing improvement in the things themselves. If you have to drop the engine out of your car to add some oil, ease of maintenance needs improvement. Same with tests: they tell us the code needs improvement.
Finally, as part of adding the dialog line to everything, I intentionally changed how it worked, by having the object return a constant “graphic center”. That works well when the objects don’t move, but monsters do move, and so their dialog line is emitted from where they were, not where they are. So I’d like to improve that.
The design change that I made was to provide a location in the call to schedule the crawl lines, where the first implementation had passed in the object. The old way seemed overly complicated to me, and in particular, it was difficult for InventoryItems to provide the information, because they don’t really have a dungeon location.
Anyway, as part N+1 of all this, I’d like to make the dialog line move if the object making the statement moves.
Let’s talk a bit about why this is hard to test, and whether, to a degree, it has to be.
Why This is Difficult
The basic problem, as I see it, is that the crawl, the lines of text that float up over the Princess’s head, are asynchronous from the game objects. Some object emits a message, like its query message, and at some time later, that message appears in the crawl. It’s like one of those Chyron™ things on your tv screen, except that mine appears a line at a time … but notably later than when the computer decided to say that.
As it’s presently implemented, the method query can be called on any dungeon object, and it is supposed to publish one or more lines of description to the crawl. Those lines are wrapped in a strange object, the OP, which amounts to an operation on the crawl or on anything in the game, because you can embed an OP that, when it would display if it were a text OP, instead performs any action you wish. This is typically to apply damage to a Monster or the Princess. We don’t need to worry about that but we do need to know that the message information is in an OP.
The creation of the OP is done via a special Bus message, informDotted (the message is prefixed by a line containing an ellipsis), which currently takes either one or two parameters, a message, and a location.
Via some rigmarole, the informDotted emits the ellipsis, and then the provided message, using another Bus message addTextToCrawl.
The addTextToCrawl is fielded by a Provider, the object that feeds the crawl. The Provider receives an information packet from the caller, and creates an OP, putting it into the crawl’s buffer.
Then, finally, when the OP turns up in the crawl’s work, the Floater, the object that actually displays the messages, draws the message on the screen, with this code:
function Floater:drawMessage(op, pos)
local txt = op:message()
local origin = op:speechOrigin(pos)
strokeWidth(1)
if origin.x ~= 0 then
line(origin.x, origin.y, pos.x,pos.y-8)
end
text(txt, pos.x, pos.y)
end
We ask the OP for the message and the speechOrigin, draw the line if we get an origin other than 0,x back, and then we draw the text.
“Nothing to it.”
This may seem like a lot, but it’s pretty close to all necessary. I can’t argue that a simpler implementation is impossible, but everything in there was the best idea I had at the time I did it. And since it’s so asynchronous from the rest of the program, and since it’s working just fine, I’m not motivated to change it much.
But how do we test this location stuff?
Yesterday, what I settled for was to write tests that check to be sure that when a dungeon object’s query is called, we provide a location to the informDotted method.
Here is one such test, for Decor:
_:test("Decor sets graphicCenter into informDotted", function()
Bus = FakeEventBus()
Bus:expect(nil,vec2(0,0))
local tile = FakeTile()
local item = "item"
local decor = Decor(tile, item, "Skeleton1")
decor:query() -- expectations are on the Bus
end)
The FakeEventBus does the checking:
function FakeEventBus:expect(msg, optionalPos)
self.checkMessage = msg
self.checkPos = optionalPos
end
function FakeEventBus:inform(message, optionalPos)
if self.checkPos then
_:expect(optionalPos).is(self.checkPos)
end
if self.checkMessage then
_:expect(message).is(self.checkMessage)
end
end
The informDotted unwinds to inform, so this receiver of the inform message checks the input values for matching the info provided. In the test above, we just check to see that we get vec2(0,0) which is enough to tell me that we are providing a position.
But wow, it’s a long trip around the horn, isn’t it?
So far, Decor, Lever, and NPC use this approach to testing. I tried some other approaches.
In Key, for example, I did this:
_:describe("Key sends coords", function()
_:before(function()
_bus = Bus
Bus = KeyFakeBus
end)
_:after(function()
Bus = _bus
end)
_:test("Key Test", function()
local key = Key()
_:expect(key:is_a(Key)).is(true)
key:query()
_:expect(_msg).is("Could I possibly be ... a key???")
_:expect(_gc).is(vec2(0,0))
end)
end)
That’s supported by the specialized KeyFakeBus:
KeyFakeBus = class()
function KeyFakeBus:init()
end
function KeyFakeBus:query(message)
print(message)
local t = {}
t.graphicCenter = function()
return vec2(0,0)
end
return t
end
function KeyFakeBus:informDotted(msg, gc)
_msg = msg
_gc = gc
end
Here, the bus just fields informDotted and saves the parameters, which the test can check. The query method was needed because the objects use the Bus to find out where they are: they literally do not know.
It seems to me that I could test Spies this exact same way. Let me try.
First, I rename the key variable to thing, in preparation:
_:test("Key Test", function()
local thing = Key()
_:expect(thing:is_a(Key)).is(true)
thing:query()
_:expect(_msg).is("Could I possibly be ... a key???")
_:expect(_gc).is(vec2(0,0))
end)
Test still runs. Duplicate it, and plug in Spikes:
_:test("Spikes Test", function()
local thing = Spikes()
_:expect(thing:is_a(Spikes)).is(true)
thing:query()
_:expect(_msg).is("Could I possibly be ... a key???")
_:expect(_gc).is(vec2(0,0))
end)
This is just an experiment but I am hopeful. We get an error:
2: Spikes Test -- Spikes:56: attempt to index a nil value (local 'tile')
We can’t quite create a Spikes so cavalierly. It wants a Tile. Let’s try FakeTile. Now we get this message:
2: Spikes Test -- FakePlayerAndTile:16: attempt to call a nil value (method 'publish')
It’s trying to publish to our bus. Let’s just ignore that: Implement empty publish:
2: Spikes Test -- Spikes:64: attempt to call a nil value (method 'subscribe')
Grr. You can see why I don’t like this. With an empty subscribe method as well, I get the error I had hoped for:
2: Spikes Test --
Actual: Deadly Spikes do damage to tender feet when down and even more when up!,
Expected: Could I possibly be ... a key???
Now I see why I don’t want to test any classes other than Key this particular way.
Revert, Regroup
If I recall correctly, I tried that way of testing Key to see if it would be easier. I think it is not easier, and that it will wind up converging on the other test’s style anyway.
Let’s see if we can replicate the other style for the objects who aren’t done yet. Here’s that test:
_:test("Decor sets graphicCenter into informDotted", function()
Bus = FakeEventBus()
Bus:expect(nil,vec2(0,0))
local tile = FakeTile()
local item = "item"
local decor = Decor(tile, item, "Skeleton1")
decor:query() -- expectations are on the Bus
end)
Can I duplicate that with Spikes?
_:test("Spikes sets graphicCenter into informDotted", function()
Bus = FakeEventBus()
Bus:expect(nil,vec2(0,0))
local tile = FakeTile()
local thing = Spikes(tile)
thing:query() -- expectations are on the Bus
end)
Test:
17: Spikes sets graphicCenter into informDotted --
Actual: Deadly Spikes do damage to tender feet when down and even more when up!,
Expected: Thank you!
Hm where did that come from? Grr:
function FakeEventBus:subscribe(object, event)
self.checkMessage = "Thank you!"
self.subscriber = object
end
This is the trouble with hand-crafted fake objects: they are not easily reused. And if they are, they tend to become complicated to use.
I’m going to try something weird. Hold my chai.
Something Weird
All I really want to know, for these objects, is whether they call Bus:informDotted with two parameters, the second one a vector, when they are sent query.
I’m going to create a new test suite and see if I can just answer that question simply.
-- TestQuery
-- 20220605
function testQuery()
CodeaUnit.detailed = false
_:describe("Test query for sending position parameter", function()
_:before(function()
end)
_:after(function()
end)
_:test("First Test", function()
_:expect(2).is(3) -- change this to fail
end)
end)
end
Oops, forgot to revert first. OK. Do again. Now we’re hooked up:
1: First Test --
Actual: 2,
Expected: 3
_:test("Spikes send position parameter", function()
Bus = nil
local query = Spikes.query
query()
end)
I’m just fetching the frakkin query function out of Spikes and calling it. I expect an error trying to index Bus.
1: Spikes send position parameter -- Spikes:110: attempt to index a nil value (global 'Bus')
Perfect. Now a bus:
local _bus
function testQuery()
CodeaUnit.detailed = false
_:describe("Test query for sending position parameter", function()
_:before(function()
_bus = Bus
Bus = FakePosBus()
end)
_:after(function()
Bus = _bus
end)
_:test("Spikes send position parameter", function()
local query = Spikes.query
query()
end)
end)
end
FakePosBus = class()
I expect it doesn’t understand informDotted. Grr, I get:
1: Spikes send position parameter -- Spikes:110: attempt to index a nil value (local 'self')
OK, it couldn’t be that easy:
function Spikes:query()
Bus:informDotted("Deadly Spikes do damage to tender feet when down and even more when up!", self:graphicCenter())
end
We need enough of a self to respond to graphicsCenter. OK, so be it:
_:test("Spikes send position parameter", function()
local fakeSelf = FakeSelf()
local query = Spikes.query
query(fakeSelf)
end)
end)
end
FakeSelf = class()
FakePosBus = class()
Now I expect FakeSelf not to understand graphicCenter.
1: Spikes send position parameter -- Spikes:110: attempt to call a nil value (method 'graphicCenter')
Implement that:
function FakeSelf:graphicCenter()
return "fake graphic center"
end
Expect FakePosBus not to understand informDotted:
1: Spikes send position parameter -- Spikes:110: attempt to call a nil value (method 'informDotted')
Implement:
function FakePosBus:informDotted(message, pos)
_:expect(pos).is("fake graphic center")
end
And now I see where this gets tricky, because it’s going to do something weird to try to get the message.
Well, no, the test ran because Spikes return a literal message. OK, good. Let’s try this with something difficult, Monster.
_:test("Monsters send position parameter", function()
local fakeSelf = FakeSelf()
local query = Monster.query
query(fakeSelf)
end)
This time, I expect the monster sends some bizarre message.
2: Monsters send position parameter -- Monster:370: attempt to index a nil value (field 'behavior')
I’m not even going to look. Essentially I’m test-driving my classes here. So:
function FakeSelf:init()
self.behavior = self
end
Now we’ll send something to behavior, which will show up as missing in FakeSelf:
2: Monsters send position parameter -- Monster:370: attempt to call a nil value (field 'query')
OK, I think I wanna look. OK, that just returns a message. Fine:
function FakeSelf:query()
return "a message"
end
Test. And it runs. Monsters do in fact send their graphicCenter to informDotted. What else?
So tempting to parameterize this test. But no.
I duplicate the test for Lever. Test. It runs. Levers do the right thing. Let’s do WayDown. Runs. The only senders I’ve not tested here are Loot, Key, Decor, and NPC.
In for a penny. If we can test them all here, we can reduce complex test elsewhere.
Loot indexes item. Let’s actually look at what it does:
function Loot:query()
self.item:query(self:graphicCenter())
end
And that’s going to go to InventoryItem:
function InventoryItem:query(optionalPos)
if self.name ~= "nothing" then
Bus:informDotted(self.message, optionalPos)
end
end
I am tempted to take a shortcut but let’s make a fake item.
FakeItem = class()
function FakeItem:query(aPosition)
_:expect(aPosition).is("huh?")
end
FakeSelf = class()
function FakeSelf:init()
self.behavior = self
self.item = FakeItem()
end
This should fail, I’m not sure just how. Perfect:
5: Loots send position parameter --
Actual: fake graphic center,
Expected: huh?
Fix the implementation:
function FakeItem:query(aPosition)
_:expect(aPosition).is("fake graphic center")
end
Test runs. Do Key1:
_:test("Keys send position parameter", function()
local fakeSelf = FakeSelf()
local query = Key.query
query(fakeSelf)
end)
Expect success. Yes. Do Decor.
7: Decor send position parameter -- Decor:235: attempt to call a nil value (method 'queryMessage')
Implement that on FakeSelf:
function FakeSelf:queryMessage()
return "query message"
end
Tests run. NPC:
8: NPCs send position parameter -- NPC:127: attempt to index a nil value (field 'fsm')
Let’s follow our nose …
function FakeSelf:init()
self.behavior = self
self.item = FakeItem()
self.fsm = self
end
And test:
8: NPCs send position parameter -- NPC:127: attempt to call a nil value (method 'event')
We can have this just return, I think.
function FakeSelf:event()
return "event"
end
Test. Tests run. Would we like to reduce the duplication here?
_:test("Spikes send position parameter", function()
local fakeSelf = FakeSelf()
local query = Spikes.query
query(fakeSelf)
end)
_:test("Monsters send position parameter", function()
local fakeSelf = FakeSelf()
local query = Monster.query
query(fakeSelf)
end)
_:test("Levers send position parameter", function()
local fakeSelf = FakeSelf()
local query = Lever.query
query(fakeSelf)
end)
_:test("WayDowns send position parameter", function()
local fakeSelf = FakeSelf()
local query = WayDown.query
query(fakeSelf)
end)
_:test("Loots send position parameter", function()
local fakeSelf = FakeSelf()
local query = Loot.query
query(fakeSelf)
end)
_:test("Keys send position parameter", function()
local fakeSelf = FakeSelf()
local query = Key.query
query(fakeSelf)
end)
_:test("Decor send position parameter", function()
local fakeSelf = FakeSelf()
local query = Decor.query
query(fakeSelf)
end)
_:test("NPCs send position parameter", function()
local fakeSelf = FakeSelf()
local query = NPC.query
query(fakeSelf)
end)
Maybe later. Let’s commit: New TestQuery tests all existing dungeon objects for passing position to informDotted.
And let’s reflect.
Reflection
The good news is that we really have tested to be sure that each of these class’s query method sends two parameters to Bus:informDotted. That’s better than we had before. What we have not tested, however, is whether that second parameter is a vector, much less whether it is the object’s position in the Dungeon.
Still, it’s better than what we had.
I’m pleased with how we kind of TDD’d the tests fake objects, letting the failures tell us what we needed. That went very smoothly.
There’s perhaps a lesson here. The thing that was completely stopping me yesterday was pretty easy today … after first trying some other hard things, hoping that my existing tests were easy to extend. It’s possible that a more robust set of fake objects could have done the job. I’m sure that my London School colleagues could have whipped out some amazing test double mock recording almighty object that would have done the job, but I don’t have one of those in Lua and honestly, as a Detroit School2 kind of guy, I don’t want one.
What Now?
What shall we do now? We could break. It is Sunday and perhaps soon we’ll have our usual lovely brekkers and watch the old people show. In the meantime I could read. But I do have the feeling that there is some duplication to remove, and I would also like to change to a scheme where the dialog line follows the Monster.
Let’s have a look at all the query methods in DungeonObjects:
function DungeonObject:query()
-- default
end
function Monster:query()
Bus:informDotted(self.behavior.query(self), self:graphicCenter())
end
function WayDown:query()
Bus:informDotted("I am the Way Down! Proceed with due caution!", self:graphicCenter())
end
function Loot:query()
self.item:query(self:graphicCenter())
end
function Key:query()
Bus:informDotted("Could I possibly be ... a key???", self:graphicCenter())
end
function Spikes:query()
Bus:informDotted("Deadly Spikes do damage to tender feet when down and even more when up!", self:graphicCenter())
end
function Decor:query()
Bus:informDotted(self:queryMessage(self.kind), self:graphicCenter())
end
function InventoryItem:query(optionalPos)
if self.name ~= "nothing" then
Bus:informDotted(self.message, optionalPos)
end
end
function Lever:query()
Bus:informDotted("At first glance, this appears to be a lever.\nDo you think it does anything?", self:graphicCenter())
end
function NPC:query()
self.fsm:event("query")
end
function NPC:publishTale(fsm, taleName)
local tale = fsm.tab.data[taleName]
local done, msg = tale:next()
msg = self:name()..": "..msg
Bus:informDotted(msg, self:graphicCenter())
return done
end
A brief glance at NPC’s FSM table told me that it calls back to publishTale, where the informDotted` is done.
So all these objects are calling informDotted and most of them are doing it rather quickly. I’m thinking of a refactoring leading to this situation:
- All or most DungeonObjects inherit the
queryimplementation from DungeonObject; - That top-level method calls
self:getQueryMessage()andself:getQueryLocation(), or words to that effect; - Individual classes implement those methods according to their likes. Most of them do not implement
getQueryLocationwhich defaults toself:graphicCenter, again in DungeonObject.
The only Bus call to informDotted would wind up in DungeonObject and we’d be positioned to change how that works for location, because there would be only one (possibly a few) calls to graphicCenter.
This is a fairly simple refactoring, though it is repetitive from class to class. That’s good, as it means we can commit multiple times and do it over days if we needed to. We don’t need to, of course, although if I start today I probably won’t finish. So even here …
Let’s try it. Tests are green. fresh commit.
Refactoring query
I’ll start by putting the new query code into DungeonObject. This is harmless because no one inherits it. Everyone currently overrides. (as far as I know … I suppose there may be DungeonObjects who don’t respond. We’ll not worry about that. Perhaps we should. I’m sorry I brought it up
function DungeonObject:query()
local msg = self:getQueryMessage()
local pos = self:getQueryLocation()
Bus:informDotted(msg, pos)
end
function DungeonObject:getQueryLocation()
return self:graphicCenter()
end
function DungeonObject:getQueryMessage()
return "DungeonObject"
end
This should be harmless. Test. Green. Commit: Begin moving query to DungeonObject. Basic methods.
Now to do the easiest one:
function WayDown:query()
Bus:informDotted("I am the Way Down! Proceed with due caution!", self:graphicCenter())
end
This becomes:
function WayDown:getQueryMessage()
return "I am the Way Down! Proceed with due caution!"
end
I apologize in advance for testing this in the game. We’ll think about the test shortly. But no. I get a surprising error:
4: WayDowns send position parameter -- Entity:21: attempt to call a nil value (method 'getQueryMessage')
Ah, that’s good, actually. We need that method in our Fake object:
function FakeSelf:getQueryMessage()
return "query message"
end
Now to test in the game. LOL not yet:
4: WayDowns send position parameter -- Entity:22: attempt to call a nil value (method 'getQueryLocation')
My bad.
function FakeSelf:getQueryLocation()
return "fake graphic center"
end
Now can I get a green bar? Yes. The WayDown works in the game and passes the tests.
Let’s do another easy one. Spikes should be easy.
function Spikes:query()
Bus:informDotted("Deadly Spikes do damage to tender feet when down and even more when up!", self:graphicCenter())
end
That becomes, as before:
function Spikes:getQueryMessage()
return "Deadly Spikes do damage to tender feet when down and even more when up!"
end
Test. Should be green. And works. Should have committed once more already, let’s do it now: Commit: Spikes and WayDown use inherited query. TestQuery enhanced to support inherited query.
Breakfast is nearly in the skillet, so let’s sum up.
Summary
I feel quite righteous, having tried so many times to get this simple change at least somewhat tested, and while the final tests are a bit tricky, because they fetch a specific method without creating the instance, they do accomplish what I needed, which was to be sure that everyone was calling graphicCenter. This is a bit over-detailed but at the time it was the common element that needed to be done.
I don’t think they’re great tests, but they did the job and they are going to help me refactor this code to a better implementation, and, I believe, to switch over to the more dynamic version for Monsters.
There is a larger lesson, however, which is that DungeonObjects are still too tightly connected to other objects. They require a tile, though they do not do much with it, and they access the Bus. Using a fake Bus has been helpful, but I think maybe we can do better.
Still, this is progress and will also improve the DungeonObject hierarchy
The Dungeon program has a life of its own. I guess since it is such a well of interesting examples, I’ll keep looking at it and finding things to write about until I get a better idea.
This morning pleased me more than yesterday, for sure!
I hope you’ll stick with me. And if you have a better idea for what I should do, let me know!
-
He said do key! ↩
-
There are two “schools” of TDD-style testing, Detroit, and London. Claims that there is a “Chicago” school refer to some strongly-contributing Chicago people, for whom I do sincerely have the greatest respect for their early work bringing these ideas to the world. But it’s Detroit, not Chicago. I have spoken. ↩