refactor improve NoBetterThan (#26126)

- The `NoBetterThan` function can only handle comparisons between
"pending," "success," "error," and "failure." For any other comparison,
we directly return false. This prevents logic errors like the one in
#26121.
- The callers of the `NoBetterThan` function should also avoid making
incomparable calls.

---------

Co-authored-by: yp05327 <576951401@qq.com>
Co-authored-by: puni9869 <80308335+puni9869@users.noreply.github.com>

Author: 3 people

modules/structs/commit_status.go

@@ -4,7 +4,7 @@
 package structs
 
 // CommitStatusState holds the state of a CommitStatus
-// It can be "pending", "success", "error", "failure", and "warning"
+// It can be "pending", "success", "error" and "failure"
 type CommitStatusState string
 
 const (
@@ -18,14 +18,25 @@ const (
 	CommitStatusFailure CommitStatusState = "failure"
 )
 
+var commitStatusPriorities = map[CommitStatusState]int{
+	CommitStatusError:   0,
+	CommitStatusFailure: 1,
+	CommitStatusPending: 2,
+	CommitStatusSuccess: 3,
+}
+
 // 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 {
-	commitStatusPriorities := map[CommitStatusState]int{
-		CommitStatusError:   0,
-		CommitStatusFailure: 1,
-		CommitStatusPending: 2,
-		CommitStatusSuccess: 3,
+	// NoBetterThan only handles the 4 states above
+	if _, exist := commitStatusPriorities[css]; !exist {
+		return false
 	}
+
+	if _, exist := commitStatusPriorities[css2]; !exist {
+		return false
+	}
+
 	return commitStatusPriorities[css] <= commitStatusPriorities[css2]
 }
 

modules/structs/commit_status_test.go

@@ -0,0 +1,174 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package structs
+
+import (
+	"testing"
+)
+
+func TestNoBetterThan(t *testing.T) {
+	type args struct {
+		css  CommitStatusState
+		css2 CommitStatusState
+	}
+	var unExpectedState CommitStatusState
+	tests := []struct {
+		name string
+		args args
+		want 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,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.args.css.NoBetterThan(tt.args.css2)
+			if result != tt.want {
+				t.Errorf("NoBetterThan() = %v, want %v", result, tt.want)
+			}
+		})
+	}
+}

services/convert/status.go

@@ -48,7 +48,7 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 	retStatus.Statuses = make([]*api.CommitStatus, 0, len(statuses))
 	for _, status := range statuses {
 		retStatus.Statuses = append(retStatus.Statuses, ToCommitStatus(ctx, status))
-		if status.State.NoBetterThan(retStatus.State) {
+		if retStatus.State == "" || status.State.NoBetterThan(retStatus.State) {
 			retStatus.State = status.State
 		}
 	}