So the reason I thought this fix wouldn't work is because I had a failure in BaseSuite.SetUpSuite(). It turns out, the failure was actually valid. SetUpSuite is doing: s.PatchValue(&utils.OutgoingAccessAllowed, false)
However, PatchValue calls AddCleanup, and AddCleanup cleans up during TearDownTest. Which means that OutgoingAccessAllowed is thought to be set to false for the duration of the *suite* but actually as soon as the first test finishes, it gets set back to the original value. I've got a patch up for our testing infrastructure, we'll see how deep the rabbit goes to actually use that in Juju. John =:-> On Thu, Mar 17, 2016 at 9:11 AM, John Meinel <[email protected]> wrote: > https://github.com/juju/testing/pull/92 is a possible "fix" for this. It > just panic() if it notices that the suite that was seen in SetUpSuite() is > different than the one seen in AddSuiteCleanup(), and similarly for SetUp() > and AddCleanup() > > A different possibility would be to ignore "s" and only use the original > pointer? Would that be a better fix? > > John > =:-> > > > On Thu, Mar 17, 2016 at 8:52 AM, John Meinel <[email protected]> > 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, 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 [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
