Monthly Archives: December 2019

Dynamically scoped variables in Go

This is a thought experiment in API design. It starts with the classic Go unit testing idiom:

func TestOpenFile(t *testing.T) {
        f, err := os.Open("notfound")
        if err != nil {
                t.Fatal(err)
        }

        // ...
}

What’s the problem with this code? The assertion. if err != nil { ... } is repetitive and in the case where multiple conditions need to be checked, somewhat error prone if the author of the test uses t.Error not t.Fatal, eg:

        f, err := os.Open("notfound")
        if err != nil {
                t.Error(err)
        }
        f.Close() // boom!

What’s the solution? DRY it up, of course, by moving the repetitive assertion logic to a helper:

func TestOpenFile(t *testing.T) {
        f, err := os.Open("notfound")
        check(t, err)

        // ...
}
 
func check(t *testing.T, err error) {
       if err != nil {
                t.Helper()
                t.Fatal(err)
        }
}

Using the check helper the code is a little cleaner, and clearer, check the error, and hopefully the indecision between t.Error and t.Fatal has been solved. The downside of abstracting the assertion to a helper function is now you need to pass a testing.T into each and every invocation. Worse, you need to pass a *testing.T to everything that needs to call check, transitively, just in case.

This is ok, I guess, but I will make the observation that the t variable is only needed when the assertion fails — and even in a testing scenario, most of the time, most of the tests pass, so that means reading, and writing, all these t‘s is a constant overhead for the relatively rare occasion that a test fails.

What about if we did something like this instead?

func TestOpenFile(t *testing.T) {
        f, err := os.Open("notfound")
        check(err)
 
        // ...
}
 
func check(err error) {
        if err != nil {
                panic(err.Error())
        }
}

Yeah, that’ll work, but it has a few problems

% go test
--- FAIL: TestOpenFile (0.00s)
panic: open notfound: no such file or directory [recovered]
        panic: open notfound: no such file or directory

goroutine 22 [running]:
testing.tRunner.func1(0xc0000b4400)
        /Users/dfc/go/src/testing/testing.go:874 +0x3a3
panic(0x111b040, 0xc0000866f0)
        /Users/dfc/go/src/runtime/panic.go:679 +0x1b2
github.com/pkg/expect_test.check(...)
        /Users/dfc/src/github.com/pkg/expect/expect_test.go:18
github.com/pkg/expect_test.TestOpenFile(0xc0000b4400)
        /Users/dfc/src/github.com/pkg/expect/expect_test.go:10 +0xa1
testing.tRunner(0xc0000b4400, 0x115ac90)
        /Users/dfc/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
        /Users/dfc/go/src/testing/testing.go:960 +0x350
exit status 2

Let’s start with the good; we didn’t have to pass a testing.T every place we call check, the test fails immediately, and we get a nice message in the panic — albeit twice. But where the assertion failed is hard to see. It occurred on expect_test.go:11 but you’d be forgiven for not knowing that.

So panic isn’t really a good solution, but there’s something in this stack trace that is — can you see it? Here’s a hint, github.com/pkg/expect_test.TestOpenFile(0xc0000b4400).

TestOpenFile has a t value, it was passed to it by tRunner, so there’s a testing.T in memory at address 0xc0000b4400. What if we could get access to that t inside check? Then we could use it to call t.Helper and t.Fatal. Is that possible?

Dynamic scoping

What we want is to be able to access a variable whose declaration is neither global, or local to the function, but somewhere higher in the call stack. This is called dynamic scoping. Go doesn’t support dynamic scoping, but it turns out, for restricted cases, we can fake it. I’ll skip to the chase:

// getT returns the address of the testing.T passed to testing.tRunner
// which called the function which called getT. If testing.tRunner cannot
// be located in the stack, say if getT is not called from the main test
// goroutine, getT returns nil.
func getT() *testing.T {
        var buf [8192]byte
        n := runtime.Stack(buf[:], false)
        sc := bufio.NewScanner(bytes.NewReader(buf[:n]))
        for sc.Scan() {
                var p uintptr
                n, _ := fmt.Sscanf(sc.Text(), "testing.tRunner(%v", &p)
                if n != 1 {
                        continue
                }
                return (*testing.T)(unsafe.Pointer(p))
        }
        return nil
}

We know that each Test is called by the testing package in its own goroutine (see the stack trace above). The testing package launches the test via a function called tRunner which takes a *testing.T and a func(*testing.T) to invoke. Thus we grab a stack trace of the current goroutine, scan through it for the line beginning with testing.tRunner — which can only be the testing package as tRunner is a private function — and parse the address of the first parameter, which is a pointer to a testing.T. With a little unsafe we convert the raw pointer back to a *testing.T and we’re done.

If the search fails then it is likely that getT wasn’t called from a Test. This is actually ok because the reason we needed the *testing.T was to call t.Fatal and the testing package already requires that t.Fatal be called from the main test goroutine.

import "github.com/pkg/expect"

func TestOpenFile(t *testing.T) {
        f, err := os.Open("notfound")
        expect.Nil(err)
 
        // ...
}

Putting it all together we’ve eliminated the assertion boilerplate and possibly made the expectation of the test a little clearer to read, after opening the file err is expected to be nil.

Is this fine?

At this point you should be asking, is this fine? And the answer is, no, this is not fine. You should be screaming internally at this point. But it’s probably worth introspecting those feelings of revulsion.

Apart from the inherent fragility of scrobbling around in a goroutine’s call stack, there are some serious design issues:

  1. The expect.Nil‘s behaviour now depends on who called it. Provided with the same arguments it may have different behaviour depending on where it appears in the call stack — this is unexpected.
  2. Taken to the extreme dynamic scoping effective brings into the scope of a single function all the variables passed into any function that preceded it. It is a side channel for passing data in to and out of functions that is not explicitly documented in function declaration.

Ironically these are precisely the critiques I have of context.Context. I’ll leave it to you to decide if they are justified.

A final word

This is a bad idea, no argument there. This is not a pattern you should ever use in production code. But, this isn’t production code, it’s a test, and perhaps there are different rules that apply to test code. After all, we use mocks, and stubs, and monkey patching, and type assertions, and reflection, and helper functions, and build flags, and global variables, all so we can test our code effectively. None of those, uh, hacks will ever show up in the production code path, so is it really the end of the world?

If you’ve read this far perhaps you’ll agree with me that as unconventional as this approach is, not having to pass a *testing.T into every function that could possibly need to assert something transitively, makes for clearer test code.

So maybe, in this case, the ends do justify the means.


If you’re interested, I’ve put together a small assertion library using this pattern. Caveat emptor.

Complementary engineering indicators

Last year I had the opportunity to watch Cat Swetel’s presentation The Development Metrics You Should Use (but Don’t). The information that could be gleaned from just tracking the start and finish date of work items was eye opening. If you’re using an issue tracker this information is probably already (perhaps with some light data munging) available — no need for TPS reports. Additionally, statistics obtained by data mining your project’s issue tracker are, perhaps, less likely to be juked.

Around the time I saw Cat’s presentation I finished reading Andy Grove’s High Output Management. The hidden gem in this book (assuming becoming a meeting powerhouse isn’t your bag) was Grove’s notion of indicator pairs. An example of a paired indicator might be the number of sales deals closed paired with the customer retention rate. The underling principle being optimising for one indicator will have an adverse impact on the other. In the example, overly aggressive or deceptive tactics could superficially raise the number of sales made, but would be reflected in a dip in the retention rate as customers returned the product or terminated their service prematurely.

These ideas lead me to thinking about indicators you could use for a team delivering a software product. Could those indicators be derived cheaply from the hand to hand combat of software delivery? Could they be structured in a way that aggressively pursuing one metric would be reflected negatively in another? I think so.

These are the three metrics that I’ve been using to track the health of the project that I lead.

  • Date; was the software done when we said it would be done. If you prefer this indicator as a scalar, how many days difference is there between the ship date agreed on at the start of the sprint/milestone/whatever and what was the actual date that you considered it done.
  • Completeness; when the software is done, how many of the things we said we’re going to do actually got delivered in that release.
  • Defects reported; once the software is in the field, what is the rate of bugs reported.

It is relatively easy, for example, to hit a delivery date if you aggressively descope anything risky or simply don’t do it. But in doing so this lack of promised functionality would impact the completeness metric.

Conversely, it’s straight forward to hit your milestone’s completeness target if you let the release date slip and slip. Bringing both the metics into line requires good estimation skills to judge how much can be attempted in milestone and provide direct feedback if your estimation skills needed work.

The third indicator, defects reported in the field, acts as a check on the other two. It would be easy to consistent hit your delivery date with 100% feature completion if your team does a shoddy job. The high fives and :tada: emojis will be short lived if each release brings with it a swathe of high priority bug reports. This indicator also tends to have a second order effect, rushed features to meet a deadline tend to generate remedial work in the following milestones, crowding out promised work or blowing later deadlines.

I consider these to be complementary metrics, they should be considered together, as a group, rather than individually. Ideally your team should be delivering what you promised, when you promised it, with a low defect rate. But more importantly, if that isn’t the case, if one of the indicators is unhealthy, addressing it shouldn’t result in the problem moving to another.