Code review comment for lp:~benji/juju-core/1130173

Revision history for this message
William Reade (fwereade) wrote :

Code looks fine, but please fix the tests.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go
File state/statecmd/resolved_test.go (right):

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode18
state/statecmd/resolved_test.go:18: repo *charm.LocalRepository
What are conn and repo for?

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode21
state/statecmd/resolved_test.go:21: // Ensure our test suite satisfies
Suite
This is not correct: Suite is a function, and the purpose of the line is
to register the suite for running by gocheck.

The following construction (as snarfed from worker/deployer/simple.go):

var _ Context = (*SimpleContext)(nil)

...is the one that ensures that a pointer to a SimpleContext satisfies
the Context interface (and does so at compile time).

It seems there are several places this misunderstanding is propagated in
this package; please fix those too.

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode24
state/statecmd/resolved_test.go:24: func panicWrite(name string, cert,
key []byte) error {
As far as I can tell, the code from here...

https://codereview.appspot.com/7460047/diff/1/state/statecmd/resolved_test.go#newcode55
state/statecmd/resolved_test.go:55: c.Assert(err, IsNil)
...to here could be replaced with a single call to AddTestingCharm. What
am I missing?

https://codereview.appspot.com/7460047/

« Back to merge proposal