Adventures in TDD – fillin’ a hole in the SignalCommandMap

A simple question: What did you have for breakfast this morning?

Me? I had toast. 2 slices, white, with Flora and some great peach jam that we brought back from Spain.And a cup of tea, white with 2 sugars.

Another question: What didn’t you have for breakfast this morning?

Me? I didn’t have oatmeal. Or eggs. Or a hippopotamus, or lego, or chainsaw oil. The list of things I didn’t have is endless. Literally endless.

It’s impossible to fully scope the list of things that something doesn’t do, and yet as developers it’s important that we don’t just know what our code does, we also know what it doesn’t do.

This weekend I found and fixed a potentially nasty hole in the robotlegs SignalCommandMap – more details of the actual hole a little later, and if you’re using the utility already you should consider updating to get this fix.

I’d love to say that it was a piece of inspired insight on my part, but I think there’s more value in the truth – that it was a series of happy co-incidences.

Collaborating on the GuardedCommandMap

The SignalCommandMap allows you to pair a Command Class to an instance or Class of a Signal. When the Signal dispatches, the values dispatched are available for injection into the Command, which is then executed.

On reading about the GuardedCommandMap, Neil Manuell asked about a GuardedSignalCommandMap implementation. It’s something I’d been meaning to work on for a while, but hadn’t found the time. As it happens, we both started working on it simultaneously, but from opposite ends – he was hacking the code and, as is my habit, I began by writing tests. So, Neil sent me a pull request on github:

Its basically a cut and paste job, from your’s and Joel’s code, the only additional thing that I have done is to slightly restructure the SignalCommandMap so that we can share code.

no tests have been broken :)

I also implemented the mapGuardedSignalClass method, so tests will be need for that too.

fingers crossed

I pulled his code (github is amazing isn’t it?) and used my normal process for retrospectively adding tests; I put early returns in the class methods under test, to confirm that the code is what’s causing them to pass.

Some tests passed, but I quickly ran into InjectorErrors – the injector was complaining that it couldn’t unmap a mapping that didn’t exist.

In the Injector, mapping refers to informing the injector of a value to use for a specific type (eg Number, String, SomeVO) of injection point. When the mapping is ‘unmapped’ the injector no longer has a value available to inject against this type.

Normally I would be adding a bunch of traces at this point, but I’m a project-sprouts user and this was a Flash Builder project with some ant tasks. I don’t have Flash Builder and I could only get the FlexUnit tests to run as a headless build, and then check the test output. I couldn’t seem to pick up the trace output using fdb or any of my normal tools, so I pushed on without traces. This was important because it forced me to use tests rather than debugging to explore the error, and as a result I went down a different set of mental, and programming, paths.

The error was being thrown in an unmapSignalValues function that Neil had broken out in refactoring the SignalCommandMap so that the two maps could share code.  The SignalCommandMap tests were running without error, so I assumed it was something in the Guarded implementation causing the problems.

I did a quick project-wide search for the unmapSignalValues method being used anywhere – hoping to play spot-the-difference – but I couldn’t find it. In fact, I couldn’t find anything in the SignalCommandMap related to unmapping the values between signal firings. But that made no sense – surely it was using something so clever that I couldn’t see it?

So I added a test that was designed to fail (to throw an error to be precise):

The test involved a Command that required injector types that were dispatched by Signal1, but the Command was mapped to Signal2. I was expecting a “Don’t know how to fulfil those injections!” error when the command was created, but I found that as long as I’d fired Signal1 first, there was no error.

Peering down the hole

At first this doesn’t sound too serious – does it really matter if values persist between one Signal dispatch and the next?

Here is the potential problem:

  1. Assume we have a command with 2 injected dependencies A and B.
  2. Map a signalA that dispatches A to another command.
  3. Map a signalB that dispatches B to this command (the one with dependencies A and B).
  4. Dispatch signalA, followed by signalB – and the dependencies will be met using the two values.
  5. Dispatch signalB before signalA – and you’ll get a dependency injection error. In itself this isn’t too hard to diagnose.

The really hard to debug problem would arise if you had another signal, which also dispatched a value of the same type as signalA, and which fired between the two. If the value was a simple type – such as a Number – you’d find it very hard to pick this up. You can’t detect this with unit tests, only with end-to-end tests of complete paths through the software, and if you have user interaction it’s virtually impossible to get complete coverage for this.

If the purpose of the Number in the Command was to represent a user’s quiz scores in an e-learning app, with this data then being submitted to a remote service, you could be misrepresenting a user’s scores to their boss or teacher. If the difference was marginal (say 80 instead of 70) the error might never come to light.

You might argue that this is a ‘corner case’. The thing about corner cases is that if you’ve thought of them (and often we use this term when we realise we messed up by not thinking of something), and you’ve genuinely weighed up the pros and cons and decided to ignore them, you should close off the path. If it’s not safe to go into the corner, don’t let me go there!

As a general rule, we should aim to have order-of-execution variations do only two things:

a. work
b. explode

Any other variation – ‘kinda-sort-of works but the data gets messed up’ – should be avoided. Yeah, this stuff is hard to get right – nobody ever told you that being a developer was going to be easy, did they?

Confirming the fix

A quick check with Till and Joel, and we confirmed that yes, the dispatched values were supposed to be unmapped between firings. The SignalCommandMap should not support situations where you’re relying on values that weren’t dispatched in this Signal. This was the case in the first release of the SignalCommandMap, but successive revisions had silently created the hole. Existing tests still passed, but we hadn’t tested for what the SignalCommandMap shouldn’t do.

If you are currently using the SignalCommandMap you should check whether your code relies on this hole – all Commands should only require Injections provided by the main Injector (models, services etc), or by the Signal that was just fired.  If you switch to the new code and something explodes, great! You just found a brittle point in your code that could have led to invisible bugs down the line.

Easy – just add the unmapping code… er…

Which is when the SignalCommandMap test-suite exploded. Puking the same ‘I can’t unmap something I haven’t mapped!’ error all over the place that had originally led to me finding the hole.

So I looked at the unmapping code that Neil had added in a little more detail.

Seems sensible enough. The problem is that mapValue and unmap aren’t actually symmetrical:

In most situations, if you try to pass an argument of the wrong type you get a nice compiler error. However, when the argument is a String, and the value is coming out of an Array (or any other ambiguous type reference), it will be coerced to String instead.

So – the compiler lets it pass, and the bug most likely shows up as one of the following:

  1. A weird value in a textfield, file or data where you output the String.
  2. A null pointer error, where the String is used as a reference.
  3. Data overwriting errors – because if you’re using this value as a key in a Dictionary (or Object), objects of the same type will coerce to the same String and map to the same entry in the Dictionary.
  4. An incorrect retrieval error – as a result of a combination of 2 and 3 – you get *some* data but it’s not the data you were expecting.
  5. A strongly typed, informative error because you’ve defended against a null pointer error / overwriting etc.

Fortunately in this case the SwiftSuspenders Injector gives us number 5 – and actually if I’d read the error in more detail I might have figured it out sooner, but then I would have missed the fact that the SignalCommandMap wasn’t unmapping values.

Four lessons from this episode:

1. Don’t use Strings for anything other than actual Text values if you can help it.

Strings are the worst kind of weak magic. While robotlegs includes the ability to make named injections we recommend that you avoid them any time it’s possible. I know that Till hates the named injections!

String Code is where Dragons like to hang out

String Code is where Dragons like to hang out

2. Aim for the impossible – complete test coverage

It’s probably impossible to completely capture the set of things your Class shouldn’t do, but if we at least make this a regular part of our TDD routine then we’ve got a hope of catching some of these hard-to-debug holes. We should aim to find the Oatmeal we didn’t eat, and not worry too much about the Hippopotamus.

Finding holes is hard

Keep the Oatmeal your code shouldn't eat in mind

3. Debugging gives you data – tests give you information

Instead of concerning ourselves with code (debugging) we should favour concerning ourselves with the behaviour of our code (through tests) – thanks to Neil for drawing this one out in our discussions about what happened. The tests for behaviour will still be relevant even when the code is refactored.

Favour behaviour tests over debugging

Favour behaviour tests over debugging

4. Collaboration pays dividends

Any time you get an opportunity to have someone other than the programmer specify the tests (not necessarily code them, but list them out) – take it. Between us, Joel, Till, Neil and myself have a smaller shared set of assumptions than any of us hold individually.

Collaboration leads to fewer assumptions

The size of your ass *does* matter

Finally…

The GuardedSignalCommandMap is wicked. Get it while it’s still hot!

(Read the maps, maps and more maps post for detail on what it does.)

About the Author

I'm an actionscript programmer living and working in a tiny village in the Yorkshire Dales, UK. I used to be a TV reporter, but my inner (and often outer) geek won. I also write stuff. Most recently Head First 2D Geometry.

Visit Stray's Website

Share the post

Delicious It Digg this! Stumble this! Share on Reddit Share on Buzz Share on FriendFeed
  • http://www.newtriks.com/ newtriks

    You know what baffles me the most….? How anyone can put Flora on their toast instead of butter!
    :) nice post as always Stray!

    • http://www.xxcoder.net Stray

      Thanks :)

      As for the Flora: http://en.wikipedia.org/wiki/Familial_hypercholesterolemia

      My Dad gave me a lot of great things in life, but never being able to eat butter again was definitely something I could have done without. Luckily I have the Heterozygous kind – he has the full version so witnessing his triple bypass at 50 was great motivation for me to stick to my diet!

      • http://www.xxcoder.net Stray

        Oh… and incase that sounded remotely ‘poor me’ – I consider myself incredibly lucky to be born at a time when we have the information available to make the choices that mean that I don’t have to spark out in my 50s with no warning!

        • http://www.newtriks.com/ newtriks

          Ahhh (takes foot out of mouth), well I wasn’t thinking it was a poor me story tbh :) I know I should chill on the butter but I am a Devonshire boy so that kinda thing is hard :) .

  • http://twitter.com/stiggler Patrick Mowrer

    Very nice walkthrough of your thought process! Coincidentally, I desperately needed a GuardedSignalCommandMap to (elegantly) use existing code for a side-project Sunday. I found Neil’s fork on Github and used that, running into the same problem you outlined above and made the simple fix as a make-shift. I’m happy to be able to download the fixed source now. Thanks for your work on this!

    • http://www.xxcoder.net Stray

      Great to know it’s useful to people Patrick – thanks for stopping by with encouragement – it’ll definitely be worth pulling the updated fork.

  • Anonymous

    Just to clarify Stray, would you consider:

    public const MY_VALUE:String =”hello”;

    to be a safe use of a string?

    • http://www.xxcoder.net Stray

      Hi Neil,

      Good question!

      As was the topic of our discussion yesterday – the important thing is the behaviour, and not the implementation detail. So, it depends on what the purpose of the String is. What would happen if I accidentally used something else in place of MY_VALUE?

      If it’s an error message, user advice etc – it’s a safe use. The worst case scenario is a nonsensical message.

      If it’s being used to determine a behavioural pathway through the code, the problem is not the way you hold the value, but the function that expects it.

      This was a big part of the reason for Robert Penner creating as3 Signals – the constructor expects a String, and your architecture hopes it’s one of the static const event types, but you can feed it anything you like, and then you’ll get silent bugs in your code. Nothing explodes, something just doesn’t happen.

      Using const mitigates the risk of typos, and keeps your code DRY, but it doesn’t close off silent fails unless you also verify against the set of valid consts you were expecting and then throw an error if it doesn’t match what you expected.

      That said, we are somewhat stuck with the Event system in AS3, and I have only partially moved over to Signals. The good news about Events and robotlegs is that it’s possible to get very good test coverage of this by listening and dispatching on the shared eventDispatcher.

      Stray

      • Anonymous

        OK, I ask for a reason.

        I’m “changing some shit” in my StateMachine util after some observations by @DavidArno (bless) gave me a sudden perspective change (or shrunk my assumption set, making my ass-hole smaller).

        I’ve been using named injections to inject the Sates – I didn’t like it, but I couldn’t see another way. Of course the answer is obvious – inject the state model instead and interrogate it:

        stateModel.getState( STARTING )

        And as long as it chucks an error we are covered.

        or is there a better way that is not reliant on a String uid?

        • http://www.xxcoder.net Stray

          Good question. I just had a little browse around the old SM code on github, but I’d love to see what you’re changing. I personally don’t use a generic string-driven state machine – I prefer to customise it for each project and then I can use properties. But my workflow is big projects rather than little projects, so I can see how that could get old quickly!

          I definitely think doing away with the named injection would be a plus. But I’d love to know whether there is a way to do the whole thing without Strings… not sure what that would be – except maybe a wrapper class that provides a leaf/node access structure, and sets up the internal structure as well. Would be interesting to work out what was viable. It might turn out to be so inelegant that it’s not a win over Strings, but it’d be a good exercise.

          I’d love to know the current set of assumptions…

          • Anonymous

            I’ve actually already rewritten the SM and removed all dependencies. That allows it to be extended from diff frameworks and Observer patterns. These latest changes are I guess are a second tier as my assumptions have come from the way the pureMVC util was structured.
            I use a Signals version of the code base, which removes all the strings from the transition hooks. That still leaves the retrieval of the States to a string uid, which is acceptable.

          • http://www.xxcoder.net Stray

            Fantastic – will that be on github?

  • http://openid.blogs.es/zarate Zarate

    Hi there!

    There seems to be some problem with your RSS, throwing some PHP errors,

    Cheers!

    • http://www.xxcoder.net Stray

      Weird – I wonder if it was related to an earlier server outage? If it’s still happening, can you send me some detail? I can’t recreate it here now.

  • Oliver

    Thank you for the fabulous insight!