Fix actions skipped commit status indicator (#34507)

Addresses https://github.com/go-gitea/gitea/issues/34500

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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

@@ -42,13 +42,14 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 		SHA:        statuses[0].SHA,
 		TotalCount: len(statuses),
 		Repository: repo,
-		URL:        "",
+		URL:        "", // never set or used?
+		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
 		}
 	}
@@ -57,9 +58,13 @@ func ToCombinedStatus(ctx context.Context, statuses []*git_model.CommitStatus, r
 	// > failure if any of the contexts report as error or failure
 	// > pending if there are no statuses or a context is pending
 	// > success if the latest status for all contexts is success
-	if retStatus.State.IsError() {
-		retStatus.State = api.CommitStatusFailure
+	switch retStatus.State {
+	case api.CommitStatusSkipped:
+		retStatus.State = api.CommitStatusSuccess // all skipped means success
+	case api.CommitStatusPending, api.CommitStatusSuccess:
+		// use the current state for pending or success
+	default:
+		retStatus.State = api.CommitStatusFailure // otherwise, it is a failure
 	}
-
 	return retStatus
 }

services/pull/commit_status.go

@@ -46,59 +46,33 @@ 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
 			}
 		}
 	}
 
 	if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess {
-		status := git_model.CalcCommitStatus(commitStatuses)
-		if status != nil {
-			return status.State
+		if len(commitStatuses) == 0 {
+			// "no statuses" should mean "pending"
+			return structs.CommitStatusPending
 		}
-		return structs.CommitStatusSuccess
+		status := git_model.CalcCommitStatus(commitStatuses)
+		if status.State == structs.CommitStatusSkipped {
+			return structs.CommitStatusSuccess // if all statuses are skipped, return success
+		}
+		return status.State
 	}
 
 	return returnedStatus
 }
 
-// IsCommitStatusContextSuccess returns true if all required status check contexts succeed.
-func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requiredContexts []string) bool {
-	// If no specific context is required, require that last commit status is a success
-	if len(requiredContexts) == 0 {
-		status := git_model.CalcCommitStatus(commitStatuses)
-		if status == nil || status.State != structs.CommitStatusSuccess {
-			return false
-		}
-		return true
-	}
-
-	for _, ctx := range requiredContexts {
-		var found bool
-		for _, commitStatus := range commitStatuses {
-			if commitStatus.Context == ctx {
-				if commitStatus.State != structs.CommitStatusSuccess {
-					return false
-				}
-
-				found = true
-				break
-			}
-		}
-		if !found {
-			return false
-		}
-	}
-	return true
-}
-
 // IsPullCommitStatusPass returns if all required status checks PASS
 func IsPullCommitStatusPass(ctx context.Context, pr *issues_model.PullRequest) (bool, error) {
 	pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)

services/pull/commit_status_test.go

@@ -14,52 +14,70 @@ import (
 )
 
 func TestMergeRequiredContextsCommitStatus(t *testing.T) {
-	testCases := [][]*git_model.CommitStatus{
+	cases := []struct {
+		commitStatuses   []*git_model.CommitStatus
+		requiredContexts []string
+		expected         structs.CommitStatusState
+	}{
 		{
-			{Context: "Build 1", State: structs.CommitStatusSuccess},
-			{Context: "Build 2", State: structs.CommitStatusSuccess},
-			{Context: "Build 3", State: structs.CommitStatusSuccess},
+			commitStatuses:   []*git_model.CommitStatus{},
+			requiredContexts: []string{},
+			expected:         structs.CommitStatusPending,
 		},
 		{
-			{Context: "Build 1", State: structs.CommitStatusSuccess},
-			{Context: "Build 2", State: structs.CommitStatusSuccess},
-			{Context: "Build 2t", State: structs.CommitStatusPending},
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build xxx", State: structs.CommitStatusSkipped},
+			},
+			requiredContexts: []string{"Build*"},
+			expected:         structs.CommitStatusSuccess,
 		},
 		{
-			{Context: "Build 1", State: structs.CommitStatusSuccess},
-			{Context: "Build 2", State: structs.CommitStatusSuccess},
-			{Context: "Build 2t", State: structs.CommitStatusFailure},
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build 1", State: structs.CommitStatusSkipped},
+				{Context: "Build 2", State: structs.CommitStatusSuccess},
+				{Context: "Build 3", State: structs.CommitStatusSuccess},
+			},
+			requiredContexts: []string{"Build*"},
+			expected:         structs.CommitStatusSuccess,
 		},
 		{
-			{Context: "Build 1", State: structs.CommitStatusSuccess},
-			{Context: "Build 2", State: structs.CommitStatusSuccess},
-			{Context: "Build 2t", State: structs.CommitStatusSuccess},
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build 1", State: structs.CommitStatusSuccess},
+				{Context: "Build 2", State: structs.CommitStatusSuccess},
+				{Context: "Build 2t", State: structs.CommitStatusPending},
+			},
+			requiredContexts: []string{"Build*", "Build 2t*"},
+			expected:         structs.CommitStatusPending,
 		},
 		{
-			{Context: "Build 1", State: structs.CommitStatusSuccess},
-			{Context: "Build 2", State: structs.CommitStatusSuccess},
-			{Context: "Build 2t", State: structs.CommitStatusSuccess},
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build 1", State: structs.CommitStatusSuccess},
+				{Context: "Build 2", State: structs.CommitStatusSuccess},
+				{Context: "Build 2t", State: structs.CommitStatusFailure},
+			},
+			requiredContexts: []string{"Build*", "Build 2t*"},
+			expected:         structs.CommitStatusFailure,
+		},
+		{
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build 1", State: structs.CommitStatusSuccess},
+				{Context: "Build 2", State: structs.CommitStatusSuccess},
+				{Context: "Build 2t", State: structs.CommitStatusSuccess},
+			},
+			requiredContexts: []string{"Build*", "Build 2t*", "Build 3*"},
+			expected:         structs.CommitStatusPending,
+		},
+		{
+			commitStatuses: []*git_model.CommitStatus{
+				{Context: "Build 1", State: structs.CommitStatusSuccess},
+				{Context: "Build 2", State: structs.CommitStatusSuccess},
+				{Context: "Build 2t", State: structs.CommitStatusSuccess},
+			},
+			requiredContexts: []string{"Build*", "Build *", "Build 2t*", "Build 1*"},
+			expected:         structs.CommitStatusSuccess,
 		},
 	}
-	testCasesRequiredContexts := [][]string{
-		{"Build*"},
-		{"Build*", "Build 2t*"},
-		{"Build*", "Build 2t*"},
-		{"Build*", "Build 2t*", "Build 3*"},
-		{"Build*", "Build *", "Build 2t*", "Build 1*"},
-	}
-
-	testCasesExpected := []structs.CommitStatusState{
-		structs.CommitStatusSuccess,
-		structs.CommitStatusPending,
-		structs.CommitStatusFailure,
-		structs.CommitStatusPending,
-		structs.CommitStatusSuccess,
-	}
-
-	for i, commitStatuses := range testCases {
-		if MergeRequiredContextsCommitStatus(commitStatuses, testCasesRequiredContexts[i]) != testCasesExpected[i] {
-			assert.Fail(t, "Test case failed", "Test case %d failed", i+1)
-		}
+	for i, c := range cases {
+		assert.Equal(t, c.expected, MergeRequiredContextsCommitStatus(c.commitStatuses, c.requiredContexts), "case %d", i)
 	}
 }