feat: split job steps into its own files/structs (#1004)

* refactor: split step_context into separate files

This commit moves functions from the step_context.go file into different
files, but does otherwise not change anything.
This is done to make it easier to review the changes made to these
functions in the next commit, where we introduce a step factory to
facilitate better unit testing of steps.

Co-authored-by: Marcus Noll <[email protected]>
Co-authored-by: Björn Brauer <[email protected]>
Co-authored-by: Robert Kowalski <[email protected]>
Co-authored-by: Philipp Hinrichsen <[email protected]>
Co-authored-by: Jonas Holland <[email protected]>

* refactor: introduce step factory and make steps testable

With this commit we're introducing the `stepFactory` and interfaces
and implementations for each different kind of step (run, docker,
local and remote actions).
Separating each step kind into its own interface and implementation
makes it easier to reason about and to change behaviour of the step.

By introducing interfaces we enable better unit testability as now
each step implementation, the step factory and the job executor can
be tested on their own by mocking out parts that are irrelevant.

This commits prepares us for implementing pre/post actions in a
later PR.

Co-authored-by: Marcus Noll <[email protected]>
Co-authored-by: Björn Brauer <[email protected]>
Co-authored-by: Robert Kowalski <[email protected]>
Co-authored-by: Philipp Hinrichsen <[email protected]>
Co-authored-by: Jonas Holland <[email protected]>

* fix: run post steps in reverse order

* test: add missing asserts for mocks

* refactor: use local reference instead of function

This may make code more easy to follow.

* refactor: correct typo in function name

* test: use named structs

* test: only expected valid calls

There are mocks which are only called on certain conditions.

* refactor: use step-model to get step name

Using the step-logger we have to get the logger name from the
step model.

* test: only mock stopContainer if required

Co-authored-by: Björn Brauer <[email protected]>
Co-authored-by: Marcus Noll <[email protected]>
Co-authored-by: Robert Kowalski <[email protected]>
Co-authored-by: Philipp Hinrichsen <[email protected]>
Co-authored-by: Jonas Holland <[email protected]>
Co-authored-by: Casey Lee <[email protected]>
Co-authored-by: Christopher Homberger <[email protected]>

Author: 8 people

pkg/runner/action.go

@@ -1,6 +1,7 @@
 package runner
 
 import (
+	"context"
 	"io"
 	"io/fs"
 	"strings"
@@ -121,13 +122,92 @@ runs:
 				return nil
 			}
 
-			closerMock.On("Close")
+			if tt.filename != "" {
+				closerMock.On("Close")
+			}
 
-			sc := &StepContext{}
-			action, err := sc.readAction(tt.step, "actionDir", "actionPath", readFile, writeFile)
+			action, err := readActionImpl(tt.step, "actionDir", "actionPath", readFile, writeFile)
 
 			assert.Nil(t, err)
 			assert.Equal(t, tt.expected, action)
+
+			closerMock.AssertExpectations(t)
+		})
+	}
+}
+
+type exprEvalMock struct {
+	ExpressionEvaluator
+	mock.Mock
+}
+
+func (e *exprEvalMock) Interpolate(expr string) string {
+	args := e.Called(expr)
+	return args.String(0)
+}
+
+func TestActionRunner(t *testing.T) {
+	table := []struct {
+		name string
+		step actionStep
+	}{
+		{
+			name: "Test",
+			step: &stepActionRemote{
+				Step: &model.Step{
+					Uses: "repo@ref",
+				},
+				RunContext: &RunContext{
+					ActionRepository: "repo",
+					ActionPath:       "path",
+					ActionRef:        "ref",
+					Config:           &Config{},
+					Run: &model.Run{
+						JobID: "job",
+						Workflow: &model.Workflow{
+							Jobs: map[string]*model.Job{
+								"job": {
+									Name: "job",
+								},
+							},
+						},
+					},
+				},
+				action: &model.Action{
+					Inputs: map[string]model.Input{
+						"key": {
+							Default: "default value",
+						},
+					},
+					Runs: model.ActionRuns{
+						Using: "node16",
+					},
+				},
+				env: map[string]string{},
+			},
+		},
+	}
+
+	for _, tt := range table {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+
+			cm := &containerMock{}
+			cm.On("CopyDir", "/var/run/act/actions/dir/", "dir/", false).Return(func(ctx context.Context) error { return nil })
+			cm.On("Exec", []string{"node", "/var/run/act/actions/dir/path"}, map[string]string{"INPUT_KEY": "default value"}, "", "").Return(func(ctx context.Context) error { return nil })
+			tt.step.getRunContext().JobContainer = cm
+
+			ee := &exprEvalMock{}
+			ee.On("Interpolate", "default value").Return("default value")
+			tt.step.getRunContext().ExprEval = ee
+
+			_, localAction := tt.step.(*stepActionRemote)
+
+			err := runActionImpl(tt.step, "dir", "path", "repo", "ref", localAction)(ctx)
+
+			assert.Nil(t, err)
+			ee.AssertExpectations(t)
+			cm.AssertExpectations(t)
 		})
 	}
 }

pkg/runner/action_test.go

@@ -0,0 +1,68 @@
+package runner
+
+import (
+	"context"
+
+	"github.com/nektos/act/pkg/common"
+	"github.com/nektos/act/pkg/container"
+	"github.com/stretchr/testify/mock"
+)
+
+type containerMock struct {
+	mock.Mock
+	container.Container
+}
+
+func (cm *containerMock) Create(capAdd []string, capDrop []string) common.Executor {
+	args := cm.Called(capAdd, capDrop)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) Pull(forcePull bool) common.Executor {
+	args := cm.Called(forcePull)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) Start(attach bool) common.Executor {
+	args := cm.Called(attach)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) Remove() common.Executor {
+	args := cm.Called()
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) Close() common.Executor {
+	args := cm.Called()
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) UpdateFromEnv(srcPath string, env *map[string]string) common.Executor {
+	args := cm.Called(srcPath, env)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) UpdateFromImageEnv(env *map[string]string) common.Executor {
+	args := cm.Called(env)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) UpdateFromPath(env *map[string]string) common.Executor {
+	args := cm.Called(env)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) Copy(destPath string, files ...*container.FileEntry) common.Executor {
+	args := cm.Called(destPath, files)
+	return args.Get(0).(func(context.Context) error)
+}
+
+func (cm *containerMock) CopyDir(destPath string, srcPath string, useGitIgnore bool) common.Executor {
+	args := cm.Called(destPath, srcPath, useGitIgnore)
+	return args.Get(0).(func(context.Context) error)
+}
+func (cm *containerMock) Exec(command []string, env map[string]string, user, workdir string) common.Executor {
+	args := cm.Called(command, env, user, workdir)
+	return args.Get(0).(func(context.Context) error)
+}

pkg/runner/container_mock_test.go

@@ -70,8 +70,7 @@ func (rc *RunContext) NewExpressionEvaluator() ExpressionEvaluator {
 }
 
 // NewExpressionEvaluator creates a new evaluator
-func (sc *StepContext) NewExpressionEvaluator() ExpressionEvaluator {
-	rc := sc.RunContext
+func (rc *RunContext) NewStepExpressionEvaluator(step step) ExpressionEvaluator {
 	// todo: cleanup EvaluationEnvironment creation
 	job := rc.Run.Job()
 	strategy := make(map[string]interface{})
@@ -97,7 +96,7 @@ func (sc *StepContext) NewExpressionEvaluator() ExpressionEvaluator {
 
 	ee := &exprparser.EvaluationEnvironment{
 		Github: rc.getGithubContext(),
-		Env:    rc.GetEnv(),
+		Env:    *step.getEnv(),
 		Job:    rc.getJobContext(),
 		Steps:  rc.getStepsContext(),
 		Runner: map[string]interface{}{

pkg/runner/expression.go

@@ -147,13 +147,13 @@ func TestEvaluateRunContext(t *testing.T) {
 	}
 }
 
-func TestEvaluateStepContext(t *testing.T) {
+func TestEvaluateStep(t *testing.T) {
 	rc := createRunContext(t)
-
-	sc := &StepContext{
+	step := &stepRun{
 		RunContext: rc,
 	}
-	ee := sc.NewExpressionEvaluator()
+
+	ee := rc.NewStepExpressionEvaluator(step)
 
 	tables := []struct {
 		in      string

pkg/runner/expression_test.go

@@ -14,13 +14,14 @@ type jobInfo interface {
 	startContainer() common.Executor
 	stopContainer() common.Executor
 	closeContainer() common.Executor
-	newStepExecutor(step *model.Step) common.Executor
 	interpolateOutputs() common.Executor
 	result(result string)
 }
 
-func newJobExecutor(info jobInfo) common.Executor {
+func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executor {
 	steps := make([]common.Executor, 0)
+	preSteps := make([]common.Executor, 0)
+	postSteps := make([]common.Executor, 0)
 
 	steps = append(steps, func(ctx context.Context) error {
 		if len(info.matrix()) > 0 {
@@ -29,15 +30,30 @@ func newJobExecutor(info jobInfo) common.Executor {
 		return nil
 	})
 
-	steps = append(steps, info.startContainer())
+	infoSteps := info.steps()
 
-	for i, step := range info.steps() {
-		if step.ID == "" {
-			step.ID = fmt.Sprintf("%d", i)
+	if len(infoSteps) == 0 {
+		return common.NewDebugExecutor("No steps found")
+	}
+
+	preSteps = append(preSteps, info.startContainer())
+
+	for i, stepModel := range infoSteps {
+		if stepModel.ID == "" {
+			stepModel.ID = fmt.Sprintf("%d", i)
+		}
+
+		step, err := sf.newStep(stepModel, rc)
+
+		if err != nil {
+			return common.NewErrorExecutor(err)
 		}
-		stepExec := info.newStepExecutor(step)
+
+		preSteps = append(preSteps, step.pre())
+
+		stepExec := step.main()
 		steps = append(steps, func(ctx context.Context) error {
-			stepName := step.String()
+			stepName := stepModel.String()
 			return (func(ctx context.Context) error {
 				err := stepExec(ctx)
 				if err != nil {
@@ -50,9 +66,11 @@ func newJobExecutor(info jobInfo) common.Executor {
 				return nil
 			})(withStepLogger(ctx, stepName))
 		})
+
+		postSteps = append([]common.Executor{step.post()}, postSteps...)
 	}
 
-	steps = append(steps, func(ctx context.Context) error {
+	postSteps = append(postSteps, func(ctx context.Context) error {
 		jobError := common.JobError(ctx)
 		if jobError != nil {
 			info.result("failure")
@@ -67,5 +85,10 @@ func newJobExecutor(info jobInfo) common.Executor {
 		return nil
 	})
 
-	return common.NewPipelineExecutor(steps...).Finally(info.interpolateOutputs()).Finally(info.closeContainer())
+	pipeline := make([]common.Executor, 0)
+	pipeline = append(pipeline, preSteps...)
+	pipeline = append(pipeline, steps...)
+	pipeline = append(pipeline, postSteps...)
+
+	return common.NewPipelineExecutor(pipeline...).Finally(info.interpolateOutputs()).Finally(info.closeContainer())
 }