Code review comment for lp:~thumper/juju-core/cleanup-suite

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with a couple of minor suggestions.

Embedding this into LoggingSuite makes me
feel twitchy though. This a marriage entirely
of convenience - LoggingSuite previously
was about a single thing only, and now has
this functionality shoehorned in just because
lots of things use LoggingSuite.

I would be more inclined to embed it in some
of the "kitchen-sink" suites: JujuConnSuite;
or rename LoggingSuite to LoggingAndCleanupSuite
perhaps throughout.

I like the functionality in general though.

https://codereview.appspot.com/13633045/diff/1/testing/cleanup.go
File testing/cleanup.go (right):

https://codereview.appspot.com/13633045/diff/1/testing/cleanup.go#newcode14
testing/cleanup.go:14: testStack []func()
One possibility is to use jc.Restorer instead of func() here, which
perhaps makes the purpose of the functions more clear.

It doesn't really make any difference, just a thought.

https://codereview.appspot.com/13633045/diff/1/testing/cleanup_test.go
File testing/cleanup_test.go (right):

https://codereview.appspot.com/13633045/diff/1/testing/cleanup_test.go#newcode10
testing/cleanup_test.go:10: testing.CleanupSuite
I'm not sure we need to embed this - you can just
declare it in each test, which I think will make the
tests a little clearer as it avoids shared state.

https://codereview.appspot.com/13633045/

« Back to merge proposal