Improve our test isolation #4798

Merged
merged 31 commits into from Mar 21, 2016

Conversation

Projects
None yet
2 participants
Owner

jameinel commented Mar 19, 2016

We make heavy use of PatchValue in our test suite. But the existing code actually had several failure modes.

  1. SetUpSuite(){PatchValue()} would actually add the Cleanup call to the first TearDownTest, which means that it only held true for one test.
  2. Test suites that would call PatchValue, but not properly call TearDown, would leave values patched permanently. This includes cases where a test was written without a "pointer" receiver. (func (f Foo) TestBar(c *gc.C)). That actually copies the stack of things to be cleaned up, and mutates the copy, and when the TestBar function returns, the stack resets to its original value.
  3. Test suites would call PatchValue, and then call CleanupSuite.SetUpTest(). What they didn't realize is that SetUpTest() resets the list of things to be cleaned up to 'nil'. Which meant we would never run any of the cleanups.

I submitted a patch to github.com/juju/testing that changed PatchValue to be safer, and refuse to patch when it looks like we weren't doing what you want. This change against Juju Core brings in the updated testing package, and fixes all of the places that failed because they weren't actually setting themselves up correctly.

The safety checks also mean that you have to call CleanupSuite.SetUpTest() before you call PatchValue for that test. Which required moving some PatchValues around. Sometimes moving things from SetUpTest to SetUpSuite if they needed to be patched before Base.SetUpTest() is called. (we have a few places that need to inject Clock or something like that, into the service that is started in SetUpTest.)

Everything passes on my Trusty box, but I can't be sure it will pass on all platforms.

(Review request: http://reviews.vapour.ws/r/4240/)

jameinel added some commits Mar 17, 2016

Initial work on fixing our test suite to actually have valid lifetime…
…s for PatchValue objects.

We have a bunch of tests that thought they were patching things out and could trust them
to be correct, but they didn't realize the PatchValue lifetime was not what they thought
it was. So they were actually testing the wrong thing.
Or leaving global state accidentally dirty and the other tests then are using the wrong
data.
Our BaseSuite was guilty of this, which means lots of things could be wrong.
If your base FS is btrfs, then the tests would give the wrong answers.
It might be better to patch ContainerDirFilesystem as then we could test all
the logic on all platforms, but that's a lot more work.
a couple more tweaks.
One small problem that still remains is teardown takes 60s
to stop trying to remove the security group.
Merge remote-tracking branch 'upstream/master' into unsafe-cleanups
Update the various changes for the version.Current =>
jujuversion.Current changes.

Conflicts:
	provider/ec2/ebs_test.go
	provider/ec2/live_test.go
	provider/ec2/local_test.go
	provider/joyent/local_test.go
signedSuite was calling PatchValue but never calling TearDownSuite.
This is where we leaked setting SignedMetadataPublicKey so all other tests always
had it set to the testing key.
Found a missing PatchValue.
Still are missing a call to ToolsFixture.SetUpTest for localServerSuite.
We have to have the clock before calling ConnSuite.SetUpTest,
but we can patch the GetClock function during SetUpSuite.
Merge remote-tracking branch 'upstream/master' into unsafe-cleanups
Clean up a couple of conflicts, did a bunch of testing, looks like we're still
clean.

Conflicts:
	cmd/juju/service/bundle_test.go
	dependencies.tsv
Owner

jameinel commented Mar 21, 2016

$$merge$$

Contributor

jujubot commented Mar 21, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Mar 21, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/6962

Owner

jameinel commented Mar 21, 2016

$$merge$$ looks like someone landed a new test suite that didn't call BaseSuite.

Contributor

jujubot commented Mar 21, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Mar 21, 2016

Merge pull request #4798 from jameinel/unsafe-cleanups
Improve our test isolation

We make heavy use of PatchValue in our test suite. But the existing code actually had several failure modes.
1) SetUpSuite(){PatchValue()} would actually add the Cleanup call to the first TearDownTest, which means that it only held true for one test.
2) Test suites that would call PatchValue, but not properly call TearDown, would leave values patched permanently. This includes cases where a test was written without a "pointer" receiver. (func (f Foo) TestBar(c *gc.C)). That actually copies the stack of things to be cleaned up, and mutates the copy, and when the TestBar function returns, the stack resets to its original value.
3) Test suites would call PatchValue, and then call CleanupSuite.SetUpTest(). What they didn't realize is that SetUpTest() resets the list of things to be cleaned up to 'nil'. Which meant we would never run any of the cleanups.

I submitted a patch to github.com/juju/testing that changed PatchValue to be safer, and refuse to patch when it looks like we weren't doing what you want. This change against Juju Core brings in the updated testing package, and fixes all of the places that failed because they weren't actually setting themselves up correctly.

The safety checks also mean that you have to call CleanupSuite.SetUpTest() before you call PatchValue for that test. Which required moving some PatchValues around. Sometimes moving things from SetUpTest to SetUpSuite if they needed to be patched before Base.SetUpTest() is called. (we have a few places that need to inject Clock or something like that, into the service that is started in SetUpTest.)

Everything passes on my Trusty box, but I can't be sure it will pass on all platforms.


(Review request: http://reviews.vapour.ws/r/4240/)

@jujubot jujubot merged commit 3e3f19d into juju:master Mar 21, 2016

@jameinel jameinel deleted the jameinel:unsafe-cleanups branch Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment