Code review comment for lp:~thumper/juju-core/refactor-uniter-tests

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with some tweaks

https://codereview.appspot.com/41210043/diff/1/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/context.go#newcode191
worker/uniter/context.go:191: writeChanges := err == nil
Why not just make writeChanges a method param?
And set it accordingly in the caller if the err the caller has is nil?

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter.go#newcode35
worker/uniter/uniter.go:35: type UniterExecutionObserver interface {
Doc string please

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (left):

https://codereview.appspot.com/41210043/diff/1/worker/uniter/uniter_test.go#oldcode192
worker/uniter/uniter_test.go:192: func (ctx *context) matchLogHooks(c
*gc.C) (match bool, overshoot bool) {
Is this still the best method name?

https://codereview.appspot.com/41210043/

« Back to merge proposal