fix(actions): address remaining review points — ownership docs, log levels, tests

- Expand NewInterpeter ownership comment to explicitly confirm that
  run.Workflow is fresh per call and job is only read (not written),
  so concurrent calls for different matrix combinations are race-free
- Downgrade the two remaining log.Info lines (matrix re-evaluation
  complete / succeeded) to log.Debug per reviewer request
- Add unit tests for constructWorkflowWithNeeds (happy path, invalid
  payload, missing jobs section), mergeNeedsIntoVars, and a
  multi-dimension dynamic matrix case in TestHasMatrixWithNeeds

Co-Authored-By: Claude <claude-sonnet-4-5@anthropic.com>
This commit is contained in:
ZPascal
2026-05-26 16:55:27 +02:00
committed by I539231
co-authored by Claude
parent 7ab7f69bb0
commit b0f2477ced
4 changed files with 118 additions and 3 deletions
+5 -1
View File
@@ -34,7 +34,11 @@ func NewInterpeter(
JobID: jobID,
}
// Add the current job to the workflow so run.Job() doesn't return nil
// run.Workflow is allocated above and never shared: each call to NewInterpeter
// creates its own *model.Run. Writing job into the fresh Jobs map here is
// therefore safe even when NewInterpeter is called concurrently for different
// matrix combinations of the same job. The job pointer itself is only read
// (for Strategy and Needs()), never written through.
run.Workflow.Jobs[jobID] = job
for id, result := range results {
+1 -1
View File
@@ -412,7 +412,7 @@ func (r *jobStatusResolver) resolve(ctx context.Context) map[int64]actions_model
// more to do here — just move to the next job.
if len(newMatrixJobs) > 0 {
resolveMetrics.matrixReevaluated++
log.Info("Matrix re-evaluation succeeded for job %d (JobID: %s): created %d new jobs (duration: %dms)",
log.Debug("Matrix re-evaluation succeeded for job %d (JobID: %s): created %d new jobs (duration: %dms)",
id, actionRunJob.JobID, len(newMatrixJobs), duration)
continue
}
+1 -1
View File
@@ -344,7 +344,7 @@ func ReEvaluateMatrixForJobWithNeeds(ctx context.Context, job *actions_model.Act
return nil, nil
}
log.Info("Matrix re-evaluation complete for job %d (JobID: %s): created %d new jobs",
log.Debug("Matrix re-evaluation complete for job %d (JobID: %s): created %d new jobs",
job.ID, job.JobID, len(newJobs))
return newJobs, nil
+111
View File
@@ -6,7 +6,11 @@ package actions
import (
"testing"
actions_model "code.gitea.io/gitea/models/actions"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.yaml.in/yaml/v4"
)
func TestHasMatrixWithNeeds(t *testing.T) {
@@ -67,6 +71,15 @@ fail-fast: false
`,
want: false,
},
{
name: "multi-dimension dynamic matrix",
strategy: `
matrix:
os: [ubuntu-latest, windows-latest]
version: ${{ fromJson(needs.setup.outputs.versions) }}
`,
want: true,
},
}
for _, tt := range tests {
@@ -75,3 +88,101 @@ fail-fast: false
})
}
}
func TestMergeNeedsIntoVars(t *testing.T) {
base := map[string]string{"MY_VAR": "hello"}
needs := map[string]*TaskNeed{
"setup": {
Result: actions_model.StatusSuccess,
Outputs: map[string]string{"versions": `["1","2"]`, "extra": "val"},
},
}
merged := mergeNeedsIntoVars(base, needs)
assert.Equal(t, "hello", merged["MY_VAR"])
assert.Equal(t, `["1","2"]`, merged["needs.setup.outputs.versions"])
assert.Equal(t, "val", merged["needs.setup.outputs.extra"])
// base must not be mutated
assert.NotContains(t, base, "needs.setup.outputs.versions")
}
func TestConstructWorkflowWithNeeds(t *testing.T) {
// Minimal WorkflowPayload with a strategy referencing a needs output.
payload, err := yaml.Marshal(map[string]any{
"jobs": map[string]any{
"build": map[string]any{
"runs-on": "ubuntu-latest",
"strategy": map[string]any{
"matrix": map[string]any{
"version": `${{ fromJson(needs.setup.outputs.versions) }}`,
},
},
"steps": []any{},
},
},
})
require.NoError(t, err)
job := &actions_model.ActionRunJob{
JobID: "build",
WorkflowPayload: payload,
RawStrategy: `
matrix:
version: ${{ fromJson(needs.setup.outputs.versions) }}
`,
Needs: []string{"setup"},
}
needs := map[string]*TaskNeed{
"setup": {
Result: actions_model.StatusSuccess,
Outputs: map[string]string{"versions": `["1.20","1.21"]`},
},
}
out, err := constructWorkflowWithNeeds(job, needs)
require.NoError(t, err)
// The resulting YAML should contain both the build job and a stub for setup.
var wf map[string]any
require.NoError(t, yaml.Unmarshal(out, &wf))
jobs, ok := wf["jobs"].(map[string]any)
require.True(t, ok)
assert.Contains(t, jobs, "build")
assert.Contains(t, jobs, "setup", "stub for needs dependency must be present")
// build job needs must list the dependency
buildJob, ok := jobs["build"].(map[string]any)
require.True(t, ok)
assert.Contains(t, buildJob, "needs")
// RawStrategy must be re-injected (not the pre-baked array form)
strategy, ok := buildJob["strategy"].(map[string]any)
require.True(t, ok)
matrix, ok := strategy["matrix"].(map[string]any)
require.True(t, ok)
versionExpr, ok := matrix["version"].(string)
require.True(t, ok)
assert.Contains(t, versionExpr, "fromJson")
}
func TestConstructWorkflowWithNeeds_InvalidPayload(t *testing.T) {
job := &actions_model.ActionRunJob{
JobID: "build",
WorkflowPayload: []byte("not: valid: yaml: ["),
}
_, err := constructWorkflowWithNeeds(job, nil)
assert.Error(t, err)
}
func TestConstructWorkflowWithNeeds_MissingJobsSection(t *testing.T) {
payload, err := yaml.Marshal(map[string]any{"name": "test"})
require.NoError(t, err)
job := &actions_model.ActionRunJob{
JobID: "build",
WorkflowPayload: payload,
}
_, err = constructWorkflowWithNeeds(job, nil)
assert.Error(t, err)
}