Space Invaders 55
Another defect. I’m starting to feel … afraid. That’s not a good sign.
Dave1707 of the Codea forum has found another defect. It looks like this:
The lead invader is out of step. It turns out that she doesn’t step downward.
I knew almost immediately what the defect was. Yesterday, I said this:
function Army:updateOneLiveInvader(motion)
local continue = true
while(continue) do
continue = self:nextInvader():update(motion, self)
end
end
Note that I passed in the motion vector. Why? Well, the real reason is that I intend to override it in the test. It’s good for the game, however, because it can save a table access. Are you buying that? I’m not, not really. I did it for the test. I could have hammered
motionin my test but this seemed better.
At this moment, I’m not sure exactly why this fails, but I do know that if I pass in self.motion to update instead of just motion, the invader stays in step.
This was supposed to be a behavior-maintaining change, a refactoring. But it wasn’t. My tests didn’t find it, and rather obviously I didn’t let the game run very long at the time of commit yesterday, or I’d have seen the problem.
When this sort of thing happens–when we do this sort of thing–when we make this kind of mistake–it tends to reduce our confidence. We become less willing to make changes to the code, changes that we would otherwise consider desirable. Since, in the words of my friend GeePaw Hill, we are in the business of harvesting the value of change, being afraid to change things is, how can I say this … bad.
Now I have a luxury that you may not have. I’m writing to show what really happens to someone who is writing software, using the best thinking and practice they can bring to the keyboard on a given day. If things go bad, that’s embarrassing, but if things always went perfectly, these articles wouldn’t serve their purpose. When things go bad, that’s good for these articles.
My slight but growing fear of changing this program isn’t something wrong with me. I shouldn’t scarf a Xanax and carry on. I need to listen to that fear, accept that it’s not telling me something about me, it’s telling me something about this program. Very likely it is telling me two things:
- This program is getting pretty complicated, and as is always the case, some of that complication is probably accidental. The program isn’t as simple as it needs to be for my finite mind to deal with it.
- This program does not have enough good tests associated with it. When we have a good array of tests for the program, whenever we step out of bounds, a test will usually spot the violation and blow the whistle.
I have “always said” that when a defect appears in our program, we should first write a test to show that defect. and then fix the defect. I’ve “always said” that we should then think about what other problems might arise that are like the one we just found, and write tests for those as well.
I have also “always said” that I don’t know any good ways to test graphical programs like this one.
I have also always done less well than I’ve “always said”. This is not unique to me: most of us probably often fall short of our ideals. No? Just me? Beuller? Well, anyway, that’s me, and just now the mismatch between ideals and reality has bitten me.
I’m not at all sure how to test for this problem. The issue occurs when motion changes. Here’s some of that code:
function Army:updateOneLiveInvader(motion)
local continue = true
while(continue) do
continue = self:nextInvader():update(motion, self)
end
end
function Army:nextInvader()
self:handleCycleEnd()
local inv = self.invaders[self.invaderNumber]
self.invaderNumber = self.invaderNumber + 1
return inv
end
function Army:handleCycleEnd()
if self.invaderNumber > #self.invaders then
self.armySize = self.invaderCount
self.invaderCount = 0
self.invaderNumber = 1
self:adjustMotion()
end
end
function Army:adjustMotion()
if self.overTheEdge then
self.overTheEdge = false
self.motion = self.motion*reverse + stepDown
else
self.motion.y = 0 -- stop the down-stepping
end
end
Imagine that we’re in updateOneLiveInvader. We have a cached version of self.motion in the method. We call nextInvader, which calls handleCycleEnd, which wraps from 55 down to 1, and caches the next (first) invader. Then it calls adjustMotion which, if we’re over the edge, changes the value of self.motion. But we’re holding on to the old value, so when all this returns from nextInvader, we call update with the old value of motion. And the first invader does not march downward, in a display of abject cowardice … no, she just never got the word.
Well, the first test that comes to mind is to somehow set up everything so that we’re at the edge, and about to wrap around, and then check to be sure that the first invader steps down. I think that test is capable of being written. A big concern to me just now is that the test really isn’t “obvious”: I can’t think why I’d have written it in the past. So I can certainly write it now, to show the defect and then show it fixed, but it may not be the test we really need for this stepping logic, because we wouldn’t have written it.
So I need to think about the logic of the situation and see if a more sensible test comes to mind.
We’re marching along, invader by invader, telling them to update by the current motion. If they get to the end of the screen, we want everyone to step downward by 8 and reverse direction. We want to do that only if everyone is processed. But we only want them to step downward once, until they hit the other edge. So after everyone is processed one more time, we want to go back to the regular sideways step.
Easy enough to say. Well, maybe not even that. But it gets worse, because we only move one invader at a time. We call our update 55 times before they’re all moved. And in principle, we should check every one of them. It might make sense to just check the first and last. Checking the first would be the test we need now.
Pressure
Here we are. We have a broken game out there in the wild. We know the fix. Our boss is hanging over us, perhaps shouting some unintelligible garbage about fixing this damn bug right now dammit or dammit heads will roll. We’re feeling the pressure and we know the fix. We know we “should” write this test, but we know it’s hard to write.
Do we fold, and do the fix, with good intentions about putting the test in after the fix is out? We might. And if we do, might the boss then switch, still angry, to ask where the hell is that feature that you were supposed to be working on today? They might.
Might we skip that test and move on, because after all, the bug is fixed?
We might.
To tell the truth,1 I’m tempted to say, right here: “I might, and in fact that’s what I’m going to do”.
But today, I’m not going to do that. Today, I’m going to write at least the test that shows the bug. Why?
Well, I want to improve my testing, and when I find that this test isn’t as hard to write as I fear, it might make me less concerned about difficult tests.
Second, either way, it will help create a stronger testing habit.
Third, I feel that I owe it to my reader to show them that I’m capable of doing the right thing once in a rare while.
But before that, I’m going for chai.
Chai Run Reflections
The nice thing about a chai run in the middle of the job here is that it gives me a pleasant drive to reflect on the work without the immediate pressure of doing it or writing about it. This time, I was thinking “how would updating the invaders work if it had a really nice implementation?”
I don’t have an answer but overall I think it’s actually pretty close to really nice. I could imagine an “update processor” object that encapsulated just the few things that go on in updating, mainly those four methods I printed earlier. That would break out just that bit of logic from the Army’s other concerns, because its other main concern, dropping bombs, isn’t much like updating invaders at all. So perhaps those two different notions are asking to be separated.
But the actual implementation is pretty nice, at least as I imagine it without looking at the code. When we look at the code we often see things that our memory of the design glosses over, so there may be those things in there. But overall, it ticks through one more live invader, handles wraparound, handles having seen the edge, handles stepping down (albeit newly incorrectly as of yesterday’s mistake), and so on.
Even if I saw a much better way, I would hesitate to refactor to a new design having just seen that the tests aren’t strong enough to protect me here. So other than very gentle refactoring to support some new tests, I’ll leave this design in place until it’s better supported.
As for how to test, my imagined test will go something like this:
- Set up the army
- Record the positions of first and last invader
- Run one update cycle by calling it 55 times
- Check the positions of the recorded invaders
- Begin another cycle, doing a couple of invaders
- Trigger the “at edge”
- Continue the update cycle
- Check to see that no one has stepped down
- Do another cycle
- Check to see that both 1 and 55 have stepped down
- Maybe do another cycle to show that they don’t step down
Now that’s a lot. And It doesn’t fail until about step ten, which doesn’t please me. Maybe if I write small tests with just a few assertions, I can do the concerning one first, and then the rest. I’ll try that: it’s the one that shows the defect anyway, so it deserves to be first. Maybe this will do:
_:test("Invaders step downward after hitting edge", function()
local army = Army()
local inv01 = army.invaders[1]
local inv55 = army.invaders[55]
local pos01 = inv01.pos
local pos55 = inv55.pos
army:invaderOverEdge(inv55)
for i = 1,55 do
army:updateOneLiveInvader(army.motion)
end
_:expect(inv55.pos.y).is(pos55.y-8)
_:expect(inv01.pos.y).is(pos01.y-8)
end)
I’m far from certain that this will fail. If my life is perfect, the first expect will succeed, and the second fail. Unfortunately, they both fail, we did not step down. I’m not too surprised but I am disappointed.
My next try will be to move a few of them, set the over the edge, and then move the rest. That should leave everyone at the same y the first time through. Then we’ll go again and check them for moving down.
_:test("Invaders step downward after hitting edge", function()
local army = Army()
local inv01 = army.invaders[1]
local inv55 = army.invaders[55]
local pos01 = inv01.pos
local pos55 = inv55.pos
for i = 1,3 do
army:updateOneLiveInvader(army.motion)
end
army:invaderOverEdge(inv55)
for i = 4,55 do
army:updateOneLiveInvader(army.motion)
end
_:expect(inv55.pos.y).is(pos55.y)
_:expect(inv01.pos.y).is(pos01.y)
for i = 1,55 do
army:updateOneLiveInvader(army.motion)
end
_:expect(inv55.pos.y).is(pos55.y-8)
_:expect(inv01.pos.y, "first guy").is(pos01.y-8)
end)
This passes three times and fails once. Just what I wanted, we’ve caused our bug.
31: Invaders step downward after hitting edge first guy -- Actual: 105.0, Expected: 97.0
Now let’s fix it. I observe that we pass in the motion and the army. The invader can ask the army for its motion. Let’s fix it that way, with a new method on Army, invaderMotion:
function Army:invaderMotion()
return self.motion
end
Now we remove the motion parameter from this whole nest:
function Army:update()
self:updateBombCycle()
self:possiblyDropBomb()
self:updateOneLiveInvader()n-- <---
self.rollingBomb:update(self)
self.plungerBomb:update(self)
self.squiggleBomb:update(self)
self.saucer:update(self)
end
function Army:updateOneLiveInvader()
local continue = true
while(continue) do
continue = self:nextInvader():update(self)
end
end
And we cause Invader to ask for the motion:
function Invader:update(army)
if not self.alive then return true end
army:reportingForDuty()
self.picture = (self.picture+1)%2
self.pos = self.pos + army:invaderMotion()
Shield:checkForShieldDamage(self)
if self:overEdge() then
army:invaderOverEdge(self)
end
return false
end
I do expect this to fix the defect.
31: Invaders step downward after hitting edge first guy -- OK
And it does. Commit: fixed defect first invader didn’t step down. Well, I’d commit if I could. Working Copy seems to have been torpedoed by iPadOS 14.0. Moving right along, we have a good version.
I should beef up that test a bit but it’s actually pretty solid as it stands, especially if I don’t intend any further refactoring. We’re way better off than we were, at least. And I do have another issue to deal with.
Dave1707 (getting to be my nemesis in a friendly kind of way at least he runs my program like many of you don’t) doesn’t have CodeaUnit and doesn’t want to have it, so he has to comment out the runTests and showTests calls from Main. Let’s change things so that those don’t happen if you have no CodeaUnit:
function setup()
if CodeaUnit then runTests() end
parameter.boolean("Cheat", false)
Runner = GameRunner()
...
function draw()
pushMatrix()
pushStyle()
background(40, 40, 50)
if CodeaUnit then showTests() end
Runner:draw()
popStyle()
popMatrix()
Runner:update()
end
That should do the trick. But when I ran the tests, I got a test failure:
21: bomb drops below bottom-most live invader -- Actual: 26.0, Expected: 40.0
I’d swear that didn’t break before. But it must have. Certainly those if statements didn’t break anything. I don’t like what I did there, however, so I’ll back them out, make sure the test still breaks, then see what happened.
Test still fails. I must have been looking so carefully at my one test result that I didn’t see the screen turn red. What do I need, a siren?
Here’s the test:
_:test("bomb drops below bottom-most live invader", function()
local army = Army()
local v, vold
vold = army:bombPosition(1)
army:directKillInvader(1)
for inv = 1,55 do
army:updateOneLiveInvader(vec2(16,0))
end
v = army:bombPosition(1)
_:expect(v.x).is(vold.x + 16)
end)
Ha, easy enough. Our update doesn’t take a parameter any more. But what’s the deal on this test. It’s only testing the x coordinate. Oh, yes, the Y is checked elsewhere, this is a check for the bombs tracking right and left with the invaders. We can hammer army.motion to make this work:
_:test("bomb drops below bottom-most live invader", function()
local army = Army()
local v, vold
vold = army:bombPosition(1)
army:directKillInvader(1)
army.motion = vec2(16,0)
for inv = 1,55 do
army:updateOneLiveInvader()
end
v = army:bombPosition(1)
_:expect(v.x).is(vold.x + 16)
end)
And the tests are … not green, still yellow, because of the ignored one. We’ll look at that soonish.
Now let’s put the CodeaUnit changes in, but in better places:
function runTests()
if not CodeaUnit then return end
local det = CodeaUnit.detailed
CodeaUnit.detailed = false
Console = _.execute()
CodeaUnit.detailed = det
end
function showTests()
if not CodeaUnit then return end
pushMatrix()
pushStyle()
...
I think that’s better. First I’ll check that the tests run … and they do. Then I’ll undefine CodeaUnit and see that they don’t:
function setup()
CodeaUnit = nil
runTests()
parameter.boolean("Cheat", false)
...
And they don’t run. Sweet. I’m update my CUBase template with those changes for the next app.
We’re done for the morning, let’s sum up.
Summing Up
Yesterday, n hopes of making a test less invasive, I cached the Army’s motion variable in a calling sequence. But it wasn’t supposed to be cached. There were no tests to detect the problem. And the developer didn’t run the program long enough to see the problem on screen.
Result: we shipped a defect.
Today, we thought about the problem and attributed it correctly to the difference between our ideals: always test anything that could break, and our actuals: test what seems to need it and isn’t too hard to test.
That difference was too broad and a defect fell through the cracks.
Writing the test wasn’t really that difficult. It is a bit weird, and I wish it were easier, but the logic in question is pretty complicated, so it’s no surprise that it’s a bit difficult to set up. And it wasn’t THAT difficult, was it?
What I hope to take away from today are two lessons:
First, that test that I don’t want to write isn’t really that hard.
Second, the program is getting complicated, so we really need that test.
Third–I liked about the “two”–the program is getting complicated, let’s bear down a bit on uncomplicating it. De-complificating? Anti-complification? Simplifying, that’s it!
See you next time!
-
He generally does tell the truth. I think it’s because he finds it difficult to remember the lies, so he would get caught out a lot. ↩