Improve shell command stderr handling

Changes in `shell` package:
While there are many public functions for grabbing a command's mixed
output or stdout output only, there is no way to specifically get Stderr
instead. Stderr is needed to debug issues, which are (almost) always
accompanied by an error (non-0 exit status). This commit changes the
handling of stdout and stderr to make it possible to return a "merged"
string (stdout+stderr) or stdout only. As we can now grab stderr
separately, we can also add it to the returned error, if any.

This means that every error that is returned by public functions in this
package is now of type `ErrWithCmdOutput`, which makes it possible for the
callee to explicitely get the stderr stream, if needed, or just print
the whole error including stderr.
It also means that per default, an error will not just be
"FatalError{Underlying: exit status 1}", but instead contain the whole
Stderr stream. The underlying error is encapsulated in the new error
type.

Signed-off-by: Ramon Rüttimann <me@ramonr.ch>

modules/shell/command.go

@@ -11,10 +11,9 @@ import (
 	"sync"
 	"syscall"
 
-	"github.com/stretchr/testify/require"
-
 	"github.com/gruntwork-io/terratest/modules/logger"
 	"github.com/gruntwork-io/terratest/modules/testing"
+	"github.com/stretchr/testify/require"
 )
 
 // Command is a simpler struct for defining commands than Go's built-in Cmd.
@@ -27,44 +26,46 @@ type Command struct {
 	Logger *logger.Logger
 }
 
-// RunCommand runs a shell command and redirects its stdout and stderr to the stdout of the atomic script itself.
+// RunCommand runs a shell command and redirects its stdout and stderr to the stdout of the atomic script itself. If
+// there are any errors, fail the test.
 func RunCommand(t testing.TestingT, command Command) {
 	err := RunCommandE(t, command)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 }
 
-// RunCommandE runs a shell command and redirects its stdout and stderr to the stdout of the atomic script itself.
+// RunCommandE runs a shell command and redirects its stdout and stderr to the stdout of the atomic script itself. Any
+// returned error will be of type ErrWithCmdOutput, containing the output streams and the underlying error.
 func RunCommandE(t testing.TestingT, command Command) error {
-	_, err := RunCommandAndGetOutputE(t, command)
-	return err
+	output, err := runCommand(t, command)
+	if err != nil {
+		return &ErrWithCmdOutput{err, output}
+	}
+	return nil
 }
 
-// RunCommandAndGetOutput runs a shell command and returns its stdout and stderr as a string. The stdout and stderr of that command will also
-// be printed to the stdout and stderr of this Go program to make debugging easier.
+// RunCommandAndGetOutput runs a shell command and returns its stdout and stderr as a string. The stdout and stderr of
+// that command will also be logged with Command.Log to make debugging easier. If there are any errors, fail the test.
 func RunCommandAndGetOutput(t testing.TestingT, command Command) string {
 	out, err := RunCommandAndGetOutputE(t, command)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 	return out
 }
 
-// RunCommandAndGetOutputE runs a shell command and returns its stdout and stderr as a string. The stdout and stderr of that command will also
-// be printed to the stdout and stderr of this Go program to make debugging easier.
+// RunCommandAndGetOutputE runs a shell command and returns its stdout and stderr as a string. The stdout and stderr of
+// that command will also be logged with Command.Log to make debugging easier. Any returned error will be of type
+// ErrWithCmdOutput, containing the output streams and the underlying error.
 func RunCommandAndGetOutputE(t testing.TestingT, command Command) (string, error) {
-	allOutput := []string{}
-	err := runCommandAndStoreOutputE(t, command, &allOutput, &allOutput)
+	output, err := runCommand(t, command)
+	if err != nil {
+		return output.merged.String(), &ErrWithCmdOutput{err, output}
+	}
 
-	output := strings.Join(allOutput, "\n")
-	return output, err
+	return output.merged.String(), nil
 }
 
-// RunCommandAndGetStdOut runs a shell command and returns solely its stdout (but
-// not stderr) as a string. The stdout and stderr of that command will also be
-// printed to the stdout and stderr of this Go program to make debugging easier.
-// If there are any errors, fail the test.
+// RunCommandAndGetStdOut runs a shell command and returns solely its stdout (but not stderr) as a string. The stdout and
+// stderr of that command will also be logged with Command.Log to make debugging easier. If there are any errors, fail
+// the test.
 func RunCommandAndGetStdOut(t testing.TestingT, command Command) string {
 	output, err := RunCommandAndGetStdOutE(t, command)
 	require.NoError(t, err)
@@ -73,20 +74,29 @@ func RunCommandAndGetStdOut(t testing.TestingT, command Command) string {
 
 // RunCommandAndGetStdOutE runs a shell command and returns solely its stdout (but not stderr) as a string. The stdout
 // and stderr of that command will also be printed to the stdout and stderr of this Go program to make debugging easier.
+// Any returned error will be of type ErrWithCmdOutput, containing the output streams and the underlying error.
 func RunCommandAndGetStdOutE(t testing.TestingT, command Command) (string, error) {
-	stdout := []string{}
-	stderr := []string{}
-	err := runCommandAndStoreOutputE(t, command, &stdout, &stderr)
+	output, err := runCommand(t, command)
+	if err != nil {
+		return output.stdout.String(), &ErrWithCmdOutput{err, output}
+	}
+
+	return strings.Join(output.stdout.Lines, "\n"), nil
+}
 
-	output := strings.Join(stdout, "\n")
-	return output, err
+type ErrWithCmdOutput struct {
+	Underlying error
+	Output     *output
 }
 
-// runCommandAndStoreOutputE runs a shell command and stores each line from stdout
-// and stderr in the given storedStdout and storedStderr variables, respectively.
-// Depending on the logger, the stdout and stderr of that command will also be
-// printed to the stdout and stderr of this Go program to make debugging easier.
-func runCommandAndStoreOutputE(t testing.TestingT, command Command, storedStdout *[]string, storedStderr *[]string) error {
+func (e *ErrWithCmdOutput) Error() string {
+	return fmt.Sprintf("error while running command: %v; %s", e.Underlying, e.Output.stderr.String())
+}
+
+// runCommand runs a shell command and stores each line from stdout and stderr in Output. Depending on the logger, the
+// stdout and stderr of that command will also be printed to the stdout and stderr of this Go program to make debugging
+// easier.
+func runCommand(t testing.TestingT, command Command) (*output, error) {
 	command.Logger.Logf(t, "Running command %s with args %s", command.Command, command.Args)
 
 	cmd := exec.Command(command.Command, command.Args...)
@@ -96,84 +106,85 @@ func runCommandAndStoreOutputE(t testing.TestingT, command Command, storedStdout
 
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	stderr, err := cmd.StderrPipe()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	err = cmd.Start()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	if err := readStdoutAndStderr(t, command.Logger, stdout, stderr, storedStdout, storedStderr); err != nil {
-		return err
-	}
-
-	if err := cmd.Wait(); err != nil {
-		return err
+	output, err := readStdoutAndStderr(t, command.Logger, stdout, stderr)
+	if err != nil {
+		return output, err
 	}
 
-	return nil
+	return output, cmd.Wait()
 }
 
 // This function captures stdout and stderr into the given variables while still printing it to the stdout and stderr
 // of this Go program
-func readStdoutAndStderr(t testing.TestingT, log *logger.Logger, stdout, stderr io.ReadCloser, storedStdout, storedStderr *[]string) error {
+func readStdoutAndStderr(t testing.TestingT, log *logger.Logger, stdout, stderr io.ReadCloser) (*output, error) {
+	out := newOutput()
 	stdoutReader := bufio.NewReader(stdout)
 	stderrReader := bufio.NewReader(stderr)
 
 	wg := &sync.WaitGroup{}
-	mutex := &sync.Mutex{}
 
 	wg.Add(2)
 	var stdoutErr, stderrErr error
 	go func() {
 		defer wg.Done()
-		stdoutErr = readData(t, log, stdoutReader, mutex, storedStdout)
+		stdoutErr = readData(t, log, stdoutReader, out.stdout)
 	}()
 	go func() {
 		defer wg.Done()
-		stderrErr = readData(t, log, stderrReader, mutex, storedStderr)
+		stderrErr = readData(t, log, stderrReader, out.stderr)
 	}()
 	wg.Wait()
 
 	if stdoutErr != nil {
-		return stdoutErr
+		return out, stdoutErr
 	}
 	if stderrErr != nil {
-		return stderrErr
+		return out, stderrErr
 	}
 
-	return nil
+	return out, nil
 }
 
-func readData(t testing.TestingT, log *logger.Logger, reader *bufio.Reader, mutex *sync.Mutex, allOutput *[]string) error {
+func readData(t testing.TestingT, log *logger.Logger, reader *bufio.Reader, writer io.StringWriter) error {
 	var line string
 	var err error
 	for {
 		line, err = reader.ReadString('\n')
 
-		// remove newline, our output is in a slice,
-		// one element per line.
-		line = strings.TrimSuffix(line, "\n")
-
-		// only return early if the line does not have
-		// any contents. We could have a line that does
-		// not not have a newline before io.EOF, we still
-		// need to add it to the output.
-		if len(line) == 0 && err == io.EOF {
-			break
+		// separate block so that we do not overwrite the err variable
+		// that we need afterwards to check if we're done
+		{
+			// remove newline, our output is in a slice,
+			// one element per line.
+			line = strings.TrimSuffix(line, "\n")
+
+			// only return early if the line does not have
+			// any contents. We could have a line that does
+			// not not have a newline before io.EOF, we still
+			// need to add it to the output.
+			if len(line) == 0 && err == io.EOF {
+				break
+			}
+
+			log.Logf(t, line)
+			if _, err := writer.WriteString(line); err != nil {
+				return err
+			}
 		}
 
-		log.Logf(t, line)
-		mutex.Lock()
-		*allOutput = append(*allOutput, line)
-		mutex.Unlock()
-
 		if err != nil {
 			break
 		}
@@ -187,6 +198,10 @@ func readData(t testing.TestingT, log *logger.Logger, reader *bufio.Reader, mute
 // GetExitCodeForRunCommandError tries to read the exit code for the error object returned from running a shell command. This is a bit tricky to do
 // in a way that works across platforms.
 func GetExitCodeForRunCommandError(err error) (int, error) {
+	if errWithOutput, ok := err.(*ErrWithCmdOutput); ok {
+		err = errWithOutput.Underlying
+	}
+
 	// http://stackoverflow.com/a/10385867/483528
 	if exitErr, ok := err.(*exec.ExitError); ok {
 		// The program has exited with an exit code != 0

modules/shell/command_test.go

@@ -62,6 +62,23 @@ echo_stderr
 	assert.Equal(t, expectedText, strings.TrimSpace(out))
 }
 
+func TestRunCommandGetExitCode(t *testing.T) {
+	t.Parallel()
+
+	cmd := Command{
+		Command: "bash",
+		Args:    []string{"-c", "exit 42"},
+		Logger:  logger.Discard,
+	}
+
+	out, err := RunCommandAndGetOutputE(t, cmd)
+	assert.Equal(t, "", out)
+	assert.NotNil(t, err)
+	code, err := GetExitCodeForRunCommandError(err)
+	assert.Nil(t, err)
+	assert.Equal(t, code, 42)
+}
+
 func TestRunCommandAndGetOutputConcurrency(t *testing.T) {
 	t.Parallel()
 

modules/shell/output.go

@@ -0,0 +1,75 @@
+package shell
+
+import (
+	"strings"
+	"sync"
+)
+
+// output contains the output after runnig a command.
+type output struct {
+	stdout *outputStream
+	stderr *outputStream
+	// merged contains stdout  and stderr merged into one stream.
+	merged *merged
+}
+
+func newOutput() *output {
+	m := new(merged)
+	return &output{
+		merged: m,
+		stdout: &outputStream{
+			merged: m,
+		},
+		stderr: &outputStream{
+			merged: m,
+		},
+	}
+}
+
+func (o *output) Stdout() string {
+	return o.stdout.String()
+}
+
+func (o *output) Stderr() string {
+	return o.stderr.String()
+}
+
+func (o *output) CombinedOutput() string {
+	return o.merged.String()
+}
+
+type outputStream struct {
+	Lines []string
+	*merged
+}
+
+func (st *outputStream) WriteString(s string) (n int, err error) {
+	st.Lines = append(st.Lines, string(s))
+	return st.merged.WriteString(s)
+}
+
+func (st *outputStream) String() string {
+	return strings.Join(st.Lines, "\n")
+}
+
+type merged struct {
+	// ensure that there are no parallel writes
+	sync.Mutex
+	Lines []string
+}
+
+func (m *merged) String() string {
+	m.Lock()
+	defer m.Unlock()
+
+	return strings.Join(m.Lines, "\n")
+}
+
+func (m *merged) WriteString(s string) (n int, err error) {
+	m.Lock()
+	defer m.Unlock()
+
+	m.Lines = append(m.Lines, string(s))
+
+	return len(s), nil
+}