diff --git a/api/runners_test.go b/api/runners_test.go index 755b7bd..e8f8aa5 100644 --- a/api/runners_test.go +++ b/api/runners_test.go @@ -29,7 +29,7 @@ type MiddlewareTestSuite struct { func (s *MiddlewareTestSuite) SetupTest() { s.manager = &runner.ManagerMock{} - s.runner = runner.NewNomadJob(tests.DefaultRunnerID, nil) + s.runner = runner.NewNomadJob(tests.DefaultRunnerID, nil, nil) s.capturedRunner = nil s.runnerRequest = func(runnerId string) *http.Request { path, err := s.router.Get("test-runner-id").URL(RunnerIdKey, runnerId) @@ -92,7 +92,7 @@ type RunnerRouteTestSuite struct { func (s *RunnerRouteTestSuite) SetupTest() { s.runnerManager = &runner.ManagerMock{} s.router = NewRouter(s.runnerManager, nil) - s.runner = runner.NewNomadJob("some-id", nil) + s.runner = runner.NewNomadJob("some-id", nil, nil) s.executionId = "execution-id" s.runner.Add(s.executionId, &dto.ExecutionRequest{}) s.runnerManager.On("Get", s.runner.Id()).Return(s.runner, nil) diff --git a/api/websocket_test.go b/api/websocket_test.go index 4f83b4d..48cb8ff 100644 --- a/api/websocket_test.go +++ b/api/websocket_test.go @@ -364,7 +364,7 @@ func TestCodeOceanToRawReaderReturnsOnlyAfterOneByteWasReadFromConnection(t *tes func newNomadAllocationWithMockedApiClient(runnerId string) (r runner.Runner, mock *nomad.ExecutorAPIMock) { mock = &nomad.ExecutorAPIMock{} - r = runner.NewNomadJob(runnerId, mock) + r = runner.NewNomadJob(runnerId, mock, nil) return } diff --git a/runner/inactivity_timer_mock.go b/runner/inactivity_timer_mock.go new file mode 100644 index 0000000..366cd55 --- /dev/null +++ b/runner/inactivity_timer_mock.go @@ -0,0 +1,43 @@ +// Code generated by mockery v0.0.0-dev. DO NOT EDIT. + +package runner + +import ( + time "time" + + mock "github.com/stretchr/testify/mock" +) + +// InactivityTimerMock is an autogenerated mock type for the InactivityTimer type +type InactivityTimerMock struct { + mock.Mock +} + +// ResetTimeout provides a mock function with given fields: +func (_m *InactivityTimerMock) ResetTimeout() { + _m.Called() +} + +// SetupTimeout provides a mock function with given fields: duration +func (_m *InactivityTimerMock) SetupTimeout(duration time.Duration) { + _m.Called(duration) +} + +// StopTimeout provides a mock function with given fields: +func (_m *InactivityTimerMock) StopTimeout() { + _m.Called() +} + +// TimeoutPassed provides a mock function with given fields: +func (_m *InactivityTimerMock) TimeoutPassed() bool { + ret := _m.Called() + + var r0 bool + if rf, ok := ret.Get(0).(func() bool); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} diff --git a/runner/manager_test.go b/runner/manager_test.go index 10a4b30..833a692 100644 --- a/runner/manager_test.go +++ b/runner/manager_test.go @@ -37,7 +37,7 @@ func (s *ManagerTestSuite) SetupTest() { cancel() s.nomadRunnerManager = NewNomadRunnerManager(s.apiMock, ctx) - s.exerciseRunner = NewRunner(tests.DefaultRunnerID) + s.exerciseRunner = NewRunner(tests.DefaultRunnerID, s.nomadRunnerManager) s.registerDefaultEnvironment() } @@ -110,7 +110,7 @@ func (s *ManagerTestSuite) TestClaimReturnsNoRunnerOfDifferentEnvironment() { func (s *ManagerTestSuite) TestClaimDoesNotReturnTheSameRunnerTwice() { s.AddIdleRunnerForDefaultEnvironment(s.exerciseRunner) - s.AddIdleRunnerForDefaultEnvironment(NewRunner(tests.AnotherRunnerID)) + s.AddIdleRunnerForDefaultEnvironment(NewRunner(tests.AnotherRunnerID, s.nomadRunnerManager)) firstReceivedRunner, err := s.nomadRunnerManager.Claim(defaultEnvironmentID) s.NoError(err) @@ -220,7 +220,7 @@ func (s *ManagerTestSuite) TestUpdateRunnersRemovesIdleAndUsedRunner() { environment, ok := s.nomadRunnerManager.environments.Get(defaultEnvironmentID) s.Require().True(ok) - testRunner := NewRunner(allocation.JobID) + testRunner := NewRunner(allocation.JobID, s.nomadRunnerManager) environment.idleRunners.Add(testRunner) s.nomadRunnerManager.usedRunners.Add(testRunner) diff --git a/runner/runner_mock.go b/runner/runner_mock.go index 709fe0f..9ac27fe 100644 --- a/runner/runner_mock.go +++ b/runner/runner_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.8.0. DO NOT EDIT. +// Code generated by mockery v0.0.0-dev. DO NOT EDIT. package runner @@ -9,6 +9,8 @@ import ( dto "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" mock "github.com/stretchr/testify/mock" + + time "time" ) // RunnerMock is an autogenerated mock type for the Runner type @@ -83,6 +85,35 @@ func (_m *RunnerMock) Pop(id ExecutionId) (*dto.ExecutionRequest, bool) { return r0, r1 } +// ResetTimeout provides a mock function with given fields: +func (_m *RunnerMock) ResetTimeout() { + _m.Called() +} + +// SetupTimeout provides a mock function with given fields: duration +func (_m *RunnerMock) SetupTimeout(duration time.Duration) { + _m.Called(duration) +} + +// StopTimeout provides a mock function with given fields: +func (_m *RunnerMock) StopTimeout() { + _m.Called() +} + +// TimeoutPassed provides a mock function with given fields: +func (_m *RunnerMock) TimeoutPassed() bool { + ret := _m.Called() + + var r0 bool + if rf, ok := ret.Get(0).(func() bool); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + // UpdateFileSystem provides a mock function with given fields: request func (_m *RunnerMock) UpdateFileSystem(request *dto.UpdateFileSystemRequest) error { ret := _m.Called(request) diff --git a/runner/runner_test.go b/runner/runner_test.go index a4d66c7..508548c 100644 --- a/runner/runner_test.go +++ b/runner/runner_test.go @@ -20,19 +20,19 @@ import ( ) func TestIdIsStored(t *testing.T) { - runner := NewNomadJob(tests.DefaultJobID, nil) + runner := NewNomadJob(tests.DefaultJobID, nil, nil) assert.Equal(t, tests.DefaultJobID, runner.Id()) } func TestMarshalRunner(t *testing.T) { - runner := NewNomadJob(tests.DefaultJobID, nil) + runner := NewNomadJob(tests.DefaultJobID, nil, nil) marshal, err := json.Marshal(runner) assert.NoError(t, err) assert.Equal(t, "{\"runnerId\":\""+tests.DefaultJobID+"\"}", string(marshal)) } func TestExecutionRequestIsStored(t *testing.T) { - runner := NewNomadJob(tests.DefaultJobID, nil) + runner := NewNomadJob(tests.DefaultJobID, nil, nil) executionRequest := &dto.ExecutionRequest{ Command: "command", TimeLimit: 10, @@ -47,7 +47,7 @@ func TestExecutionRequestIsStored(t *testing.T) { } func TestNewContextReturnsNewContextWithRunner(t *testing.T) { - runner := NewNomadJob(tests.DefaultRunnerID, nil) + runner := NewNomadJob(tests.DefaultRunnerID, nil, nil) ctx := context.Background() newCtx := NewContext(ctx, runner) storedRunner := newCtx.Value(runnerContextKey).(Runner) @@ -57,7 +57,7 @@ func TestNewContextReturnsNewContextWithRunner(t *testing.T) { } func TestFromContextReturnsRunner(t *testing.T) { - runner := NewNomadJob(tests.DefaultRunnerID, nil) + runner := NewNomadJob(tests.DefaultRunnerID, nil, nil) ctx := NewContext(context.Background(), runner) storedRunner, ok := FromContext(ctx) @@ -72,50 +72,80 @@ func TestFromContextReturnsIsNotOkWhenContextHasNoRunner(t *testing.T) { assert.False(t, ok) } -func TestExecuteCallsAPI(t *testing.T) { - apiMock := &nomad.ExecutorAPIMock{} - apiMock.On("ExecuteCommand", mock.Anything, mock.Anything, mock.Anything, true, mock.Anything, mock.Anything, mock.Anything).Return(0, nil) - runner := NewNomadJob(tests.DefaultRunnerID, apiMock) +func TestExecuteInteractivelyTestSuite(t *testing.T) { + suite.Run(t, new(ExecuteInteractivelyTestSuite)) +} +type ExecuteInteractivelyTestSuite struct { + suite.Suite + runner *NomadJob + apiMock *nomad.ExecutorAPIMock + timer *InactivityTimerMock + mockedExecuteCommandCall *mock.Call + mockedTimeoutPassedCall *mock.Call +} + +func (s *ExecuteInteractivelyTestSuite) SetupTest() { + s.apiMock = &nomad.ExecutorAPIMock{} + s.mockedExecuteCommandCall = s.apiMock. + On("ExecuteCommand", mock.Anything, mock.Anything, mock.Anything, true, mock.Anything, mock.Anything, mock.Anything). + Return(0, nil) + s.timer = &InactivityTimerMock{} + s.timer.On("ResetTimeout").Return() + s.mockedTimeoutPassedCall = s.timer.On("TimeoutPassed").Return(false) + s.runner = &NomadJob{ + ExecutionStorage: NewLocalExecutionStorage(), + InactivityTimer: s.timer, + id: tests.DefaultRunnerID, + api: s.apiMock, + } +} + +func (s *ExecuteInteractivelyTestSuite) TestCallsApi() { request := &dto.ExecutionRequest{Command: "echo 'Hello World!'"} - runner.ExecuteInteractively(request, nil, nil, nil) + s.runner.ExecuteInteractively(request, nil, nil, nil) time.Sleep(tests.ShortTimeout) s.apiMock.AssertCalled(s.T(), "ExecuteCommand", tests.DefaultRunnerID, mock.Anything, request.FullCommand(), true, mock.Anything, mock.Anything, mock.Anything) } -func TestExecuteReturnsAfterTimeout(t *testing.T) { - apiMock := newApiMockWithTimeLimitHandling() - runner := NewNomadJob(tests.DefaultRunnerID, apiMock) +func (s *ExecuteInteractivelyTestSuite) TestReturnsAfterTimeout() { + s.mockedExecuteCommandCall.Run(func(args mock.Arguments) { + ctx := args.Get(1).(context.Context) + <-ctx.Done() + }). + Return(0, nil) timeLimit := 1 execution := &dto.ExecutionRequest{TimeLimit: timeLimit} - exit, _ := runner.ExecuteInteractively(execution, nil, nil, nil) + exit, _ := s.runner.ExecuteInteractively(execution, nil, nil, nil) select { case <-exit: - assert.FailNow(t, "ExecuteInteractively should not terminate instantly") - case <-time.After(50 * time.Millisecond): + s.FailNow("ExecuteInteractively should not terminate instantly") + case <-time.After(tests.ShortTimeout): } select { case <-time.After(time.Duration(timeLimit) * time.Second): - assert.FailNow(t, "ExecuteInteractively should return after the time limit") + s.FailNow("ExecuteInteractively should return after the time limit") case exitInfo := <-exit: - assert.Equal(t, uint8(0), exitInfo.Code) + s.Equal(uint8(0), exitInfo.Code) } } -func newApiMockWithTimeLimitHandling() (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) { - ctx := args.Get(1).(context.Context) - <-ctx.Done() - }). - Return(0, nil) - return +func (s *ExecuteInteractivelyTestSuite) TestResetTimerGetsCalled() { + execution := &dto.ExecutionRequest{} + s.runner.ExecuteInteractively(execution, nil, nil, nil) + s.timer.AssertCalled(s.T(), "ResetTimeout") +} + +func (s *ExecuteInteractivelyTestSuite) TestExitHasTimeoutErrorIfExecutionTimesOut() { + s.mockedTimeoutPassedCall.Return(true) + execution := &dto.ExecutionRequest{} + exitChannel, _ := s.runner.ExecuteInteractively(execution, nil, nil, nil) + exit := <-exitChannel + s.Equal(ErrorRunnerInactivityTimeout, exit.Err) } func TestUpdateFileSystemTestSuite(t *testing.T) { @@ -125,6 +155,7 @@ func TestUpdateFileSystemTestSuite(t *testing.T) { type UpdateFileSystemTestSuite struct { suite.Suite runner *NomadJob + timer *InactivityTimerMock apiMock *nomad.ExecutorAPIMock mockedExecuteCommandCall *mock.Call command []string @@ -133,18 +164,25 @@ type UpdateFileSystemTestSuite struct { func (s *UpdateFileSystemTestSuite) SetupTest() { s.apiMock = &nomad.ExecutorAPIMock{} - s.runner = NewNomadJob(tests.DefaultRunnerID, s.apiMock) + s.timer = &InactivityTimerMock{} + s.timer.On("ResetTimeout").Return() + s.timer.On("TimeoutPassed").Return(false) + s.runner = &NomadJob{ + ExecutionStorage: NewLocalExecutionStorage(), + InactivityTimer: s.timer, + id: tests.DefaultRunnerID, + api: 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) { s.command = args.Get(2).([]string) s.stdin = args.Get(4).(*bytes.Buffer) - }) + }).Return(0, nil) } func (s *UpdateFileSystemTestSuite) TestUpdateFileSystemForRunnerPerformsTarExtractionWithAbsoluteNamesOnRunner() { // note: this method tests an implementation detail of the method UpdateFileSystemOfRunner method // if the implementation changes, delete this test and write a new one - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{} err := s.runner.UpdateFileSystem(copyRequest) s.NoError(err) @@ -167,7 +205,6 @@ func (s *UpdateFileSystemTestSuite) TestUpdateFileSystemForRunnerReturnsErrorIfA } func (s *UpdateFileSystemTestSuite) TestFilesToCopyAreIncludedInTarArchive() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Copy: []dto.File{{Path: tests.DefaultFileName, Content: []byte(tests.DefaultFileContent)}}} err := s.runner.UpdateFileSystem(copyRequest) s.NoError(err) @@ -182,7 +219,6 @@ func (s *UpdateFileSystemTestSuite) TestFilesToCopyAreIncludedInTarArchive() { } func (s *UpdateFileSystemTestSuite) TestTarFilesContainCorrectPathForRelativeFilePath() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Copy: []dto.File{{Path: tests.DefaultFileName, Content: []byte(tests.DefaultFileContent)}}} _ = s.runner.UpdateFileSystem(copyRequest) @@ -193,7 +229,6 @@ func (s *UpdateFileSystemTestSuite) TestTarFilesContainCorrectPathForRelativeFil } func (s *UpdateFileSystemTestSuite) TestFilesWithAbsolutePathArePutInAbsoluteLocation() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Copy: []dto.File{{Path: tests.FileNameWithAbsolutePath, Content: []byte(tests.DefaultFileContent)}}} _ = s.runner.UpdateFileSystem(copyRequest) @@ -203,7 +238,6 @@ func (s *UpdateFileSystemTestSuite) TestFilesWithAbsolutePathArePutInAbsoluteLoc } func (s *UpdateFileSystemTestSuite) TestDirectoriesAreMarkedAsDirectoryInTar() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Copy: []dto.File{{Path: tests.DefaultDirectoryName, Content: []byte{}}}} _ = s.runner.UpdateFileSystem(copyRequest) @@ -216,7 +250,6 @@ func (s *UpdateFileSystemTestSuite) TestDirectoriesAreMarkedAsDirectoryInTar() { } func (s *UpdateFileSystemTestSuite) TestFilesToRemoveGetRemoved() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Delete: []dto.FilePath{tests.DefaultFileName}} err := s.runner.UpdateFileSystem(copyRequest) s.NoError(err) @@ -225,7 +258,6 @@ func (s *UpdateFileSystemTestSuite) TestFilesToRemoveGetRemoved() { } func (s *UpdateFileSystemTestSuite) TestFilesToRemoveGetEscaped() { - s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Delete: []dto.FilePath{"/some/potentially/harmful'filename"}} err := s.runner.UpdateFileSystem(copyRequest) s.NoError(err) @@ -233,6 +265,13 @@ func (s *UpdateFileSystemTestSuite) TestFilesToRemoveGetEscaped() { s.Contains(strings.Join(s.command, " "), "'/some/potentially/harmful'\\''filename'") } +func (s *UpdateFileSystemTestSuite) TestResetTimerGetsCalled() { + copyRequest := &dto.UpdateFileSystemRequest{} + err := s.runner.UpdateFileSystem(copyRequest) + s.NoError(err) + s.timer.AssertCalled(s.T(), "ResetTimeout") +} + type TarFile struct { Name string Content string @@ -264,14 +303,15 @@ type InactivityTimerTestSuite struct { } func (s *InactivityTimerTestSuite) SetupTest() { - s.returned = make(chan bool) - s.runner = NewRunner(tests.DefaultRunnerID) + s.returned = make(chan bool, 1) s.manager = &ManagerMock{} s.manager.On("Return", mock.Anything).Run(func(_ mock.Arguments) { s.returned <- true }).Return(nil) - s.runner.SetupTimeout(tests.ShortTimeout, s.runner, s.manager) + s.runner = NewRunner(tests.DefaultRunnerID, s.manager) + + s.runner.SetupTimeout(tests.ShortTimeout) } func (s *InactivityTimerTestSuite) TearDownTest() { @@ -287,7 +327,7 @@ func (s *InactivityTimerTestSuite) TestRunnerIsNotReturnedBeforeTimeout() { } func (s *InactivityTimerTestSuite) TestResetTimeoutExtendsTheDeadline() { - time.Sleep(2 * tests.ShortTimeout / 4) + time.Sleep(3 * tests.ShortTimeout / 4) s.runner.ResetTimeout() s.False(tests.ChannelReceivesSomething(s.returned, 3*tests.ShortTimeout/4), "Because of the reset, the timeout should not be reached by now.") @@ -311,23 +351,24 @@ func (s *InactivityTimerTestSuite) TestTimeoutPassedReturnsTrueAfterDeadline() { func (s *InactivityTimerTestSuite) TestTimerIsNotResetAfterDeadline() { time.Sleep(2 * tests.ShortTimeout) - <-s.returned + // We need to empty the returned channel so Return can send to it again. + tests.ChannelReceivesSomething(s.returned, 0) s.runner.ResetTimeout() s.False(tests.ChannelReceivesSomething(s.returned, 2*tests.ShortTimeout)) } -func (s *InactivityTimerTestSuite) TestSetupTimoutStopsOldTimout() { - s.runner.SetupTimeout(3*tests.ShortTimeout, s.runner, s.manager) +func (s *InactivityTimerTestSuite) TestSetupTimeoutStopsOldTimeout() { + s.runner.SetupTimeout(3 * tests.ShortTimeout) s.False(tests.ChannelReceivesSomething(s.returned, 2*tests.ShortTimeout)) s.True(tests.ChannelReceivesSomething(s.returned, 2*tests.ShortTimeout)) } func (s *InactivityTimerTestSuite) TestTimerIsInactiveWhenDurationIsZero() { - s.runner.SetupTimeout(0, s.runner, s.manager) + s.runner.SetupTimeout(0) s.False(tests.ChannelReceivesSomething(s.returned, tests.ShortTimeout)) } -// NewRunner creates a new runner with the provided id. -func NewRunner(id string) Runner { - return NewNomadJob(id, nil) +// NewRunner creates a new runner with the provided id and manager. +func NewRunner(id string, manager Manager) Runner { + return NewNomadJob(id, nil, manager) } diff --git a/runner/storage_test.go b/runner/storage_test.go index ca64d3b..2b25c64 100644 --- a/runner/storage_test.go +++ b/runner/storage_test.go @@ -19,7 +19,7 @@ type RunnerPoolTestSuite struct { func (suite *RunnerPoolTestSuite) SetupTest() { suite.runnerStorage = NewLocalRunnerStorage() - suite.runner = NewRunner(tests.DefaultRunnerID) + suite.runner = NewRunner(tests.DefaultRunnerID, nil) suite.runner.Add(tests.DefaultExecutionID, &dto.ExecutionRequest{Command: "true"}) } @@ -31,7 +31,7 @@ func (suite *RunnerPoolTestSuite) TestAddedRunnerCanBeRetrieved() { } func (suite *RunnerPoolTestSuite) TestRunnerWithSameIdOverwritesOldOne() { - otherRunnerWithSameId := NewRunner(suite.runner.Id()) + otherRunnerWithSameId := NewRunner(suite.runner.Id(), nil) // assure runner is actually different suite.NotEqual(suite.runner, otherRunnerWithSameId) @@ -86,7 +86,7 @@ func (suite *RunnerPoolTestSuite) TestLenChangesOnStoreContentChange() { }) suite.Run("len increases again when different runner is added", func() { - anotherRunner := NewRunner(tests.AnotherRunnerID) + anotherRunner := NewRunner(tests.AnotherRunnerID, nil) suite.runnerStorage.Add(anotherRunner) suite.Equal(2, suite.runnerStorage.Length()) })