Merge pull request '[gitea] week 2024-52 cherry pick (gitea/main -> forgejo)' (#6342) from earl-warren/wcp/2024-52 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6342
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

4 files changed, 141 insertions, 19 deletions

diff --git a/models/actions/run_job.go b/models/actions/run_job.go
index 2319af8e08..de4b6aab66 100644
--- a/models/actions/run_job.go
+++ b/models/actions/run_job.go
@@ -153,28 +153,34 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col
 }
 
 func AggregateJobStatus(jobs []*ActionRunJob) Status {
-	allDone := true
-	allWaiting := true
-	hasFailure := false
+	allSuccessOrSkipped := len(jobs) != 0
+	allSkipped := len(jobs) != 0
+	var hasFailure, hasCancelled, hasWaiting, hasRunning, hasBlocked bool
 	for _, job := range jobs {
-		if !job.Status.IsDone() {
-			allDone = false
-		}
-		if job.Status != StatusWaiting && !job.Status.IsDone() {
-			allWaiting = false
-		}
-		if job.Status == StatusFailure || job.Status == StatusCancelled {
-			hasFailure = true
-		}
+		allSuccessOrSkipped = allSuccessOrSkipped && (job.Status == StatusSuccess || job.Status == StatusSkipped)
+		allSkipped = allSkipped && job.Status == StatusSkipped
+		hasFailure = hasFailure || job.Status == StatusFailure
+		hasCancelled = hasCancelled || job.Status == StatusCancelled
+		hasWaiting = hasWaiting || job.Status == StatusWaiting
+		hasRunning = hasRunning || job.Status == StatusRunning
+		hasBlocked = hasBlocked || job.Status == StatusBlocked
 	}
-	if allDone {
-		if hasFailure {
-			return StatusFailure
-		}
+	switch {
+	case allSkipped:
+		return StatusSkipped
+	case allSuccessOrSkipped:
 		return StatusSuccess
-	}
-	if allWaiting {
+	case hasCancelled:
+		return StatusCancelled
+	case hasFailure:
+		return StatusFailure
+	case hasRunning:
+		return StatusRunning
+	case hasWaiting:
 		return StatusWaiting
+	case hasBlocked:
+		return StatusBlocked
+	default:
+		return StatusUnknown // it shouldn't happen
 	}
-	return StatusRunning
 }
diff --git a/models/actions/run_job_status_test.go b/models/actions/run_job_status_test.go
new file mode 100644
index 0000000000..04fd9ceba7
--- /dev/null
+++ b/models/actions/run_job_status_test.go
@@ -0,0 +1,85 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package actions
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAggregateJobStatus(t *testing.T) {
+	testStatuses := func(expected Status, statuses []Status) {
+		t.Helper()
+		var jobs []*ActionRunJob
+		for _, v := range statuses {
+			jobs = append(jobs, &ActionRunJob{Status: v})
+		}
+		actual := AggregateJobStatus(jobs)
+		if !assert.Equal(t, expected, actual) {
+			var statusStrings []string
+			for _, s := range statuses {
+				statusStrings = append(statusStrings, s.String())
+			}
+			t.Errorf("AggregateJobStatus(%v) = %v, want %v", statusStrings, statusNames[actual], statusNames[expected])
+		}
+	}
+
+	cases := []struct {
+		statuses []Status
+		expected Status
+	}{
+		// unknown cases, maybe it shouldn't happen in real world
+		{[]Status{}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusSuccess}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusSkipped}, StatusUnknown},
+		{[]Status{StatusUnknown, StatusFailure}, StatusFailure},
+		{[]Status{StatusUnknown, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusUnknown, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusUnknown, StatusRunning}, StatusRunning},
+		{[]Status{StatusUnknown, StatusBlocked}, StatusBlocked},
+
+		// success with other status
+		{[]Status{StatusSuccess}, StatusSuccess},
+		{[]Status{StatusSuccess, StatusSkipped}, StatusSuccess}, // skipped doesn't affect success
+		{[]Status{StatusSuccess, StatusFailure}, StatusFailure},
+		{[]Status{StatusSuccess, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusSuccess, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusSuccess, StatusRunning}, StatusRunning},
+		{[]Status{StatusSuccess, StatusBlocked}, StatusBlocked},
+
+		// any cancelled, then cancelled
+		{[]Status{StatusCancelled}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusSuccess}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusSkipped}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusFailure}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusWaiting}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusRunning}, StatusCancelled},
+		{[]Status{StatusCancelled, StatusBlocked}, StatusCancelled},
+
+		// failure with other status, fail fast
+		// Should "running" win? Maybe no: old code does make "running" win, but GitHub does fail fast.
+		{[]Status{StatusFailure}, StatusFailure},
+		{[]Status{StatusFailure, StatusSuccess}, StatusFailure},
+		{[]Status{StatusFailure, StatusSkipped}, StatusFailure},
+		{[]Status{StatusFailure, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusFailure, StatusWaiting}, StatusFailure},
+		{[]Status{StatusFailure, StatusRunning}, StatusFailure},
+		{[]Status{StatusFailure, StatusBlocked}, StatusFailure},
+
+		// skipped with other status
+		// TODO: need to clarify whether a PR with "skipped" job status is considered as "mergeable" or not.
+		{[]Status{StatusSkipped}, StatusSkipped},
+		{[]Status{StatusSkipped, StatusSuccess}, StatusSuccess},
+		{[]Status{StatusSkipped, StatusFailure}, StatusFailure},
+		{[]Status{StatusSkipped, StatusCancelled}, StatusCancelled},
+		{[]Status{StatusSkipped, StatusWaiting}, StatusWaiting},
+		{[]Status{StatusSkipped, StatusRunning}, StatusRunning},
+		{[]Status{StatusSkipped, StatusBlocked}, StatusBlocked},
+	}
+
+	for _, c := range cases {
+		testStatuses(c.expected, c.statuses)
+	}
+}
diff --git a/models/fixtures/action_run.yml b/models/fixtures/action_run.yml
index 2fe9094d13..7a7bf34197 100644
--- a/models/fixtures/action_run.yml
+++ b/models/fixtures/action_run.yml
@@ -433,6 +433,25 @@
   need_approval: 0
   approved_by: 0
 -
+  id: 794
+  title: "job output"
+  repo_id: 4
+  owner_id: 1
+  workflow_id: "test.yaml"
+  index: 190
+  trigger_user_id: 1
+  ref: "refs/heads/test"
+  commit_sha: "c2d72f548424103f01ee1dc02889c1e2bff816b0"
+  event: "push"
+  is_fork_pull_request: 0
+  status: 1
+  started: 1683636528
+  stopped: 1683636626
+  created: 1683636108
+  updated: 1683636626
+  need_approval: 0
+  approved_by: 0
+-
   id: 891
   title: "update actions"
   repo_id: 1
diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml
index 93003049c6..b75d3706cc 100644
--- a/models/fixtures/branch.yml
+++ b/models/fixtures/branch.yml
@@ -45,3 +45,15 @@
   is_deleted: false
   deleted_by_id: 0
   deleted_unix: 0
+
+-
+  id: 15
+  repo_id: 4
+  name: 'master'
+  commit_id: 'c7cd3cd144e6d23c9d6f3d07e52b2c1a956e0338'
+  commit_message: 'add Readme'
+  commit_time: 1588147171
+  pusher_id: 13
+  is_deleted: false
+  deleted_by_id: 0
+  deleted_unix: 0