Code review comment for lp:~thumper/juju-core/user-display-name

New branch is moving to github.

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go#newcode42
state/api/usermanager/client_test.go:42: err :=
s.APIState.Call("UserManager", "", "AddUser", userArgs, results)
On 2014/06/03 05:17:26, menn0 wrote:
> Maybe add a comment here about why s.APIState.Call("UserManager",...)
is used
> here instead of s.usermanager.AddUser()? I understand why but had to
think about
> it for a bit.

Done.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go#oldcode39
state/apiserver/charms_test.go:39: password, err :=
utils.RandomPassword()
On 2014/06/03 05:17:26, menn0 wrote:
> Play devil's advocate: this change means the user has a fixed password
instead
> of a random one. Are you sure that doesn't negatively impact any
existing tests?
> I can't think of why it would but it is a test functionality change.

Sure, but the tests all still pass :-)

I'm considering a way to change the way we create test fixture objects.
A cunning plan.

https://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode75
state/user_test.go:75: user := s.makeUser(c)
On 2014/06/03 21:29:26, waigani wrote:
> It seems you are making a user for every test. Why not makeUser in
SetUpTest?

Not quite every test :-)

https://codereview.appspot.com/102970043/