From 22c4599542ee3e10bcab4c9136467bbac8e90ba0 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Mon, 16 Dec 2024 21:49:53 +0800
Subject: [PATCH] Improve Actions status aggregations (#32860)

Make the result the same as GitHub:

* all skipped, then result is skipped
* any cancelled, then result cancelled
---
 models/actions/run_job.go             | 15 ++++++++-------
 models/actions/run_job_status_test.go | 25 +++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/models/actions/run_job.go b/models/actions/run_job.go
index 8c131351d5..de4b6aab66 100644
--- a/models/actions/run_job.go
+++ b/models/actions/run_job.go
@@ -153,20 +153,25 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col
 }
 
 func AggregateJobStatus(jobs []*ActionRunJob) Status {
-	allSuccessOrSkipped := true
-	var hasFailure, hasCancelled, hasSkipped, hasWaiting, hasRunning, hasBlocked bool
+	allSuccessOrSkipped := len(jobs) != 0
+	allSkipped := len(jobs) != 0
+	var hasFailure, hasCancelled, hasWaiting, hasRunning, hasBlocked bool
 	for _, job := range jobs {
 		allSuccessOrSkipped = allSuccessOrSkipped && (job.Status == StatusSuccess || job.Status == StatusSkipped)
+		allSkipped = allSkipped && job.Status == StatusSkipped
 		hasFailure = hasFailure || job.Status == StatusFailure
 		hasCancelled = hasCancelled || job.Status == StatusCancelled
-		hasSkipped = hasSkipped || job.Status == StatusSkipped
 		hasWaiting = hasWaiting || job.Status == StatusWaiting
 		hasRunning = hasRunning || job.Status == StatusRunning
 		hasBlocked = hasBlocked || job.Status == StatusBlocked
 	}
 	switch {
+	case allSkipped:
+		return StatusSkipped
 	case allSuccessOrSkipped:
 		return StatusSuccess
+	case hasCancelled:
+		return StatusCancelled
 	case hasFailure:
 		return StatusFailure
 	case hasRunning:
@@ -175,10 +180,6 @@ func AggregateJobStatus(jobs []*ActionRunJob) Status {
 		return StatusWaiting
 	case hasBlocked:
 		return StatusBlocked
-	case hasCancelled:
-		return StatusCancelled
-	case hasSkipped:
-		return StatusSkipped
 	default:
 		return StatusUnknown // it shouldn't happen
 	}
diff --git a/models/actions/run_job_status_test.go b/models/actions/run_job_status_test.go
index bac480a96b..04fd9ceba7 100644
--- a/models/actions/run_job_status_test.go
+++ b/models/actions/run_job_status_test.go
@@ -11,6 +11,7 @@ import (
 
 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})
@@ -29,6 +30,16 @@ func TestAggregateJobStatus(t *testing.T) {
 		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
@@ -38,18 +49,28 @@ func TestAggregateJobStatus(t *testing.T) {
 		{[]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}, StatusFailure},
+		{[]Status{StatusFailure, StatusCancelled}, StatusCancelled},
 		{[]Status{StatusFailure, StatusWaiting}, StatusFailure},
 		{[]Status{StatusFailure, StatusRunning}, StatusFailure},
 		{[]Status{StatusFailure, StatusBlocked}, StatusFailure},
 
 		// skipped with other status
-		{[]Status{StatusSkipped}, StatusSuccess},
+		// 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},