/testing

Add a panic() if AddCleanup is unsafe. #92

Merged
merged 5 commits into from Mar 17, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jameinel @axw @davecheney
@jameinel
Owner

jameinel commented Mar 17, 2016

If you call AddCleanup from a Test that uses a non-pointer receiver, then the cleanup
will actually never be called, because the original suite is no longer around.

Also, make AddCleanup automatically act like AddSuiteCleanup if it is called while we aren't inside a Test context. That way thinks like PatchValue actually work like we think they do, rather than having them just not work at all.
@jameinel
Help people be safe with their Cleanup usage by adding panic() for un… 
…safe behavior.

If you call AddCleanup from a Test that uses a non-pointer receiver, then the cleanup
will actually never be called, because the original suite is no longer around.
1efa135

@jameinel jameinel changed the title from Add a panic() if AddCleanup usage by adding panic() for un… to Add a panic() if AddCleanup is unsafe. Mar 17, 2016

@axw
Member

axw commented Mar 17, 2016

LGTM
@davecheney
Contributor

davecheney commented Mar 17, 2016

LGTM. Nice catch!

On Thu, Mar 17, 2016 at 4:23 PM, Andrew Wilkins notifications@github.com
wrote:

LGTM

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#92 (comment)

jameinel added some commits Mar 17, 2016

@jameinel
catch a few more unsafe ways to use AddCleanup and AddSuiteCleanup.
0bf044b
@jameinel
Make AddCleanup safe to call at any time, but give it the actual righ… 
…t lifetime.

We use the check if SetUpTest() has been called, to know if we should call the cleanup during
TearDownTest or during TearDownSuite().
34606df
cleanup.go
+ s.suiteStack = append(s.suiteStack, cleanup)
+ return
+ }
+ if s != s.testSuite {
@axw

axw Mar 17, 2016

Member

I think this one can go. s.testSuite is always nil, or equal to s.suiteStuite. If anything else were true, then either this or the s != s.suiteSuite test would panic.

@jameinel

jameinel Mar 17, 2016

Owner

Confirmed. I removed it and the test suite still passed which should catch this sort of thing.

home_test.go
+ s.fakeHomeSuite.TearDownSuite(c)
+ s.IsolationSuite.TearDownSuite(c)
+}
+
func (s *fakeHomeSuite) TestHomeCreated(c *gc.C) {
// A fake home is created and set.
s.fakeHomeSuite.SetUpTest(c)
@axw

axw Mar 17, 2016

Member

should this one be moved up into SetUpTest above?

@jameinel

jameinel Mar 17, 2016

Owner

We only create the fakeHomeSuite one time in SetUpSuite() thus we should call fakeHomeSuite.SetUpSuite() there, and then tear it down during TearDownSuite(), IMO.

The tests themselves are just calling SetUpTest() and TearDownTest()

@axw
Member

axw commented Mar 17, 2016

LGTM
@jameinel
Add more checking of what state we think our CleanupSuite is in. 
Hopefully this also gives more useful error messages, so people can understand
what they're doing wrong.
AddCleanup can be called:
  After CleanupSuite.SetUpSuite() (during the test's own SetUpSuite())
  before the first SetUpTest(), which means we'll add a Suite level cleanup.
  After CleanupSuite.SetUpTest() which means we'll create a Test level cleanup.
  Not after TearDownTest(), because that would imply a suite-level cleanup that
  only applied to some tests.
071a376

jameinel added a commit that referenced this pull request Mar 17, 2016

@jameinel
Merge pull request #92 from jameinel/unsafe-add-cleanup 
Add a panic() if AddCleanup is unsafe.

Also, make it so AddCleanup will create a Suite level cleanup if it is called before we start a Test. Disallow calling AddCleanup after TearDownTest has been called before the next SetUpTest is started.
45f216f

@jameinel jameinel merged commit 45f216f into juju:master Mar 17, 2016

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