From 0cec4b84e2e2385d33cd19351f8a9e098a29ecc2 Mon Sep 17 00:00:00 2001 From: badhezi Date: Wed, 28 May 2025 18:36:21 +0300 Subject: [PATCH] Fix actions skipped commit status indicator (#34507) Addresses https://github.com/go-gitea/gitea/issues/34500 Co-authored-by: wxiaoguang --- models/git/commit_status.go | 8 +- models/git/commit_status_summary.go | 6 +- modules/structs/commit_status.go | 20 +-- modules/structs/commit_status_test.go | 168 ++------------------ services/actions/commit_status.go | 4 +- services/convert/status.go | 15 +- services/pull/commit_status.go | 46 ++---- services/pull/commit_status_test.go | 90 ++++++----- templates/repo/commit_status.tmpl | 3 + web_src/js/components/DashboardRepoList.vue | 3 +- 10 files changed, 113 insertions(+), 250 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 0e4e8215b0..2e765391b8 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -230,18 +230,24 @@ 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. + // Frontend code (tmpl&vue) sometimes depend on the empty fields to skip rendering commit status elements (need to double check in the future) lastStatus = &CommitStatus{} } } diff --git a/models/git/commit_status_summary.go b/models/git/commit_status_summary.go index a3f440fd86..774e49bb98 100644 --- a/models/git/commit_status_summary.go +++ b/models/git/commit_status_summary.go @@ -59,7 +59,11 @@ func UpdateCommitStatusSummary(ctx context.Context, repoID int64, sha string) er if err != nil { return err } - state := CalcCommitStatus(commitStatuses) + // it guarantees that commitStatuses is not empty because this function is always called after a commit status is created + if len(commitStatuses) == 0 { + setting.PanicInDevOrTesting("no commit statuses found for repo %d and sha %s", repoID, sha) + } + state := CalcCommitStatus(commitStatuses) // non-empty commitStatuses is guaranteed // mysql will return 0 when update a record which state hasn't been changed which behaviour is different from other database, // so we need to use insert in on duplicate if setting.Database.Type.IsMySQL() { diff --git a/modules/structs/commit_status.go b/modules/structs/commit_status.go index dc880ef5eb..398001974d 100644 --- a/modules/structs/commit_status.go +++ b/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 diff --git a/modules/structs/commit_status_test.go b/modules/structs/commit_status_test.go index 88e09aadc1..c11daf248d 100644 --- a/modules/structs/commit_status_test.go +++ b/modules/structs/commit_status_test.go @@ -10,165 +10,21 @@ import ( ) 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.False(t, CommitStatusError.HasHigherPriorityThan(CommitStatusError)) } diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 4541b3af4c..5d17df8047 100644 --- a/services/actions/commit_status.go +++ b/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 } diff --git a/services/convert/status.go b/services/convert/status.go index 6cef63c1cd..4fffbfbe5e 100644 --- a/services/convert/status.go +++ b/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 } diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 553496713f..58d26c5a00 100644 --- a/services/pull/commit_status.go +++ b/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) diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 592acdd55c..9cb20d6c5d 100644 --- a/services/pull/commit_status_test.go +++ b/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) } } diff --git a/templates/repo/commit_status.tmpl b/templates/repo/commit_status.tmpl index eb700ab2bb..7184f5f8eb 100644 --- a/templates/repo/commit_status.tmpl +++ b/templates/repo/commit_status.tmpl @@ -14,3 +14,6 @@ {{if eq .State "warning"}} {{svg "gitea-exclamation" 18 "commit-status icon text yellow"}} {{end}} +{{if eq .State "skipped"}} + {{svg "octicon-skip" 18 "commit-status icon text grey"}} +{{end}} diff --git a/web_src/js/components/DashboardRepoList.vue b/web_src/js/components/DashboardRepoList.vue index fc6a7bd281..0528ceaf90 100644 --- a/web_src/js/components/DashboardRepoList.vue +++ b/web_src/js/components/DashboardRepoList.vue @@ -6,7 +6,7 @@ import {fomanticQuery} from '../modules/fomantic/base.ts'; const {appSubUrl, assetUrlPrefix, pageData} = window.config; -type CommitStatus = 'pending' | 'success' | 'error' | 'failure' | 'warning'; +type CommitStatus = 'pending' | 'success' | 'error' | 'failure' | 'warning' | 'skipped'; type CommitStatusMap = { [status in CommitStatus]: { @@ -22,6 +22,7 @@ const commitStatus: CommitStatusMap = { error: {name: 'gitea-exclamation', color: 'red'}, failure: {name: 'octicon-x', color: 'red'}, warning: {name: 'gitea-exclamation', color: 'yellow'}, + skipped: {name: 'octicon-skip', color: 'grey'}, }; export default defineComponent({