From 17ebf904bc91daa10ab08422c8b4870cd27b3962 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Sat, 13 Sep 2025 13:09:35 -0600 Subject: [PATCH] invoke interpolateResults before setJobResult --- act/runner/job_executor.go | 40 +++-- act/runner/job_executor_test.go | 37 ++++- act/runner/mocks/FieldLogger.go | 264 ++++++++++++++++++++++++++++++++ 3 files changed, 316 insertions(+), 25 deletions(-) create mode 100644 act/runner/mocks/FieldLogger.go diff --git a/act/runner/job_executor.go b/act/runner/job_executor.go index cb50c84c..7f018bf4 100644 --- a/act/runner/job_executor.go +++ b/act/runner/job_executor.go @@ -104,7 +104,7 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo } } - postExecutor = postExecutor.Finally(func(ctx context.Context) error { + setJobResults := func(ctx context.Context) error { jobError := common.JobError(ctx) // Fresh context to ensure job result output works even if prev. context was a cancelled job @@ -113,28 +113,32 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo setJobResult(ctx, info, rc, jobError == nil) setJobOutputs(ctx, rc) + return nil + } + + cleanupJob := func(_ctx context.Context) error { var err error - { - // Separate timeout for cleanup tasks; logger is cleared so that cleanup logs go to runner, not job - ctx, cancel := context.WithTimeout(context.Background(), cleanupTimeout) - defer cancel() - logger := common.Logger(ctx) - logger.Debugf("Cleaning up container for job %s", rc.jobContainerName()) - if err = info.stopContainer()(ctx); err != nil { - logger.Errorf("Error while stop job container %s: %v", rc.jobContainerName(), err) - } + // Separate timeout for cleanup tasks; logger is cleared so that cleanup logs go to runner, not job + ctx, cancel := context.WithTimeout(context.Background(), cleanupTimeout) + defer cancel() - if !rc.IsHostEnv(ctx) && rc.getNetworkCreated(ctx) { - networkName := rc.getNetworkName(ctx) - logger.Debugf("Cleaning up network %s for job %s", networkName, rc.jobContainerName()) - if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil { - logger.Errorf("Error while cleaning network %s: %v", networkName, err) - } + logger := common.Logger(ctx) + logger.Debugf("Cleaning up container for job %s", rc.jobContainerName()) + if err = info.stopContainer()(ctx); err != nil { + logger.Errorf("Error while stop job container %s: %v", rc.jobContainerName(), err) + } + + if !rc.IsHostEnv(ctx) && rc.getNetworkCreated(ctx) { + networkName := rc.getNetworkName(ctx) + logger.Debugf("Cleaning up network %s for job %s", networkName, rc.jobContainerName()) + if err := container.NewDockerNetworkRemoveExecutor(networkName)(ctx); err != nil { + logger.Errorf("Error while cleaning network %s: %v", networkName, err) } } + return err - }) + } pipeline := make([]common.Executor, 0) pipeline = append(pipeline, preSteps...) @@ -152,6 +156,8 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo return postExecutor(ctx) }). Finally(info.interpolateOutputs()). + Finally(setJobResults). + Finally(cleanupJob). Finally(info.closeContainer())) } diff --git a/act/runner/job_executor_test.go b/act/runner/job_executor_test.go index e42bf610..a6c70ce0 100644 --- a/act/runner/job_executor_test.go +++ b/act/runner/job_executor_test.go @@ -12,10 +12,14 @@ import ( "code.forgejo.org/forgejo/runner/v11/act/common" "code.forgejo.org/forgejo/runner/v11/act/container" "code.forgejo.org/forgejo/runner/v11/act/model" + "code.forgejo.org/forgejo/runner/v11/act/runner/mocks" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) +//go:generate mockery --srcpkg=github.com/sirupsen/logrus --name=FieldLogger + func TestJobExecutor(t *testing.T) { tables := []TestJobFileInfo{ {workdir, "uses-and-run-in-one-step", "push", "Invalid run/uses syntax for job:test step:Test", platforms, secrets}, @@ -127,8 +131,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { executedSteps: []string{ "startContainer", "step1", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "success", @@ -144,8 +149,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { executedSteps: []string{ "startContainer", "step1", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "failure", @@ -162,8 +168,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { "startContainer", "pre1", "step1", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "success", @@ -180,8 +187,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { "startContainer", "step1", "post1", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "success", @@ -199,8 +207,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { "pre1", "step1", "post1", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "success", @@ -229,8 +238,9 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { "step3", "post3", "post2", - "stopContainer", "interpolateOutputs", + "setJobResults", + "stopContainer", "closeContainer", }, result: "success", @@ -246,7 +256,19 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { t.Run(tt.name, func(t *testing.T) { fmt.Printf("::group::%s\n", tt.name) - ctx := common.WithJobErrorContainer(t.Context()) + executorOrder := make([]string, 0) + + mockLogger := mocks.NewFieldLogger(t) + mockLogger.On("Debugf", mock.Anything, mock.Anything).Return(0).Maybe() + mockLogger.On("WithField", "jobResult", mock.Anything). + Run(func(args mock.Arguments) { + executorOrder = append(executorOrder, "setJobResults") + }). + Return(&logrus.Entry{Logger: &logrus.Logger{}}).Maybe() + mockLogger.On("WithField", mock.Anything, mock.Anything).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Maybe() + mockLogger.On("WithFields", mock.Anything).Return(&logrus.Entry{Logger: &logrus.Logger{}}).Maybe() + + ctx := common.WithLogger(common.WithJobErrorContainer(t.Context()), mockLogger) jim := &jobInfoMock{} sfm := &stepFactoryMock{} rc := &RunContext{ @@ -262,7 +284,6 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { Config: &Config{}, } rc.ExprEval = rc.NewExpressionEvaluator(ctx) - executorOrder := make([]string, 0) jim.On("steps").Return(tt.steps) diff --git a/act/runner/mocks/FieldLogger.go b/act/runner/mocks/FieldLogger.go new file mode 100644 index 00000000..5cfa5ad2 --- /dev/null +++ b/act/runner/mocks/FieldLogger.go @@ -0,0 +1,264 @@ +// Code generated by mockery v2.53.5. DO NOT EDIT. + +package mocks + +import ( + logrus "github.com/sirupsen/logrus" + mock "github.com/stretchr/testify/mock" +) + +// FieldLogger is an autogenerated mock type for the FieldLogger type +type FieldLogger struct { + mock.Mock +} + +// Debug provides a mock function with given fields: args +func (_m *FieldLogger) Debug(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Debugf provides a mock function with given fields: format, args +func (_m *FieldLogger) Debugf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Debugln provides a mock function with given fields: args +func (_m *FieldLogger) Debugln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Error provides a mock function with given fields: args +func (_m *FieldLogger) Error(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Errorf provides a mock function with given fields: format, args +func (_m *FieldLogger) Errorf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Errorln provides a mock function with given fields: args +func (_m *FieldLogger) Errorln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Fatal provides a mock function with given fields: args +func (_m *FieldLogger) Fatal(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Fatalf provides a mock function with given fields: format, args +func (_m *FieldLogger) Fatalf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Fatalln provides a mock function with given fields: args +func (_m *FieldLogger) Fatalln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Info provides a mock function with given fields: args +func (_m *FieldLogger) Info(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Infof provides a mock function with given fields: format, args +func (_m *FieldLogger) Infof(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Infoln provides a mock function with given fields: args +func (_m *FieldLogger) Infoln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Panic provides a mock function with given fields: args +func (_m *FieldLogger) Panic(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Panicf provides a mock function with given fields: format, args +func (_m *FieldLogger) Panicf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Panicln provides a mock function with given fields: args +func (_m *FieldLogger) Panicln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Print provides a mock function with given fields: args +func (_m *FieldLogger) Print(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Printf provides a mock function with given fields: format, args +func (_m *FieldLogger) Printf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Println provides a mock function with given fields: args +func (_m *FieldLogger) Println(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warn provides a mock function with given fields: args +func (_m *FieldLogger) Warn(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warnf provides a mock function with given fields: format, args +func (_m *FieldLogger) Warnf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warning provides a mock function with given fields: args +func (_m *FieldLogger) Warning(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warningf provides a mock function with given fields: format, args +func (_m *FieldLogger) Warningf(format string, args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, format) + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warningln provides a mock function with given fields: args +func (_m *FieldLogger) Warningln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// Warnln provides a mock function with given fields: args +func (_m *FieldLogger) Warnln(args ...interface{}) { + var _ca []interface{} + _ca = append(_ca, args...) + _m.Called(_ca...) +} + +// WithError provides a mock function with given fields: err +func (_m *FieldLogger) WithError(err error) *logrus.Entry { + ret := _m.Called(err) + + if len(ret) == 0 { + panic("no return value specified for WithError") + } + + var r0 *logrus.Entry + if rf, ok := ret.Get(0).(func(error) *logrus.Entry); ok { + r0 = rf(err) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*logrus.Entry) + } + } + + return r0 +} + +// WithField provides a mock function with given fields: key, value +func (_m *FieldLogger) WithField(key string, value interface{}) *logrus.Entry { + ret := _m.Called(key, value) + + if len(ret) == 0 { + panic("no return value specified for WithField") + } + + var r0 *logrus.Entry + if rf, ok := ret.Get(0).(func(string, interface{}) *logrus.Entry); ok { + r0 = rf(key, value) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*logrus.Entry) + } + } + + return r0 +} + +// WithFields provides a mock function with given fields: fields +func (_m *FieldLogger) WithFields(fields logrus.Fields) *logrus.Entry { + ret := _m.Called(fields) + + if len(ret) == 0 { + panic("no return value specified for WithFields") + } + + var r0 *logrus.Entry + if rf, ok := ret.Get(0).(func(logrus.Fields) *logrus.Entry); ok { + r0 = rf(fields) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*logrus.Entry) + } + } + + return r0 +} + +// NewFieldLogger creates a new instance of FieldLogger. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewFieldLogger(t interface { + mock.TestingT + Cleanup(func()) +}, +) *FieldLogger { + mock := &FieldLogger{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +}