fix: always execute closeContainer() executor (#988)

* fix: always execute closeContainer() executor

During our earlier refactoring in #984 we accidentally changed the
behaviour in such a way that the `closeContainer()` executor was never
called.

This commit restores the earlier behaviour.

Ref:
* https://github.com/nektos/act/pull/984/files#diff-c057d66dc9657d8428e290c69871596e2b567bb8fecad62a99cab54398131a84L294
* https://github.com/nektos/act/pull/984/files#diff-ea9d5c93d769ef9b268932dd9990363e97fc3bec8a00114979d049bead5dd718R68

* test: add testcases to ensure job containers are started/stopped

This commit adds tests to ensure that the executors of `startContainer`,
`stopContainer`, `interpolateOutputs` and `closeContainer` are always
called in the correct order.

pkg/runner/job_executor.go

@@ -48,6 +48,7 @@ func newJobExecutor(info jobInfo) common.Executor {
 			return nil
 		})
 	}
+
 	steps = append(steps, func(ctx context.Context) error {
 		err := info.stopContainer()(ctx)
 		if err != nil {
@@ -64,8 +65,5 @@ func newJobExecutor(info jobInfo) common.Executor {
 		return nil
 	})
 
-	return common.NewPipelineExecutor(steps...).Finally(info.interpolateOutputs()).Finally(func(ctx context.Context) error {
+	return common.NewPipelineExecutor(steps...).Finally(info.interpolateOutputs()).Finally(info.closeContainer())
-		info.closeContainer()
-		return nil
-	})
 }

pkg/runner/job_executor_test.go

@@ -62,32 +62,71 @@ func (jpm *jobInfoMock) result(result string) {
 
 func TestNewJobExecutor(t *testing.T) {
 	table := []struct {
-		name     string
+		name          string
-		steps    []*model.Step
+		steps         []*model.Step
-		result   string
+		executedSteps []string
-		hasError bool
+		result        string
+		hasError      bool
 	}{
 		{
-			"zeroSteps",
+			name:  "zeroSteps",
-			[]*model.Step{},
+			steps: []*model.Step{},
-			"success",
+			executedSteps: []string{
-			false,
+				"startContainer",
+				"stopContainer",
+				"interpolateOutputs",
+				"closeContainer",
+			},
+			result:   "success",
+			hasError: false,
 		},
 		{
-			"stepWithoutPrePost",
+			name: "stepWithoutPrePost",
-			[]*model.Step{{
+			steps: []*model.Step{{
 				ID: "1",
 			}},
-			"success",
+			executedSteps: []string{
-			false,
+				"startContainer",
+				"step1",
+				"stopContainer",
+				"interpolateOutputs",
+				"closeContainer",
+			},
+			result:   "success",
+			hasError: false,
 		},
 		{
-			"stepWithFailure",
+			name: "stepWithFailure",
-			[]*model.Step{{
+			steps: []*model.Step{{
 				ID: "1",
 			}},
-			"failure",
+			executedSteps: []string{
-			true,
+				"startContainer",
+				"step1",
+				"stopContainer",
+				"interpolateOutputs",
+				"closeContainer",
+			},
+			result:   "failure",
+			hasError: true,
+		},
+		{
+			name: "multipleSteps",
+			steps: []*model.Step{{
+				ID: "1",
+			}, {
+				ID: "2",
+			}},
+			executedSteps: []string{
+				"startContainer",
+				"step1",
+				"step2",
+				"stopContainer",
+				"interpolateOutputs",
+				"closeContainer",
+			},
+			result:   "success",
+			hasError: false,
 		},
 	}
 
@@ -95,41 +134,50 @@ func TestNewJobExecutor(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx := common.WithJobErrorContainer(context.Background())
 			jpm := &jobInfoMock{}
+			executorOrder := make([]string, 0)
 
 			jpm.On("startContainer").Return(func(ctx context.Context) error {
+				executorOrder = append(executorOrder, "startContainer")
 				return nil
 			})
 
 			jpm.On("steps").Return(tt.steps)
 
 			for _, stepMock := range tt.steps {
-				jpm.On("newStepExecutor", stepMock).Return(func(ctx context.Context) error {
+				func(stepMock *model.Step) {
-					if tt.hasError {
+					jpm.On("newStepExecutor", stepMock).Return(func(ctx context.Context) error {
-						return fmt.Errorf("error")
+						executorOrder = append(executorOrder, "step"+stepMock.ID)
-					}
+						if tt.hasError {
-					return nil
+							return fmt.Errorf("error")
-				})
+						}
+						return nil
+					})
+				}(stepMock)
 			}
 
 			jpm.On("interpolateOutputs").Return(func(ctx context.Context) error {
+				executorOrder = append(executorOrder, "interpolateOutputs")
 				return nil
 			})
 
 			jpm.On("matrix").Return(map[string]interface{}{})
 
 			jpm.On("stopContainer").Return(func(ctx context.Context) error {
+				executorOrder = append(executorOrder, "stopContainer")
 				return nil
 			})
 
 			jpm.On("result", tt.result)
 
 			jpm.On("closeContainer").Return(func(ctx context.Context) error {
+				executorOrder = append(executorOrder, "closeContainer")
 				return nil
 			})
 
 			executor := newJobExecutor(jpm)
 			err := executor(ctx)
 			assert.Nil(t, err)
+			assert.Equal(t, tt.executedSteps, executorOrder)
 		})
 	}
 }