PatchValue (AddCleanup) unsafe with non pointer receiver test

[roger peppe]

On 17 March 2016 at 04:52, John Meinel <john at arbash-meinel.com> wrote:
> I came across this in the LXD test suite today, which was hard to track
> down, so I figured I'd let everyone know about it.
>
> We have a nice helper in testing.IsolationSuite with "PatchValue()" that
> will change a global for you during the test, and then during TearDown()
> will cleanup the patch it made.
> It turns out that if your test doesn't have a pointer receiver this fails,

This is your problem. You should *always* write tests on the pointer
to the suite, and it's a bug if you don't. It's perfectly OK for helper
suites (aside: I wish we called them "fixtures") to have mutable state,
and that can't work if you define methods on the value type.

We actually already have a tool that can catch this bug - the -copylocks
flag to go vet can check that sync.Mutex is not passed by value,
so if we put a Mutex inside CleanupSuite, then go vet will complain about
programs like this:

        package main

        import (
                "github.com/juju/testing"
                gc "gopkg.in/check.v1"
        )

        type X struct {
                testing.IsolationSuite
        }

        func main() {
        }

        func (x X) TestFoo(c *gc.C) {
        }

with a message like this:

    /home/rog/src/tst.go:15: TestFoo passes lock by value: main.X
contains github.com/juju/testing.IsolationSuite contains
github.com/juju/testing.CleanupSuite contains sync.Mutex

> because the "suite" object is a copy, so when PatchValue calls AddCleanup to
> do s.testStack = append(...) the suite object goes away before TearDown is
> called.
>
> You can see this with the attached test suite.
>
> Example:
>
> func (s mySuite) TestFoo(c *gc.C) {
>  // This is unsafe because s.PatchValue ends up modifying s.testStack but
> that attribute only exists
>  // for the life of the TestFoo function
>   s.PatchValue(&globalVar, "newvalue")
> }
>
> I tried adding the attached patch so that we catch places we are using
> AddCleanup unsafely, but it fails a few tests I wasn't expecting, so I'm not
> sure if I'm actually doing the right thing.
>
> John
> =:->
>
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>