From b8f8b0f6744016331cdae79079e4365a0fca8b5e Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Wed, 20 Aug 2025 22:02:43 +0000 Subject: [PATCH] fix: an error from a SKIPPED job does not transform it into a FAILURE (#896) The result of a job can only be changed to FAILURE by the Close() function of the reporter under two conditions: - If it is UNSPECIFIED - If the context timeout In all other cases it must be preserved. It is possible, for instance, for a job to be SKIPPED and be associated with an error message, e.g. ``` time="2025-08-20T21:02:07+02:00" level=trace msg="evaluating expression 'success()'" dryrun=false job="Test Action/job-1" jobID=job-1 matrix="map[]" [Test Action/job-1] [DEBUG] evaluating expression 'success()' time="2025-08-20T21:02:07+02:00" level=trace msg="expression 'success()' evaluated to 'true'" dryrun=false job="Test Action/job-1" jobID=job-1 matrix="map[]" [Test Action/job-1] [DEBUG] expression 'success()' evaluated to 'true' time="2025-08-20T21:02:07+02:00" level=trace msg="'runs-on' key not defined in Test Action/job-1" dryrun=false job="Test Action/job-1" jobID=job-1 matrix="map[]" [Test Action/job-1] 'runs-on' key not defined in Test Action/job-1 [Test Action/job-1] [DEBUG] No steps found ``` Those errors show in the logs when a job is skipped, because the worflow is empty. But they are expected and to be ignored. Refs forgejo/runner#895 - bug fixes - [PR](https://code.forgejo.org/forgejo/runner/pulls/896): fix: an error from a SKIPPED job does not transform it into a FAILURE Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/896 Reviewed-by: Mathieu Fenniak Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- internal/pkg/report/reporter.go | 16 ++++--- internal/pkg/report/reporter_test.go | 70 +++++++++++++++------------- 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/internal/pkg/report/reporter.go b/internal/pkg/report/reporter.go index a02f0e72..42afe8e0 100644 --- a/internal/pkg/report/reporter.go +++ b/internal/pkg/report/reporter.go @@ -244,26 +244,30 @@ const ( func (r *Reporter) Close(runErr error) error { r.closed = true + setStepCancel := func() { + for _, v := range r.state.Steps { + if v.Result == runnerv1.Result_RESULT_UNSPECIFIED { + v.Result = runnerv1.Result_RESULT_CANCELLED + } + } + } + r.stateMu.Lock() var lastWords string if errors.Is(runErr, context.DeadlineExceeded) { lastWords = closeTimeoutMessage r.state.Result = runnerv1.Result_RESULT_CANCELLED + setStepCancel() } else if r.state.Result == runnerv1.Result_RESULT_UNSPECIFIED { if runErr == nil { lastWords = closeCancelledMessage } else { lastWords = runErr.Error() } - for _, v := range r.state.Steps { - if v.Result == runnerv1.Result_RESULT_UNSPECIFIED { - v.Result = runnerv1.Result_RESULT_CANCELLED - } - } r.state.Result = runnerv1.Result_RESULT_FAILURE + setStepCancel() } else if runErr != nil { lastWords = runErr.Error() - r.state.Result = runnerv1.Result_RESULT_FAILURE } if lastWords != "" { diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index 40af3e30..246d1699 100644 --- a/internal/pkg/report/reporter_test.go +++ b/internal/pkg/report/reporter_test.go @@ -410,7 +410,8 @@ func TestReporterReportLog(t *testing.T) { } func TestReporterClose(t *testing.T) { - mockReporterCloser := func(message *string, result *runnerv1.Result) *Reporter { + mockReporterCloser := func(t *testing.T, message *string, result *runnerv1.Result) *Reporter { + t.Helper() reporter, client, _ := mockReporter(t) if message != nil { client.On("UpdateLog", mock.Anything, mock.Anything).Return(func(_ context.Context, req *connect_go.Request[runnerv1.UpdateLogRequest]) (*connect_go.Response[runnerv1.UpdateLogResponse], error) { @@ -435,46 +436,50 @@ func TestReporterClose(t *testing.T) { } for _, testCase := range []struct { - name string - err error - expectedMessage string - result runnerv1.Result - expectedResult runnerv1.Result + name string + err error + expectedMessage string + result runnerv1.Result + expectedStateResult runnerv1.Result + expectedStepStateResult runnerv1.Result }{ { - name: "ResultSuccessAndNilErrorIsResultSuccess", - err: nil, - expectedMessage: "", - result: runnerv1.Result_RESULT_SUCCESS, - expectedResult: runnerv1.Result_RESULT_SUCCESS, + name: "ResultSuccessAndNilErrorIsResultSuccess", + err: nil, + expectedMessage: "", + result: runnerv1.Result_RESULT_SUCCESS, + expectedStateResult: runnerv1.Result_RESULT_SUCCESS, }, { - name: "ResultUnspecifiedAndErrorIsResultFailure", - err: errors.New("ERROR_MESSAGE"), - expectedMessage: "ERROR_MESSAGE", - result: runnerv1.Result_RESULT_UNSPECIFIED, - expectedResult: runnerv1.Result_RESULT_FAILURE, + name: "ResultUnspecifiedAndErrorIsResultFailure", + err: errors.New("ERROR_MESSAGE"), + expectedMessage: "ERROR_MESSAGE", + result: runnerv1.Result_RESULT_UNSPECIFIED, + expectedStateResult: runnerv1.Result_RESULT_FAILURE, + expectedStepStateResult: runnerv1.Result_RESULT_CANCELLED, }, { - name: "ResultUnspecifiedAndNilErrorIsResultFailure", - err: nil, - expectedMessage: closeCancelledMessage, - result: runnerv1.Result_RESULT_UNSPECIFIED, - expectedResult: runnerv1.Result_RESULT_FAILURE, + name: "ResultUnspecifiedAndNilErrorIsResultFailure", + err: nil, + expectedMessage: closeCancelledMessage, + result: runnerv1.Result_RESULT_UNSPECIFIED, + expectedStateResult: runnerv1.Result_RESULT_FAILURE, + expectedStepStateResult: runnerv1.Result_RESULT_CANCELLED, }, { - name: "ResultSuccessAndErrorIsResultFailure", - err: errors.New("ERROR_MESSAGE"), - expectedMessage: "ERROR_MESSAGE", - result: runnerv1.Result_RESULT_SUCCESS, - expectedResult: runnerv1.Result_RESULT_FAILURE, + name: "ResultSkippedAndErrorIsResultSkipped", + err: errors.New("ERROR_MESSAGE"), + expectedMessage: "ERROR_MESSAGE", + result: runnerv1.Result_RESULT_SKIPPED, + expectedStateResult: runnerv1.Result_RESULT_SKIPPED, }, { - name: "Timeout", - err: context.DeadlineExceeded, - expectedMessage: closeTimeoutMessage, - result: runnerv1.Result_RESULT_UNSPECIFIED, - expectedResult: runnerv1.Result_RESULT_CANCELLED, + name: "Timeout", + err: context.DeadlineExceeded, + expectedMessage: closeTimeoutMessage, + result: runnerv1.Result_RESULT_UNSPECIFIED, + expectedStateResult: runnerv1.Result_RESULT_CANCELLED, + expectedStepStateResult: runnerv1.Result_RESULT_CANCELLED, }, } { t.Run(testCase.name, func(t *testing.T) { @@ -482,7 +487,7 @@ func TestReporterClose(t *testing.T) { if testCase.expectedMessage != "" { message = &testCase.expectedMessage } - reporter := mockReporterCloser(message, &testCase.expectedResult) + reporter := mockReporterCloser(t, message, &testCase.expectedStateResult) // cancel() verifies Close can operate after the context is cancelled // because it uses the daemon context instead @@ -494,6 +499,7 @@ func TestReporterClose(t *testing.T) { }, } require.NoError(t, reporter.Close(testCase.err)) + assert.Equal(t, testCase.expectedStepStateResult, reporter.state.Steps[0].Result) }) } }