Add unit tests for separate stdout and stderr on execution

README.md

@@ -136,8 +136,8 @@ For example, for an interface called `ExecutorApi` in the package `nomad`, you m
 ```bash
 mockery \
 --name=ExecutorApi \
---structname=ExecutorApiMock \
+--structname=ExecutorAPIMock \
---filename=ExecutorApiMock.go \
+--filename=ExecutorAPIMock.go \
 --inpackage
 ```
 

api/websocket_test.go

@@ -34,7 +34,7 @@ type WebSocketTestSuite struct {
 	router      *mux.Router
 	executionId runner.ExecutionId
 	runner      runner.Runner
-	apiMock     *nomad.ExecutorApiMock
+	apiMock     *nomad.ExecutorAPIMock
 	server      *httptest.Server
 }
 
@@ -307,8 +307,8 @@ func TestRawToCodeOceanWriter(t *testing.T) {
 
 // --- Test suite specific test helpers ---
 
-func newNomadAllocationWithMockedApiClient(runnerId string) (r runner.Runner, mock *nomad.ExecutorApiMock) {
+func newNomadAllocationWithMockedApiClient(runnerId string) (r runner.Runner, mock *nomad.ExecutorAPIMock) {
-	mock = &nomad.ExecutorApiMock{}
+	mock = &nomad.ExecutorAPIMock{}
 	r = runner.NewNomadAllocation(runnerId, mock)
 	return
 }
@@ -335,7 +335,7 @@ func (suite *WebSocketTestSuite) webSocketUrl(scheme, runnerId string, execution
 var executionRequestLs = dto.ExecutionRequest{Command: "ls"}
 
 // mockApiExecuteLs mocks the ExecuteCommand of an ExecutorApi to act as if 'ls existing-file non-existing-file' was executed.
-func mockApiExecuteLs(api *nomad.ExecutorApiMock) {
+func mockApiExecuteLs(api *nomad.ExecutorAPIMock) {
 	helpers.MockApiExecute(api, &executionRequestLs,
 		func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, stdout, stderr io.Writer) (int, error) {
 			_, _ = stdout.Write([]byte("existing-file\n"))
@@ -347,7 +347,7 @@ func mockApiExecuteLs(api *nomad.ExecutorApiMock) {
 var executionRequestHead = dto.ExecutionRequest{Command: "head -n 1"}
 
 // mockApiExecuteHead mocks the ExecuteCommand of an ExecutorApi to act as if 'head -n 1' was executed.
-func mockApiExecuteHead(api *nomad.ExecutorApiMock) {
+func mockApiExecuteHead(api *nomad.ExecutorAPIMock) {
 	helpers.MockApiExecute(api, &executionRequestHead,
 		func(_ string, _ context.Context, _ []string, _ bool, stdin io.Reader, stdout io.Writer, stderr io.Writer) (int, error) {
 			scanner := bufio.NewScanner(stdin)
@@ -362,7 +362,7 @@ func mockApiExecuteHead(api *nomad.ExecutorApiMock) {
 var executionRequestSleep = dto.ExecutionRequest{Command: "sleep infinity"}
 
 // mockApiExecuteSleep mocks the ExecuteCommand method of an ExecutorAPI to sleep until the execution is canceled.
-func mockApiExecuteSleep(api *nomad.ExecutorApiMock) <-chan bool {
+func mockApiExecuteSleep(api *nomad.ExecutorAPIMock) <-chan bool {
 	canceled := make(chan bool, 1)
 	helpers.MockApiExecute(api, &executionRequestSleep,
 		func(_ string, ctx context.Context, _ []string, _ bool, stdin io.Reader, stdout io.Writer, stderr io.Writer) (int, error) {
@@ -376,7 +376,7 @@ func mockApiExecuteSleep(api *nomad.ExecutorApiMock) <-chan bool {
 var executionRequestError = dto.ExecutionRequest{Command: "error"}
 
 // mockApiExecuteError mocks the ExecuteCommand method of an ExecutorApi to return an error.
-func mockApiExecuteError(api *nomad.ExecutorApiMock) {
+func mockApiExecuteError(api *nomad.ExecutorAPIMock) {
 	helpers.MockApiExecute(api, &executionRequestError,
 		func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, _, _ io.Writer) (int, error) {
 			return 0, errors.New("intended error")
@@ -386,7 +386,7 @@ func mockApiExecuteError(api *nomad.ExecutorApiMock) {
 var executionRequestExitNonZero = dto.ExecutionRequest{Command: "exit 42"}
 
 // mockApiExecuteExitNonZero mocks the ExecuteCommand method of an ExecutorApi to exit with exit status 42.
-func mockApiExecuteExitNonZero(api *nomad.ExecutorApiMock) {
+func mockApiExecuteExitNonZero(api *nomad.ExecutorAPIMock) {
 	helpers.MockApiExecute(api, &executionRequestExitNonZero,
 		func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, _, _ io.Writer) (int, error) {
 			return 42, nil

environment/job_test.go

@@ -261,7 +261,7 @@ func TestCreateJobSetsAllGivenArguments(t *testing.T) {
 }
 
 func TestRegisterJobWhenNomadJobRegistrationFails(t *testing.T) {
-	apiMock := nomad.ExecutorApiMock{}
+	apiMock := nomad.ExecutorAPIMock{}
 	expectedErr := errors.New("test error")
 
 	apiMock.On("RegisterNomadJob", mock.AnythingOfType("*api.Job")).Return("", expectedErr)
@@ -278,7 +278,7 @@ func TestRegisterJobWhenNomadJobRegistrationFails(t *testing.T) {
 }
 
 func TestRegisterJobSucceedsWhenMonitoringEvaluationSucceeds(t *testing.T) {
-	apiMock := nomad.ExecutorApiMock{}
+	apiMock := nomad.ExecutorAPIMock{}
 	evaluationID := "id"
 
 	apiMock.On("RegisterNomadJob", mock.AnythingOfType("*api.Job")).Return(evaluationID, nil)
@@ -295,7 +295,7 @@ func TestRegisterJobSucceedsWhenMonitoringEvaluationSucceeds(t *testing.T) {
 }
 
 func TestRegisterJobReturnsErrorWhenMonitoringEvaluationFails(t *testing.T) {
-	apiMock := nomad.ExecutorApiMock{}
+	apiMock := nomad.ExecutorAPIMock{}
 	evaluationID := "id"
 	expectedErr := errors.New("test error")
 

environment/manager_test.go

@@ -16,7 +16,7 @@ import (
 type CreateOrUpdateTestSuite struct {
 	suite.Suite
 	runnerManagerMock        runner.ManagerMock
-	apiMock                  nomad.ExecutorApiMock
+	apiMock                  nomad.ExecutorAPIMock
 	registerNomadJobMockCall *mock.Call
 	request                  dto.ExecutionEnvironmentRequest
 	manager                  *NomadEnvironmentManager
@@ -28,7 +28,7 @@ func TestCreateOrUpdateTestSuite(t *testing.T) {
 
 func (s *CreateOrUpdateTestSuite) SetupTest() {
 	s.runnerManagerMock = runner.ManagerMock{}
-	s.apiMock = nomad.ExecutorApiMock{}
+	s.apiMock = nomad.ExecutorAPIMock{}
 	s.request = dto.ExecutionEnvironmentRequest{
 		PrewarmingPoolSize: 10,
 		CPULimit:           20,

nomad/nomad_test.go

@@ -1,6 +1,7 @@
 package nomad
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -11,8 +12,12 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
+	"gitlab.hpi.de/codeocean/codemoon/poseidon/config"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/tests"
+	"io"
 	"net/url"
+	"regexp"
+	"strings"
 	"testing"
 	"time"
 )
@@ -617,3 +622,139 @@ func createAllocation(modifyTime int64, clientStatus, desiredStatus string) *nom
 func createRecentAllocation(clientStatus, desiredStatus string) *nomadApi.Allocation {
 	return createAllocation(time.Now().Add(time.Minute).UnixNano(), clientStatus, desiredStatus)
 }
+
+func TestExecuteCommandTestSuite(t *testing.T) {
+	suite.Run(t, new(ExecuteCommandTestSuite))
+}
+
+type ExecuteCommandTestSuite struct {
+	suite.Suite
+	allocationID     string
+	ctx              context.Context
+	testCommand      string
+	testCommandArray []string
+	expectedStdout   string
+	expectedStderr   string
+	apiMock          *apiQuerierMock
+	nomadAPIClient   APIClient
+}
+
+func (s *ExecuteCommandTestSuite) SetupTest() {
+	s.allocationID = "test-allocation-id"
+	s.ctx = context.Background()
+	s.testCommand = "echo 'do nothing'"
+	s.testCommandArray = []string{"sh", "-c", s.testCommand}
+	s.expectedStdout = "stdout"
+	s.expectedStderr = "stderr"
+	s.apiMock = &apiQuerierMock{}
+	s.nomadAPIClient = APIClient{apiQuerier: s.apiMock}
+}
+
+const withTTY = true
+
+func (s *ExecuteCommandTestSuite) TestWithSeparateStderr() {
+	config.Config.Server.InteractiveStderr = true
+	commandExitCode := 42
+	stderrExitCode := 1
+
+	var stdout, stderr bytes.Buffer
+	var calledStdoutCommand, calledStderrCommand []string
+
+	// mock regular call
+	s.mockExecute(s.testCommandArray, commandExitCode, nil, func(args mock.Arguments) {
+		var ok bool
+		calledStdoutCommand, ok = args.Get(2).([]string)
+		s.Require().True(ok)
+		writer, ok := args.Get(5).(io.Writer)
+		s.Require().True(ok)
+		_, err := writer.Write([]byte(s.expectedStdout))
+		s.Require().NoError(err)
+	})
+	// mock stderr call
+	s.mockExecute(mock.AnythingOfType("[]string"), stderrExitCode, nil, func(args mock.Arguments) {
+		var ok bool
+		calledStderrCommand, ok = args.Get(2).([]string)
+		s.Require().True(ok)
+		writer, ok := args.Get(5).(io.Writer)
+		s.Require().True(ok)
+		_, err := writer.Write([]byte(s.expectedStderr))
+		s.Require().NoError(err)
+	})
+
+	exitCode, err := s.nomadAPIClient.
+		ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, nullReader{}, &stdout, &stderr)
+	s.Require().NoError(err)
+
+	s.apiMock.AssertNumberOfCalls(s.T(), "Execute", 2)
+	s.Equal(commandExitCode, exitCode)
+
+	s.Run("should wrap command in stderr wrapper", func() {
+		s.Require().NotNil(calledStdoutCommand)
+		stdoutFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrWrapperCommandFormat), "%d", "\\d*")
+		stdoutFifoRegexp = fmt.Sprintf(stdoutFifoRegexp, s.testCommand)
+		s.Regexp(stdoutFifoRegexp, calledStdoutCommand[len(calledStdoutCommand)-1])
+	})
+
+	s.Run("should call correct stderr command", func() {
+		s.Require().NotNil(calledStderrCommand)
+		stderrFifoRegexp := strings.ReplaceAll(regexp.QuoteMeta(stderrFifoCommandFormat), "%d", "\\d*")
+		s.Regexp(stderrFifoRegexp, calledStderrCommand[len(calledStderrCommand)-1])
+	})
+
+	s.Run("should return correct output", func() {
+		s.Equal(s.expectedStdout, stdout.String())
+		s.Equal(s.expectedStderr, stderr.String())
+	})
+}
+
+func (s *ExecuteCommandTestSuite) TestWithSeparateStderrReturnsCommandError() {
+	config.Config.Server.InteractiveStderr = true
+	s.mockExecute(s.testCommandArray, 1, tests.ErrDefault, func(args mock.Arguments) {})
+	s.mockExecute(mock.AnythingOfType("[]string"), 1, nil, func(args mock.Arguments) {})
+	_, err := s.nomadAPIClient.
+		ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, nullReader{}, io.Discard, io.Discard)
+	s.Equal(tests.ErrDefault, err)
+}
+
+func (s *ExecuteCommandTestSuite) TestWithoutSeparateStderr() {
+	config.Config.Server.InteractiveStderr = false
+	var stdout, stderr bytes.Buffer
+	commandExitCode := 42
+
+	// mock regular call
+	s.mockExecute(s.testCommandArray, commandExitCode, nil, func(args mock.Arguments) {
+		stdout, ok := args.Get(5).(io.Writer)
+		s.Require().True(ok)
+		_, err := stdout.Write([]byte(s.expectedStdout))
+		s.Require().NoError(err)
+		stderr, ok := args.Get(6).(io.Writer)
+		s.Require().True(ok)
+		_, err = stderr.Write([]byte(s.expectedStderr))
+		s.Require().NoError(err)
+	})
+
+	exitCode, err := s.nomadAPIClient.
+		ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, nullReader{}, &stdout, &stderr)
+	s.Require().NoError(err)
+
+	s.apiMock.AssertNumberOfCalls(s.T(), "Execute", 1)
+	s.Equal(commandExitCode, exitCode)
+	s.Equal(s.expectedStdout, stdout.String())
+	s.Equal(s.expectedStderr, stderr.String())
+}
+
+func (s *ExecuteCommandTestSuite) TestWithoutSeparateStderrReturnsCommandError() {
+	config.Config.Server.InteractiveStderr = false
+	s.mockExecute(s.testCommandArray, 1, tests.ErrDefault, func(args mock.Arguments) {})
+	_, err := s.nomadAPIClient.
+		ExecuteCommand(s.allocationID, s.ctx, s.testCommandArray, withTTY, nullReader{}, io.Discard, io.Discard)
+	s.Equal(tests.ErrDefault, err)
+}
+
+func (s *ExecuteCommandTestSuite) mockExecute(command interface{}, exitCode int,
+	err error, runFunc func(arguments mock.Arguments)) {
+	s.apiMock.On("Execute", s.allocationID, s.ctx, command, withTTY,
+		mock.Anything, mock.Anything, mock.Anything).
+		Run(runFunc).
+		Return(exitCode, err)
+}

runner/manager_test.go

@@ -25,13 +25,13 @@ func TestGetNextRunnerTestSuite(t *testing.T) {
 
 type ManagerTestSuite struct {
 	suite.Suite
-	apiMock            *nomad.ExecutorApiMock
+	apiMock            *nomad.ExecutorAPIMock
 	nomadRunnerManager *NomadRunnerManager
 	exerciseRunner     Runner
 }
 
 func (s *ManagerTestSuite) SetupTest() {
-	s.apiMock = &nomad.ExecutorApiMock{}
+	s.apiMock = &nomad.ExecutorAPIMock{}
 	// Instantly closed context to manually start the update process in some cases
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel()
@@ -42,7 +42,7 @@ func (s *ManagerTestSuite) SetupTest() {
 	s.registerDefaultEnvironment()
 }
 
-func mockRunnerQueries(apiMock *nomad.ExecutorApiMock, returnedRunnerIds []string) {
+func mockRunnerQueries(apiMock *nomad.ExecutorAPIMock, returnedRunnerIds []string) {
 	// reset expected calls to allow new mocked return values
 	apiMock.ExpectedCalls = []*mock.Call{}
 	call := apiMock.On("WatchAllocations", mock.Anything, mock.Anything, mock.Anything)
@@ -273,7 +273,7 @@ func (s *ManagerTestSuite) TestUpdateRunnersRemovesIdleAndUsedRunner() {
 	s.False(ok)
 }
 
-func modifyMockedCall(apiMock *nomad.ExecutorApiMock, method string, modifier func(call *mock.Call)) {
+func modifyMockedCall(apiMock *nomad.ExecutorAPIMock, method string, modifier func(call *mock.Call)) {
 	for _, c := range apiMock.ExpectedCalls {
 		if c.Method == method {
 			modifier(c)

runner/runner_test.go

@@ -73,7 +73,7 @@ func TestFromContextReturnsIsNotOkWhenContextHasNoRunner(t *testing.T) {
 }
 
 func TestExecuteCallsAPI(t *testing.T) {
-	apiMock := &nomad.ExecutorApiMock{}
+	apiMock := &nomad.ExecutorAPIMock{}
 	apiMock.On("ExecuteCommand", mock.Anything, mock.Anything, mock.Anything, true, mock.Anything, mock.Anything, mock.Anything).Return(0, nil)
 	runner := NewNomadAllocation(tests.DefaultRunnerID, apiMock)
 
@@ -106,8 +106,8 @@ func TestExecuteReturnsAfterTimeout(t *testing.T) {
 	}
 }
 
-func newApiMockWithTimeLimitHandling() (apiMock *nomad.ExecutorApiMock) {
+func newApiMockWithTimeLimitHandling() (apiMock *nomad.ExecutorAPIMock) {
-	apiMock = &nomad.ExecutorApiMock{}
+	apiMock = &nomad.ExecutorAPIMock{}
 	apiMock.
 		On("ExecuteCommand", mock.Anything, mock.Anything, mock.Anything, true, mock.Anything, mock.Anything, mock.Anything).
 		Run(func(args mock.Arguments) {
@@ -125,14 +125,14 @@ func TestUpdateFileSystemTestSuite(t *testing.T) {
 type UpdateFileSystemTestSuite struct {
 	suite.Suite
 	runner                   *NomadAllocation
-	apiMock                  *nomad.ExecutorApiMock
+	apiMock                  *nomad.ExecutorAPIMock
 	mockedExecuteCommandCall *mock.Call
 	command                  []string
 	stdin                    *bytes.Buffer
 }
 
 func (s *UpdateFileSystemTestSuite) SetupTest() {
-	s.apiMock = &nomad.ExecutorApiMock{}
+	s.apiMock = &nomad.ExecutorAPIMock{}
 	s.runner = NewNomadAllocation(tests.DefaultRunnerID, s.apiMock)
 	s.mockedExecuteCommandCall = s.apiMock.On("ExecuteCommand", tests.DefaultRunnerID, mock.Anything, mock.Anything, false, mock.Anything, mock.Anything, mock.Anything).
 		Run(func(args mock.Arguments) {

tests/helpers/test_helpers.go

@@ -114,7 +114,7 @@ func StartTLSServer(t *testing.T, router *mux.Router) (server *httptest.Server,
 
 // MockApiExecute mocks the ExecuteCommand method of an ExecutorApi to call the given method run when the command
 // corresponding to the given ExecutionRequest is called.
-func MockApiExecute(api *nomad.ExecutorApiMock, request *dto.ExecutionRequest,
+func MockApiExecute(api *nomad.ExecutorAPIMock, request *dto.ExecutionRequest,
 	run func(runnerId string, ctx context.Context, command []string, tty bool, stdin io.Reader, stdout, stderr io.Writer) (int, error)) {
 	call := api.On("ExecuteCommand",
 		mock.AnythingOfType("string"),