PatchValue (AddCleanup) unsafe with non pointer receiver test

[John Meinel]

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 <john at arbash-meinel.com> 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 <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, 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
>> =:->
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160317/9f33a083/attachment.html>