On 17 March 2016 at 04:52, John Meinel <j...@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@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
