Merge lp:~thumper/juju-core/juju-run-cli into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add the "run" command to the command line tool.

Hook up the Client Run and RunOnAllMachines API calls
to the command line tool.

https://codereview.appspot.com/50310043/

Add the "run" command to the command line tool.

Hook up the Client Run and RunOnAllMachines API calls
to the command line tool.

https://codereview.appspot.com/50310043/

[Tim Penhey (thumper)]

Reviewers: mp+201124_code.launchpad.net,

Message:
Please take a look.

Description:
Add the "run" command to the command line tool.

Hook up the Client Run and RunOnAllMachines API calls
to the command line tool.

https://code.launchpad.net/~thumper/juju-core/juju-run-cli/+merge/201124

Requires:
https://code.launchpad.net/~thumper/juju-core/api-juju-run/+merge/201103

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/50310043/

Affected files (+652, -0 lines):
   A [revision details]
   M cmd/juju/main.go
   M cmd/juju/main_test.go
   A cmd/juju/run.go
   A cmd/juju/run_test.go
   M testing/environ.go

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go
File cmd/juju/run.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode33
cmd/juju/run.go:33: Run the commands on the specified targets.
This just occurred to me: should we be supporting patterns, like in juju
status?
e.g. juju run --service='nova-*' <commands>

Just a thought.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode128
cmd/juju/run.go:128: var results = make([]interface{}, 0)
Why are you initialising this with 0 length, and then appending? Why not
initialise with len(runResults) and assign elements?

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode139
cmd/juju/run.go:139: values["Stdout"] = string(result.Stdout)
You're assuming that stdout/stderr are UTF-8 here, which they may not
be. At the least, can we please use utf8.Valid before converting them to
strings?

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode203
cmd/juju/run.go:203: type RunClient interface {
Nice

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go
File cmd/juju/run_test.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go#newcode210
cmd/juju/run_test.go:210: },
In light of my comments in run.go, I think a test for binary
stdout/stderr is in order.

https://codereview.appspot.com/50310043/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go
File cmd/juju/run.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode33
cmd/juju/run.go:33: Run the commands on the specified targets.
On 2014/01/10 02:13:53, axw wrote:
> This just occurred to me: should we be supporting patterns, like in
juju status?
> e.g. juju run --service='nova-*' <commands>

> Just a thought.

I do think that this is a good idea, but not for the first release :-)

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode128
cmd/juju/run.go:128: var results = make([]interface{}, 0)
On 2014/01/10 02:13:53, axw wrote:
> Why are you initialising this with 0 length, and then appending? Why
not
> initialise with len(runResults) and assign elements?

Hmm good point. Since we know the size, I'll do that.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode139
cmd/juju/run.go:139: values["Stdout"] = string(result.Stdout)
On 2014/01/10 02:13:53, axw wrote:
> You're assuming that stdout/stderr are UTF-8 here, which they may not
be. At the
> least, can we please use utf8.Valid before converting them to strings?

Now base64 encoding non-utf8 byte slice results.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go
File cmd/juju/run_test.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go#newcode210
cmd/juju/run_test.go:210: },
On 2014/01/10 02:13:53, axw wrote:
> In light of my comments in run.go, I think a test for binary
stdout/stderr is in
> order.

Added.

https://codereview.appspot.com/50310043/

[Andrew Wilkins (axwalk)]

On 2014/01/10 03:40:49, thumper wrote:
> Please take a look.

> https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go
> File cmd/juju/run.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode33
> cmd/juju/run.go:33: Run the commands on the specified targets.
> On 2014/01/10 02:13:53, axw wrote:
> > This just occurred to me: should we be supporting patterns, like in
juju
> status?
> > e.g. juju run --service='nova-*' <commands>
> >
> > Just a thought.

> I do think that this is a good idea, but not for the first release :-)

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode128
> cmd/juju/run.go:128: var results = make([]interface{}, 0)
> On 2014/01/10 02:13:53, axw wrote:
> > Why are you initialising this with 0 length, and then appending? Why
not
> > initialise with len(runResults) and assign elements?

> Hmm good point. Since we know the size, I'll do that.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode139
> cmd/juju/run.go:139: values["Stdout"] = string(result.Stdout)
> On 2014/01/10 02:13:53, axw wrote:
> > You're assuming that stdout/stderr are UTF-8 here, which they may
not be. At
> the
> > least, can we please use utf8.Valid before converting them to
strings?

> Now base64 encoding non-utf8 byte slice results.

> https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go
> File cmd/juju/run_test.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go#newcode210
> cmd/juju/run_test.go:210: },
> On 2014/01/10 02:13:53, axw wrote:
> > In light of my comments in run.go, I think a test for binary
stdout/stderr is
> in
> > order.

> Added.

Thanks, LGTM

https://codereview.appspot.com/50310043/

=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go	2013-12-11 07:55:37 +0000
+++ cmd/juju/main.go	2014-01-12 20:39:24 +0000
@@ -86,6 +86,7 @@
 	jujucmd.Register(wrap(&EndpointCommand{}))
 
 	// Error resolution and debugging commands.
+	jujucmd.Register(wrap(&RunCommand{}))
 	jujucmd.Register(wrap(&SCPCommand{}))
 	jujucmd.Register(wrap(&SSHCommand{}))
 	jujucmd.Register(wrap(&ResolvedCommand{}))
 
=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2013-12-11 07:55:37 +0000
+++ cmd/juju/main_test.go	2014-01-12 20:39:24 +0000
@@ -244,6 +244,7 @@
 	"remove-relation", // alias for destroy-relation
 	"remove-unit",     // alias for destroy-unit
 	"resolved",
+	"run",
 	"scp",
 	"set",
 	"set-constraints",
 
=== added file 'cmd/juju/run.go'
--- cmd/juju/run.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/run.go	2014-01-12 20:39:24 +0000
@@ -0,0 +1,233 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"encoding/base64"
+	"errors"
+	"fmt"
+	"strings"
+	"time"
+	"unicode/utf8"
+
+	"launchpad.net/gnuflag"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/juju"
+	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/state/api/params"
+)
+
+// RunCommand is responsible for running arbitrary commands on remote machines.
+type RunCommand struct {
+	cmd.EnvCommandBase
+	out      cmd.Output
+	all      bool
+	timeout  time.Duration
+	machines []string
+	services []string
+	units    []string
+	commands string
+}
+
+const runDoc = `
+Run the commands on the specified targets.
+
+Targets are specified using either machine ids, service names or unit
+names.  At least one target specifier is needed.
+
+Multiple values can be set for --machine, --service, and --unit by using
+comma separated values.
+
+If the target is a machine, the command is run as the "ubuntu" user on
+the remote machine.
+
+If the target is a service, the command is run on all units for that
+service. For example, if there was a service "mysql" and that service
+had two units, "mysql/0" and "mysql/1", then
+  --service mysql
+is equivalent to
+  --unit mysql/0,mysql/1
+
+Commands run for services or units are executed in a 'hook context' for
+the unit.
+
+--all is provided as a simple way to run the command on all the machines
+in the environment.  If you specify --all you cannot provide additional
+targets.
+
+`
+
+func (c *RunCommand) Info() *cmd.Info {
+	return &cmd.Info{
+		Name:    "run",
+		Args:    "<commands>",
+		Purpose: "run the commands on the remote targets specified",
+		Doc:     runDoc,
+	}
+}
+
+func (c *RunCommand) SetFlags(f *gnuflag.FlagSet) {
+	c.EnvCommandBase.SetFlags(f)
+	c.out.AddFlags(f, "smart", cmd.DefaultFormatters)
+	f.BoolVar(&c.all, "all", false, "run the commands on all the machines")
+	f.DurationVar(&c.timeout, "timeout", 5*time.Minute, "how long to wait before the remote command is considered to have failed")
+	f.Var(cmd.NewStringsValue(nil, &c.machines), "machine", "one or more machine ids")
+	f.Var(cmd.NewStringsValue(nil, &c.services), "service", "one or more service names")
+	f.Var(cmd.NewStringsValue(nil, &c.units), "unit", "one or more unit ids")
+}
+
+func (c *RunCommand) Init(args []string) error {
+	if len(args) == 0 {
+		return errors.New("no commands specified")
+	}
+	c.commands, args = args[0], args[1:]
+
+	if c.all {
+		if len(c.machines) != 0 {
+			return fmt.Errorf("You cannot specify --all and individual machines")
+		}
+		if len(c.services) != 0 {
+			return fmt.Errorf("You cannot specify --all and individual services")
+		}
+		if len(c.units) != 0 {
+			return fmt.Errorf("You cannot specify --all and individual units")
+		}
+	} else {
+		if len(c.machines) == 0 && len(c.services) == 0 && len(c.units) == 0 {
+			return fmt.Errorf("You must specify a target, either through --all, --machine, --service or --unit")
+		}
+	}
+
+	var nameErrors []string
+	for _, machineId := range c.machines {
+		if !names.IsMachine(machineId) {
+			nameErrors = append(nameErrors, fmt.Sprintf("  %q is not a valid machine id", machineId))
+		}
+	}
+	for _, service := range c.services {
+		if !names.IsService(service) {
+			nameErrors = append(nameErrors, fmt.Sprintf("  %q is not a valid service name", service))
+		}
+	}
+	for _, unit := range c.units {
+		if !names.IsUnit(unit) {
+			nameErrors = append(nameErrors, fmt.Sprintf("  %q is not a valid unit name", unit))
+		}
+	}
+	if len(nameErrors) > 0 {
+		return fmt.Errorf("The following run targets are not valid:\n%s",
+			strings.Join(nameErrors, "\n"))
+	}
+
+	return cmd.CheckEmpty(args)
+}
+
+func encodeBytes(input []byte) (value string, encoding string) {
+	if utf8.Valid(input) {
+		value = string(input)
+		encoding = "utf8"
+	} else {
+		value = base64.StdEncoding.EncodeToString(input)
+		encoding = "base64"
+	}
+	return value, encoding
+}
+
+func storeOutput(values map[string]interface{}, key string, input []byte) {
+	value, encoding := encodeBytes(input)
+	values[key] = value
+	if encoding != "utf8" {
+		values[key+".encoding"] = encoding
+	}
+}
+
+// ConvertRunResults takes the results from the api and creates a map
+// suitable for format converstion to YAML or JSON.
+func ConvertRunResults(runResults []params.RunResult) interface{} {
+	var results = make([]interface{}, len(runResults))
+
+	for i, result := range runResults {
+		// We always want to have a string for stdout, but only show stderr,
+		// code and error if they are there.
+		values := make(map[string]interface{})
+		values["MachineId"] = result.MachineId
+		if result.UnitId != "" {
+			values["UnitId"] = result.UnitId
+
+		}
+		storeOutput(values, "Stdout", result.Stdout)
+		if len(result.Stderr) > 0 {
+			storeOutput(values, "Stderr", result.Stderr)
+		}
+		if result.Code != 0 {
+			values["ReturnCode"] = result.Code
+		}
+		if result.Error != "" {
+			values["Error"] = result.Error
+		}
+		results[i] = values
+	}
+
+	return results
+}
+
+func (c *RunCommand) Run(ctx *cmd.Context) error {
+	client, err := getAPIClient(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+
+	var runResults []params.RunResult
+	if c.all {
+		runResults, err = client.RunOnAllMachines(c.commands, c.timeout)
+	} else {
+		params := params.RunParams{
+			Commands: c.commands,
+			Timeout:  c.timeout,
+			Machines: c.machines,
+			Services: c.services,
+			Units:    c.units,
+		}
+		runResults, err = client.Run(params)
+	}
+
+	if err != nil {
+		return err
+	}
+
+	// If we are just dealing with one result, AND we are using the smart
+	// format, then pretend we were running it locally.
+	if len(runResults) == 1 && c.out.Name() == "smart" {
+		result := runResults[0]
+		ctx.Stdout.Write(result.Stdout)
+		ctx.Stderr.Write(result.Stderr)
+		if result.Error != "" {
+			// Convert the error string back into an error object.
+			return fmt.Errorf("%s", result.Error)
+		}
+		if result.Code != 0 {
+			return cmd.NewRcPassthroughError(result.Code)
+		}
+		return nil
+	}
+
+	c.out.Write(ctx, ConvertRunResults(runResults))
+	return nil
+}
+
+// In order to be able to easily mock out the API side for testing,
+// the API client is got using a function.
+
+type RunClient interface {
+	Close() error
+	RunOnAllMachines(commands string, timeout time.Duration) ([]params.RunResult, error)
+	Run(run params.RunParams) ([]params.RunResult, error)
+}
+
+// Here we need the signature to be correct for the interface.
+var getAPIClient = func(name string) (RunClient, error) {
+	return juju.NewAPIClientFromName(name)
+}
 
=== added file 'cmd/juju/run_test.go'
--- cmd/juju/run_test.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/run_test.go	2014-01-12 20:39:24 +0000
@@ -0,0 +1,443 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"fmt"
+	"time"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+)
+
+type RunSuite struct {
+	testing.FakeHomeSuite
+}
+
+var _ = gc.Suite(&RunSuite{})
+
+func (*RunSuite) TestTargetArgParsing(c *gc.C) {
+	for i, test := range []struct {
+		message  string
+		args     []string
+		all      bool
+		machines []string
+		units    []string
+		services []string
+		commands string
+		errMatch string
+	}{{
+		message:  "no args",
+		errMatch: "no commands specified",
+	}, {
+		message:  "no target",
+		args:     []string{"sudo reboot"},
+		errMatch: "You must specify a target, either through --all, --machine, --service or --unit",
+	}, {
+		message:  "too many args",
+		args:     []string{"--all", "sudo reboot", "oops"},
+		errMatch: `unrecognized args: \["oops"\]`,
+	}, {
+		message:  "command to all machines",
+		args:     []string{"--all", "sudo reboot"},
+		all:      true,
+		commands: "sudo reboot",
+	}, {
+		message:  "all and defined machines",
+		args:     []string{"--all", "--machine=1,2", "sudo reboot"},
+		errMatch: `You cannot specify --all and individual machines`,
+	}, {
+		message:  "command to machines 1, 2, and 1/kvm/0",
+		args:     []string{"--machine=1,2,1/kvm/0", "sudo reboot"},
+		commands: "sudo reboot",
+		machines: []string{"1", "2", "1/kvm/0"},
+	}, {
+		message: "bad machine names",
+		args:    []string{"--machine=foo,machine-2", "sudo reboot"},
+		errMatch: "" +
+			"The following run targets are not valid:\n" +
+			"  \"foo\" is not a valid machine id\n" +
+			"  \"machine-2\" is not a valid machine id",
+	}, {
+		message:  "all and defined services",
+		args:     []string{"--all", "--service=wordpress,mysql", "sudo reboot"},
+		errMatch: `You cannot specify --all and individual services`,
+	}, {
+		message:  "command to services wordpress and mysql",
+		args:     []string{"--service=wordpress,mysql", "sudo reboot"},
+		commands: "sudo reboot",
+		services: []string{"wordpress", "mysql"},
+	}, {
+		message: "bad service names",
+		args:    []string{"--service", "foo,2,foo/0", "sudo reboot"},
+		errMatch: "" +
+			"The following run targets are not valid:\n" +
+			"  \"2\" is not a valid service name\n" +
+			"  \"foo/0\" is not a valid service name",
+	}, {
+		message:  "all and defined units",
+		args:     []string{"--all", "--unit=wordpress/0,mysql/1", "sudo reboot"},
+		errMatch: `You cannot specify --all and individual units`,
+	}, {
+		message:  "command to valid units",
+		args:     []string{"--unit=wordpress/0,wordpress/1,mysql/0", "sudo reboot"},
+		commands: "sudo reboot",
+		units:    []string{"wordpress/0", "wordpress/1", "mysql/0"},
+	}, {
+		message: "bad unit names",
+		args:    []string{"--unit", "foo,2,foo/0", "sudo reboot"},
+		errMatch: "" +
+			"The following run targets are not valid:\n" +
+			"  \"foo\" is not a valid unit name\n" +
+			"  \"2\" is not a valid unit name",
+	}, {
+		message:  "command to mixed valid targets",
+		args:     []string{"--machine=0", "--unit=wordpress/0,wordpress/1", "--service=mysql", "sudo reboot"},
+		commands: "sudo reboot",
+		machines: []string{"0"},
+		services: []string{"mysql"},
+		units:    []string{"wordpress/0", "wordpress/1"},
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		runCmd := &RunCommand{}
+		testing.TestInit(c, runCmd, test.args, test.errMatch)
+		if test.errMatch == "" {
+			c.Check(runCmd.all, gc.Equals, test.all)
+			c.Check(runCmd.machines, gc.DeepEquals, test.machines)
+			c.Check(runCmd.services, gc.DeepEquals, test.services)
+			c.Check(runCmd.units, gc.DeepEquals, test.units)
+			c.Check(runCmd.commands, gc.Equals, test.commands)
+		}
+	}
+}
+
+func (*RunSuite) TestTimeoutArgParsing(c *gc.C) {
+	for i, test := range []struct {
+		message  string
+		args     []string
+		errMatch string
+		timeout  time.Duration
+	}{{
+		message: "default time",
+		args:    []string{"--all", "sudo reboot"},
+		timeout: 5 * time.Minute,
+	}, {
+		message:  "invalid time",
+		args:     []string{"--timeout=foo", "--all", "sudo reboot"},
+		errMatch: `invalid value "foo" for flag --timeout: time: invalid duration foo`,
+	}, {
+		message: "two hours",
+		args:    []string{"--timeout=2h", "--all", "sudo reboot"},
+		timeout: 2 * time.Hour,
+	}, {
+		message: "3 minutes 30 seconds",
+		args:    []string{"--timeout=3m30s", "--all", "sudo reboot"},
+		timeout: (3 * time.Minute) + (30 * time.Second),
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		runCmd := &RunCommand{}
+		testing.TestInit(c, runCmd, test.args, test.errMatch)
+		if test.errMatch == "" {
+			c.Check(runCmd.timeout, gc.Equals, test.timeout)
+		}
+	}
+}
+
+func (s *RunSuite) TestConvertRunResults(c *gc.C) {
+	for i, test := range []struct {
+		message  string
+		results  []params.RunResult
+		expected interface{}
+	}{{
+		message:  "empty",
+		expected: []interface{}{},
+	}, {
+		message: "minimum is machine id and stdout",
+		results: []params.RunResult{
+			makeRunResult(mockResponse{machineId: "1"}),
+		},
+		expected: []interface{}{
+			map[string]interface{}{
+				"MachineId": "1",
+				"Stdout":    "",
+			}},
+	}, {
+		message: "other fields are copied if there",
+		results: []params.RunResult{
+			makeRunResult(mockResponse{
+				machineId: "1",
+				stdout:    "stdout",
+				stderr:    "stderr",
+				code:      42,
+				unitId:    "unit/0",
+				error:     "error",
+			}),
+		},
+		expected: []interface{}{
+			map[string]interface{}{
+				"MachineId":  "1",
+				"Stdout":     "stdout",
+				"Stderr":     "stderr",
+				"ReturnCode": 42,
+				"UnitId":     "unit/0",
+				"Error":      "error",
+			}},
+	}, {
+		message: "stdout and stderr are base64 encoded if not valid utf8",
+		results: []params.RunResult{
+			params.RunResult{
+				RemoteResponse: cmd.RemoteResponse{
+					Stdout: []byte{0xff},
+					Stderr: []byte{0xfe},
+				},
+				MachineId: "jake",
+			},
+		},
+		expected: []interface{}{
+			map[string]interface{}{
+				"MachineId":       "jake",
+				"Stdout":          "/w==",
+				"Stdout.encoding": "base64",
+				"Stderr":          "/g==",
+				"Stderr.encoding": "base64",
+			}},
+	}, {
+		message: "more than one",
+		results: []params.RunResult{
+			makeRunResult(mockResponse{machineId: "1"}),
+			makeRunResult(mockResponse{machineId: "2"}),
+			makeRunResult(mockResponse{machineId: "3"}),
+		},
+		expected: []interface{}{
+			map[string]interface{}{
+				"MachineId": "1",
+				"Stdout":    "",
+			},
+			map[string]interface{}{
+				"MachineId": "2",
+				"Stdout":    "",
+			},
+			map[string]interface{}{
+				"MachineId": "3",
+				"Stdout":    "",
+			},
+		},
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		result := ConvertRunResults(test.results)
+		c.Check(result, jc.DeepEquals, test.expected)
+	}
+}
+
+func (s *RunSuite) TestRunForMachineAndUnit(c *gc.C) {
+	mock := s.setupMockAPI()
+	machineResponse := mockResponse{
+		stdout:    "megatron\n",
+		machineId: "0",
+	}
+	unitResponse := mockResponse{
+		stdout:    "bumblebee",
+		machineId: "1",
+		unitId:    "unit/0",
+	}
+	mock.setResponse("0", machineResponse)
+	mock.setResponse("unit/0", unitResponse)
+
+	unformatted := ConvertRunResults([]params.RunResult{
+		makeRunResult(machineResponse),
+		makeRunResult(unitResponse),
+	})
+
+	jsonFormatted, err := cmd.FormatJson(unformatted)
+	c.Assert(err, gc.IsNil)
+
+	context, err := testing.RunCommand(c, &RunCommand{}, []string{
+		"--format=json", "--machine=0", "--unit=unit/0", "hostname",
+	})
+	c.Assert(err, gc.IsNil)
+
+	c.Check(testing.Stdout(context), gc.Equals, string(jsonFormatted)+"\n")
+}
+
+func (s *RunSuite) TestAllMachines(c *gc.C) {
+	mock := s.setupMockAPI()
+	mock.setMachinesAlive("0", "1")
+	response0 := mockResponse{
+		stdout:    "megatron\n",
+		machineId: "0",
+	}
+	response1 := mockResponse{
+		error:     "command timed out",
+		machineId: "1",
+	}
+	mock.setResponse("0", response0)
+
+	unformatted := ConvertRunResults([]params.RunResult{
+		makeRunResult(response0),
+		makeRunResult(response1),
+	})
+
+	jsonFormatted, err := cmd.FormatJson(unformatted)
+	c.Assert(err, gc.IsNil)
+
+	context, err := testing.RunCommand(c, &RunCommand{}, []string{
+		"--format=json", "--all", "hostname",
+	})
+	c.Assert(err, gc.IsNil)
+
+	c.Check(testing.Stdout(context), gc.Equals, string(jsonFormatted)+"\n")
+}
+
+func (s *RunSuite) TestSingleResponse(c *gc.C) {
+	mock := s.setupMockAPI()
+	mock.setMachinesAlive("0")
+	mockResponse := mockResponse{
+		stdout:    "stdout\n",
+		stderr:    "stderr\n",
+		code:      42,
+		machineId: "0",
+	}
+	mock.setResponse("0", mockResponse)
+	unformatted := ConvertRunResults([]params.RunResult{
+		makeRunResult(mockResponse)})
+	yamlFormatted, err := cmd.FormatYaml(unformatted)
+	c.Assert(err, gc.IsNil)
+	jsonFormatted, err := cmd.FormatJson(unformatted)
+	c.Assert(err, gc.IsNil)
+
+	for i, test := range []struct {
+		message    string
+		format     string
+		stdout     string
+		stderr     string
+		errorMatch string
+	}{{
+		message:    "smart (default)",
+		stdout:     "stdout\n",
+		stderr:     "stderr\n",
+		errorMatch: "rc: 42",
+	}, {
+		message: "yaml output",
+		format:  "yaml",
+		stdout:  string(yamlFormatted) + "\n",
+	}, {
+		message: "json output",
+		format:  "json",
+		stdout:  string(jsonFormatted) + "\n",
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		args := []string{}
+		if test.format != "" {
+			args = append(args, "--format", test.format)
+		}
+		args = append(args, "--all", "ignored")
+		context, err := testing.RunCommand(c, &RunCommand{}, args)
+		if test.errorMatch != "" {
+			c.Check(err, gc.ErrorMatches, test.errorMatch)
+		} else {
+			c.Check(err, gc.IsNil)
+		}
+		c.Check(testing.Stdout(context), gc.Equals, test.stdout)
+		c.Check(testing.Stderr(context), gc.Equals, test.stderr)
+	}
+}
+
+func (s *RunSuite) setupMockAPI() *mockRunAPI {
+	mock := &mockRunAPI{}
+	s.PatchValue(&getAPIClient, func(name string) (RunClient, error) {
+		return mock, nil
+	})
+	return mock
+}
+
+type mockRunAPI struct {
+	stdout string
+	stderr string
+	code   int
+	// machines, services, units
+	machines  map[string]bool
+	responses map[string]params.RunResult
+}
+
+type mockResponse struct {
+	stdout    string
+	stderr    string
+	code      int
+	error     string
+	machineId string
+	unitId    string
+}
+
+var _ RunClient = (*mockRunAPI)(nil)
+
+func (m *mockRunAPI) setMachinesAlive(ids ...string) {
+	if m.machines == nil {
+		m.machines = make(map[string]bool)
+	}
+	for _, id := range ids {
+		m.machines[id] = true
+	}
+}
+
+func makeRunResult(mock mockResponse) params.RunResult {
+	return params.RunResult{
+		RemoteResponse: cmd.RemoteResponse{
+			Stdout: []byte(mock.stdout),
+			Stderr: []byte(mock.stderr),
+			Code:   mock.code,
+		},
+		MachineId: mock.machineId,
+		UnitId:    mock.unitId,
+		Error:     mock.error,
+	}
+}
+
+func (m *mockRunAPI) setResponse(id string, mock mockResponse) {
+	if m.responses == nil {
+		m.responses = make(map[string]params.RunResult)
+	}
+	m.responses[id] = makeRunResult(mock)
+}
+
+func (*mockRunAPI) Close() error {
+	return nil
+}
+
+func (m *mockRunAPI) RunOnAllMachines(commands string, timeout time.Duration) ([]params.RunResult, error) {
+	var result []params.RunResult
+	for machine := range m.machines {
+		response, found := m.responses[machine]
+		if !found {
+			// Consider this a timeout
+			response = params.RunResult{MachineId: machine, Error: "command timed out"}
+		}
+		result = append(result, response)
+	}
+
+	return result, nil
+}
+
+func (m *mockRunAPI) Run(runParams params.RunParams) ([]params.RunResult, error) {
+	var result []params.RunResult
+	// Just add in ids that match in order.
+	for _, id := range runParams.Machines {
+		response, found := m.responses[id]
+		if found {
+			result = append(result, response)
+		}
+	}
+	// mock ignores services
+	for _, id := range runParams.Units {
+		response, found := m.responses[id]
+		if found {
+			result = append(result, response)
+		}
+	}
+
+	return result, nil
+}
 
=== modified file 'testing/environ.go'
--- testing/environ.go	2013-12-10 17:17:31 +0000
+++ testing/environ.go	2014-01-12 20:39:24 +0000
@@ -12,6 +12,7 @@
 
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/juju/osenv"
+	"launchpad.net/juju-core/testing/testbase"
 )
 
 // FakeConfig() returns an environment configuration for a
@@ -233,3 +234,14 @@
 func MakeMultipleEnvHome(c *gc.C) *FakeHome {
 	return MakeFakeHome(c, MultipleEnvConfig, SampleCertName, SampleCertName+"-2")
 }
+
+type FakeHomeSuite struct {
+	testbase.LoggingSuite
+	Home *FakeHome
+}
+
+func (s *FakeHomeSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
+	s.Home = MakeSampleHome(c)
+	s.AddCleanup(func(*gc.C) { s.Home.Restore() })
+}