Dungeon 295: A Nice Idea

Yesterday, testing something required my code to be a bit less smooth than I’d have liked. Today I have an idea that I think is going to be quite fine. It’s not even too clever.

Yesterday, I wanted to test the sequence of bumps against a Decor so that the user would see this sequence:

Observe Decor;
Bump Decor. No entry, Loot appears;
Bump Decor. No entry, receive Loot;
Bump Decor. Entry allowed.

The test for this is this:

        -- Decor requires three tries to enter,
        -- once to materialize the Loot
        -- once to fetch the loot without entering
        -- once to be allowed to enter thereafter.
        _:test("Decor open state", function()
            local noAction = function() end
            local noTile = FakeTile()
            local decor = Decor(noTile,item)
            decor:actionLogic(nil, noAction)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionLogic(nil, noAction)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionLogic(nil, noAction)
            _:expect(decor:isOpen(),"3").is(true)
        end)

To make this test possible, I broke up the operation of Decor’s actionWith method in a somewhat unnatural way:

function Decor:actionWith(aPlayer)
    self:actionLogic(aPlayer, self.actionOperation)
end

-- pluggable action for testing
function Decor:actionLogic(aPlayer, actionMethod)
    self.actionCount = self.actionCount + 1
    if self.actionCount == 1 then
        actionMethod(self,aPlayer)
    end
end

function Decor:actionOperation(aPlayer)
    self:changeAppearance()
    self:showLoot()
    self:applyDamage(aPlayer)
end

It’s not entirely easy to see what this code does. This was done so that the test could check actionLogic to be sure that it did what I expected. And even worse, to determine if the Decor is actually “open”, I did this:

function Decor:isOpen()
    return self.actionCount > 2
end

This all works, but it doesn’t please me. It’s not at all easy to understand. I feel that if I come back here after a while, I won’t understand it.

Then, late yesterday, while nappXXXX meditating, I had an idea that I think is going to work out nicely.

ActionSequence

I propose to write a small object, ActionSequence. It is created with a table of functions, and a receiver object. It will be called its method, probably called action, and each time it is called, it executes the next function in the table and returns. (As I write this, I realize it should return the result of the function.) When it gets to the end of the list … well, that remains to be seen, but I think it should have two options: repeat the last item on every subsequent call, or do nothing on subsequent calls.

Naturally, I plan to build this with TDD. Here goes:

I begin with this test:

-- ActionSequence
-- 20220430

function testActionSequence()
    CodeaUnit.detailed = false
    
    _:describe("ActionSequence", function()
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("ActionSequence", function()
            local seq = ActionSequence()
        end)
    end)
end 

That should fail for want of anything named ActionSequence:

1: ActionSequence -- ActionSequence:16: attempt to call a nil value (global 'ActionSequence')

And we code:

ActionSequence = class()

Test should be green. It is. Now to test something. Because I intend to use this object as part of a class wanting a sequence, I’ll write a small test class that uses it.

        _:test("ActionSequenceTest", function()
            local user = ActionSequenceTest()
        end)

This is almost silly but the tiny steps build up a nice rhythm. Test says this:

2: ActionSequenceTest -- ActionSequence:20: attempt to call a nil value (global 'ActionSequenceTest')

Which requires this:

ActionSequenceTest = class()

NO. This is too far a reach. I see how I’d do it, but I don’t see quite how I’d TDD it. Take a smaller step.

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,seqNr)
                _:expect(rcvr).is(user)
                _:expect(seqnr).is(1)
            end
            local seq = ActionSequence(user, {f1})
            seq:action()
        end)

OK, this creates an instance of my test class, just to have it as a receiver. It expects that, on action, the ActionSequence will call f1. That’s enough to drive out some behavior. Test.

3: ActionSequence first action -- ActionSequence:31: attempt to call a nil value (method 'action')

OK, makes sense. Now:

I coded this and really expected it to work:

function ActionSequence:init(receiver, sequence)
    self.receiver = receiver
    self.sequence = sequence
    self.count = 0
end

function ActionSequence:action()
    self.count = self.count + 1
    return self.sequence[count](receiver)
end

But I get this error:

3: ActionSequence first action -- ActionSequence:47: attempt to call a nil value (field '?')

That says to me that … oh … forgot to say self:

function ActionSequence:action()
    self.count = self.count + 1
    return self.sequence[self.count](receiver)
end

I love it when a test surprises me. Well, sometimes I love it. Anyway now I think this should work.

Except that I made two mistakes. No self on the receiver, and I’m supposed to be sending in the sequence number. Wow, I’m glad I have tests for this trivial thing.

function ActionSequence:action()
    self.count = self.count + 1
    return self.sequence[self.count](self.receiver, self.count)
end

NOW it had better work! It doesn’t!

3: ActionSequence first action  -- Actual: nil, Expected: 1

Good Lord I’m clumsy today. The test uses two different capitalizations:

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,seqNr)
                _:expect(rcvr).is(user)
                _:expect(seqnr).is(1)
            end
            local seq = ActionSequence(user, {f1})
            seq:action()
        end)

Let’s make the test a bit better:

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,count)
                _:expect(rcvr).is(user)
                _:expect(count).is(1)
            end
            local seq = ActionSequence(user, {f1})
            seq:action()
        end)

Now? Yes, test runs now. Let’s continue along these lines for a bit. Maybe I can convince myself that I don’t really need the ActionSequenceTest class after all.

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,count)
                _:expect(rcvr).is(user)
                _:expect(count).is(1)
                return "ok 1"
            end
            local f2 = function(rcvr,count)
                _:expect(rcvr).is(user)
                _:expect(count).is(2)
                return "ok 2"
            end
            local seq = ActionSequence(user, {f1})
            local r1 = seq:action()
            _:expect(r1).is("ok 1")
            local r2 = seq:action()
            _:expect(r2).is("ok 2")
            local r3 = seq:action()
            _:expect(r3).is(nil)
        end)

This is a bit more complicated, not as simple as my earlier steps. But I expect that it will pass the OK tests and fail on the nil test, trying to call a nil function

3: ActionSequence first action -- ActionSequence:58: attempt to call a nil value (field '?')

Perfect. Implement the limit:

function ActionSequence:action()
    self.count = self.count + 1
    if self.count > #self.sequence then
        return nil
    else
        return self.sequence[self.count](self.receiver, self.count)
    end
end

I expect this to be green.

3: ActionSequence first action  -- Actual: nil, Expected: ok 2

Interesting, since that worked before the if statement.

Well, I didn’t pass in the correct list. First fix the test.

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,count)
                _:expect(rcvr).is(user)
                _:expect(count).is(1)
                return "ok 1"
            end
            local f2 = function(rcvr,count)
                _:expect(rcvr).is(user)
                _:expect(count).is(2)
                return "ok 2"
            end
            local seq = ActionSequence(user, {f1, f2})
            local r1 = seq:action()
            _:expect(r1).is("ok 1")
            local r2 = seq:action()
            _:expect(r2).is("ok 2")
            local r3 = seq:action()
            _:expect(r3).is(nil)
        end)

It’s green. But I don’t understand how it ever got r2 to be “ok 2” without f2 being in the table.

Here’s a bit of a dilemma. The code is working now. I could move on: after all, it’s obviously correct. But why didn’t it fail on the “ok 2” when the array only had one function in it. Why didn’t it fail trying to call nil?

I decide that I really want to understand what happened, since it’s possible that there’s something important that I don’t understand. I back out the most recent changes. Kind of wish I’d been committing.

function ActionSequence:action()
    self.count = self.count + 1
    return self.sequence[self.count](self.receiver, self.count)
end

Test.

3: ActionSequence first action -- ActionSequence:58: attempt to call a nil value (field '?')

Ah. It’s not the line I expected. We didn’t pass the “ok 2” after all. OK, put it back:

function ActionSequence:action()
    self.count = self.count + 1
    if self.count > #self.sequence then
        return nil
    else
        return self.sequence[self.count](self.receiver, self.count)
    end
end

And test. Green. I decide to leave the test class, which is really only used to give the test something to check. And now, let’s commit this baby: New ActionSequence class for use in Decor.

OK, short break for a chai and a chat.

Break

This went just about the way one would want. Small object, fairly direct tests. Some learning along the way. Some small confusions, but nearly a straight line to what we wanted.

Note that I’ve only implemented one of the versions contemplated, the one that calls your functions and if it runs off the end, just returns nil. I see that the other version, that repeats that last call indefinitely, is possible, but I have no use for it. So I don’t implement it.

Now, let’s use this thing. It will break our existing Decor test, and unless we want to do some major setup, we still dare not call Decor’s actual working functions. I’m troubled by this. I had not thought this far ahead. This is a better implementation for Decor, but it’s not directly tested.

I’ll go ahead and fix Decor to use the new class.

This turns out to be rather a lot:

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

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

function Decor:isOpen()
    return self.open
end

This will not work, because I’m trying to pass aPlayer to the call to action, and it does not accept further parameters. We’ll have to improve that.

I’m more concerned about losing the logic test here but let’s press on and see where we wind up. Testing will cause the old actionLogic test to fail, and I think we’ll find that decor doesn’t work in the game. I hate having to test in the game, however.

I manage to find a Decor that errors:

CombatRound:128: attempt to index a number value (field 'defender')
stack traceback:
	CombatRound:128: in method 'applyDamage'
	Decor:164: in method 'damage'
	Decor:153: in field 'danger'
	Decor:139: in method 'applyDamage'
	Decor:126: in function <Decor:123>

Let’s review the code to be sure this is what I think it is, the failure to pass in additional parameters to the functions. Yes, we’re in applyDamage, which does want the player:

function Decor:firstAction(aPlayer)
    self:changeAppearance()
    self:showLoot()
    self:applyDamage(aPlayer)
end

So fix the ActionSequence. I had intended to do this already but I was so excited that it was working that I forgot.

I enhance the test to expect arbitrary parameters:

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,count, a1,a2)
                _:expect(a1).is(1)
                _:expect(a2).is(2)
                _:expect(rcvr).is(user)
                _:expect(count).is(1)
                return "ok 1"
            end
            local f2 = function(rcvr,count, w1,w2)
                _:expect(w1).is("hello")
                _:expect(w2).is("world")
                _:expect(rcvr).is(user)
                _:expect(count).is(2)
                return "ok 2"
            end
            local seq = ActionSequence(user, {f1, f2})
            local r1 = seq:action(1,2)
            _:expect(r1).is("ok 1")
            local r2 = seq:action("hello","world")
            _:expect(r2).is("ok 2")
            local r3 = seq:action()
            _:expect(r3).is(nil)
        end)

I expect the new checks to fail. They all do. Implement:

function ActionSequence:action(...)
    self.count = self.count + 1
    if self.count > #self.sequence then
        return nil
    else
        return self.sequence[self.count](self.receiver, self.count, ...)
    end
end

All I’ve done there is add the “…” for additional parameters, and include them in the call at the bottom. I expect the tests to work. They do. I expect the game to work, and am sad that I have to try it in the game.

Same error:

CombatRound:128: attempt to index a number value (field 'defender')
stack traceback:
	CombatRound:128: in method 'applyDamage'
	Decor:164: in method 'damage'
	Decor:159: in field 'danger'
	Decor:139: in method 'applyDamage'
	Decor:126: in function <Decor:123>
	(...tail calls...)

That is more than a little disconcerting.

I do this:

function Decor:firstAction(aPlayer)
    assert(aPlayer:is_a(Player), "expected player")
    self:changeAppearance()
    self:showLoot()
    self:applyDamage(aPlayer)
end

I get this.

Decor:124: attempt to index a number value (local 'aPlayer')
stack traceback:
	Decor:124: in function <Decor:123>
	(...tail calls...)
	Decor:120: in method 'actionWith'
	Player:272: in local 'action'
	TileArbiter:37: in method 'moveTo'
	Tile:129: in method 'attemptedEntranceBy'
	Tile:442: in function <Tile:440>

And I remember that my action functions need to expect the sequence number. That’s probably a bug in my mind, it is a feature that I do not need. Rather than change my user code, I’ll go back and remove the unnecessary “feature”.

I remove the count checks:

        _:test("ActionSequence first action", function()
            local user = ActionSequenceTest()
            local f1 = function(rcvr,a1,a2)
                _:expect(a1).is(1)
                _:expect(a2).is(2)
                _:expect(rcvr).is(user)
                return "ok 1"
            end
            local f2 = function(rcvr,w1,w2)
                _:expect(w1).is("hello")
                _:expect(w2).is("world")
                _:expect(rcvr).is(user)
                return "ok 2"
            end
            local seq = ActionSequence(user, {f1, f2})
            local r1 = seq:action(1,2)
            _:expect(r1).is("ok 1")
            local r2 = seq:action("hello","world")
            _:expect(r2).is("ok 2")
            local r3 = seq:action()
            _:expect(r3).is(nil)
        end)

And remove the additional argument in the call:

function ActionSequence:action(...)
    self.count = self.count + 1
    if self.count > #self.sequence then
        return nil
    else
        return self.sequence[self.count](self.receiver, ...)
    end
end

I expect my ActionSequence tests to pass, the test in Decor to continue to fail, and I expect to have the game work, and I continue to be sad about testing in game.

It takes me forever to find a Decor that deals damage, but when I do, it works. But the whole point yesterday was to test the sequence logic (not necessarily the damage logic) without entering the game.

Where we stand now is that the ActionSequence itself is tested. I’m satisfied that it works as intended. What is not tested is whether the Decor properly delays entry until the third attempt. (And that isn’t really quite what we want.)

I’m going to resort to a small monkey patch, to get the test that I really want.

I try this:

        _:test("Decor open state", function()
            local noAction = function() end
            local noTile = FakeTile()
            local decor = Decor(noTile,item)
            decor.danger = Decor.doNothing
            local player = nil
            decor:actionWith(player)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"3").is(true)
        end)

That fails, and I am surprised:

4: Decor open state -- Decor:147: attempt to index a nil value

What is that?

function Decor:showLoot()
    self:currentTile():moveObject(self.loot)
end

Ah, bummer. We really dare not call that firstFunction at all, at least without my other monkey patch in there.

        _:test("Decor open state", function()
            local noAction = function() end
            local noTile = FakeTile()
            local decor = Decor(noTile,item)
            decor.danger = Decor.doNothing
            decor.currentTile = function()
                return noTile
            end
            local player = nil
            decor:actionWith(player)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"3").is(true)
        end)

Hm. I’ve got a solid test here, but I had to patch Decor to make it work. Let’s try something else, a Decor subclass, TestableDecor.

        _:test("Decor open state", function()
            local noAction = function() end
            local noTile = FakeTile()
            local decor = TestableDecor(noTile,item)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"3").is(true)
        end)

And this class:

TestableDecor = class(Decor)

function TestableDecor:firstAction()
    -- nothing
end

And the test runs and it makes sense. And no monkey patching. So I’ve got that going for me. Junk in the test, though, which I didn’t even notice.

        _:test("Decor open state", function()
            local noTile = FakeTile()
            local decor = TestableDecor(noTile)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"1").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"2").is(false)
            decor:actionWith(player)
            _:expect(decor:isOpen(),"3").is(true)
        end)

We’re good. I don’t even feel the need to test in game. Commit: Decor uses ActionSequence. Open on third bump has microtest.

Let’s sum up.

Summary

I thought of an object that made for a cleaner implementation of calling a different function on each call to actionWith, to enable things like Decor to have a bit of time-dependent state. I implemented too much, speculatively, while also forgetting that I needed to pass parameters through the action call.

Putting the object into play uncovered both of these problems. I’d have done better, when thinking about the object, to have made notes of what it had to do, and to have written tests. But I didn’t make notes, and did forget.

My old Decor test, which was pretty inside out, no longer worked, as I anticipated. That left me needing to test in-game, which made me sad.

I went through a few iterations of a real test for the isOpen sequencing, and at first, had to monkey-patch two things in Decor. Instead, I wrote a TestableDecor subclass, which overrides the method that made the monkey-patching necessary. The test is now clean and the code is certainly better than yesterday.

I think it’s possible that had I thought of the TestableDecor yesterday, I’d have had a cleaner test for the opening logic and would therefore not have wanted the ActionSequence. The Decor code would have been a bit more procedural, but acceptable, I think.

So was this trip necessary? “Shouldn’t” I have “just” used the Testable subclass yesterday and avoided the ActionSequence?

Yes, the trip was necessary, because these were the steps that I saw. The path has been a bit longer, perhaps, than the “should” path, but the result is also a bit more clean.

Every time I sit down with the code, no matter how brilliant my starting plan is, the code always surprises me. It wants to do something different from what I thought should happen. It shows me something that I hadn’t thought about. Sometimes, it pokes a stick into the spokes of my mental bicycle and I fall on my nose.

The code is always the truth. My ideas, well, I enjoy them but sometimes they mislead me. By sometimes, I mean almost always. Still, they’re fascinating and push me in interesting and useful directions. They just need that tie to reality called “the code”.

A fine morning. See you next time!

D2.zip