Fix actions skipped commit status indicator

models/git/commit_status.go

@@ -230,18 +230,23 @@ func (status *CommitStatus) HideActionsURL(ctx context.Context) {
 
 // CalcCommitStatus returns commit status state via some status, the commit statues should order by id desc
 func CalcCommitStatus(statuses []*CommitStatus) *CommitStatus {
+	// This function is widely used, but it is not quite right.
+	// Ideally it should return something like "CommitStatusSummary" with properly aggregated state.
+	// GitHub's behavior: if all statuses are "skipped", GitHub will return "success" as the combined status.
 	var lastStatus *CommitStatus
 	state := api.CommitStatusSuccess
 	for _, status := range statuses {
-		if status.State.NoBetterThan(state) {
+		if state == status.State || status.State.HasHigherPriorityThan(state) {
 			state = status.State
 			lastStatus = status
 		}
 	}
 	if lastStatus == nil {
 		if len(statuses) > 0 {
+			// FIXME: a bad case: Gitea just returns the first commit status, its status is "skipped" in this case.
 			lastStatus = statuses[0]
 		} else {
+			// FIXME: another bad case: if the "statuses" slice is empty, the returned value is an invalid CommitStatus, all its fields are empty.
 			lastStatus = &CommitStatus{}
 		}
 	}

modules/structs/commit_status.go

@@ -18,6 +18,8 @@ const (
 	CommitStatusFailure CommitStatusState = "failure"
 	// CommitStatusWarning is for when the CommitStatus is Warning
 	CommitStatusWarning CommitStatusState = "warning"
+	// CommitStatusSkipped is for when CommitStatus is Skipped
+	CommitStatusSkipped CommitStatusState = "skipped"
 )
 
 var commitStatusPriorities = map[CommitStatusState]int{
@@ -26,25 +28,17 @@ var commitStatusPriorities = map[CommitStatusState]int{
 	CommitStatusWarning: 2,
 	CommitStatusPending: 3,
 	CommitStatusSuccess: 4,
+	CommitStatusSkipped: 5,
 }
 
 func (css CommitStatusState) String() string {
 	return string(css)
 }
 
-// NoBetterThan returns true if this State is no better than the given State
-// This function only handles the states defined in CommitStatusPriorities
-func (css CommitStatusState) NoBetterThan(css2 CommitStatusState) bool {
-	// NoBetterThan only handles the 5 states above
-	if _, exist := commitStatusPriorities[css]; !exist {
-		return false
-	}
-
-	if _, exist := commitStatusPriorities[css2]; !exist {
-		return false
-	}
-
-	return commitStatusPriorities[css] <= commitStatusPriorities[css2]
+// HasHigherPriorityThan returns true if this state has higher priority than the other
+// Undefined states are considered to have the highest priority like CommitStatusError(0)
+func (css CommitStatusState) HasHigherPriorityThan(other CommitStatusState) bool {
+	return commitStatusPriorities[css] < commitStatusPriorities[other]
 }
 
 // IsPending represents if commit status state is pending

modules/structs/commit_status_test.go

@@ -10,165 +10,21 @@
 )
 
 func TestNoBetterThan(t *testing.T) {
-	type args struct {
-		css  CommitStatusState
-		css2 CommitStatusState
-	}
-	var unExpectedState CommitStatusState
 	tests := []struct {
-		name string
-		args args
-		want bool
+		s1, s2 CommitStatusState
+		higher bool
 	}{
-		{
-			name: "success is no better than success",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "success is no better than pending",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusPending,
-			},
-			want: false,
-		},
-		{
-			name: "success is no better than failure",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusFailure,
-			},
-			want: false,
-		},
-		{
-			name: "success is no better than error",
-			args: args{
-				css:  CommitStatusSuccess,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "pending is no better than success",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "pending is no better than pending",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "pending is no better than failure",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusFailure,
-			},
-			want: false,
-		},
-		{
-			name: "pending is no better than error",
-			args: args{
-				css:  CommitStatusPending,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "failure is no better than success",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than pending",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than failure",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusFailure,
-			},
-			want: true,
-		},
-		{
-			name: "failure is no better than error",
-			args: args{
-				css:  CommitStatusFailure,
-				css2: CommitStatusError,
-			},
-			want: false,
-		},
-		{
-			name: "error is no better than success",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusSuccess,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than pending",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusPending,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than failure",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusFailure,
-			},
-			want: true,
-		},
-		{
-			name: "error is no better than error",
-			args: args{
-				css:  CommitStatusError,
-				css2: CommitStatusError,
-			},
-			want: true,
-		},
-		{
-			name: "unExpectedState is no better than success",
-			args: args{
-				css:  unExpectedState,
-				css2: CommitStatusSuccess,
-			},
-			want: false,
-		},
-		{
-			name: "unExpectedState is no better than unExpectedState",
-			args: args{
-				css:  unExpectedState,
-				css2: unExpectedState,
-			},
-			want: false,
-		},
+		{CommitStatusError, CommitStatusFailure, true},
+		{CommitStatusFailure, CommitStatusWarning, true},
+		{CommitStatusWarning, CommitStatusPending, true},
+		{CommitStatusPending, CommitStatusSuccess, true},
+		{CommitStatusSuccess, CommitStatusSkipped, true},
+
+		{CommitStatusError, "unknown-xxx", false},
+		{"unknown-xxx", CommitStatusFailure, true},
 	}
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := tt.args.css.NoBetterThan(tt.args.css2)
-			assert.Equal(t, tt.want, result)
-		})
+		assert.Equal(t, tt.higher, tt.s1.HasHigherPriorityThan(tt.s2), "s1=%s, s2=%s, expected=%v", tt.s1, tt.s2, tt.higher)
 	}
+	assert.Equal(t, false, CommitStatusError.HasHigherPriorityThan(CommitStatusError))
 }

services/actions/commit_status.go

@@ -149,12 +149,14 @@ func createCommitStatus(ctx context.Context, job *actions_model.ActionRunJob) er
 
 func toCommitStatus(status actions_model.Status) api.CommitStatusState {
 	switch status {
-	case actions_model.StatusSuccess, actions_model.StatusSkipped:
+	case actions_model.StatusSuccess:
 		return api.CommitStatusSuccess
 	case actions_model.StatusFailure, actions_model.StatusCancelled:
 		return api.CommitStatusFailure
 	case actions_model.StatusWaiting, actions_model.StatusBlocked, actions_model.StatusRunning:
 		return api.CommitStatusPending
+	case actions_model.StatusSkipped:
+		return api.CommitStatusSkipped
 	default:
 		return api.CommitStatusError
 	}

services/convert/status.go

@@ -43,12 +43,13 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 		TotalCount: len(statuses),
 		Repository: repo,
 		URL:        "",
+		State:      api.CommitStatusSuccess,
 	}
 
 	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
 	for _, status := range statuses {
 		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
-		if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) {
+		if status.State.HasHigherPriorityThan(retStatus.State) {
 			retStatus.State = status.State
 		}
 	}

services/pull/commit_status.go

@@ -46,13 +46,13 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 
 			// If required rule not match any action, then it is pending
 			if targetStatus == "" {
-				if structs.CommitStatusPending.NoBetterThan(returnedStatus) {
+				if structs.CommitStatusPending.HasHigherPriorityThan(returnedStatus) {
 					returnedStatus = structs.CommitStatusPending
 				}
 				break
 			}
 
-			if targetStatus.NoBetterThan(returnedStatus) {
+			if targetStatus.HasHigherPriorityThan(returnedStatus) {
 				returnedStatus = targetStatus
 			}
 		}
@@ -61,6 +61,11 @@ func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus,
 	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
 		status := git_model.CalcCommitStatus(commitStatuses)
 		if status != nil {
+			// FIXME: this check is not right, "status" can never be nil, but its fields can be empty if commitStatuses is empty
+			// here is just a quick patch to make it overall right.
+			if status.State == "" || status.State == structs.CommitStatusSkipped {
+				return structs.CommitStatusSuccess
+			}
 			return status.State
 		}
 		return structs.CommitStatusSuccess