Fix incomplete Actions status aggregations

[wxiaoguang]

fix #32857

[TheFox0x7]

@wxiaoguang why bundle skip with success? Aren't they two different states when combined?
A workflow where all jobs where skipped is aggregated to skip and when one job had a different status it's marked with that "stronger" status.
Sorry for being late on this, I thought there will be some kind of discussion before I'd start porting my PR but you beat me to it.

[wxiaoguang]

Sorry for being late on this, I thought there will be some kind of discussion before I'd start porting my PR but you beat me to it.

Sorry that I didn't know you have a PR to port 🤣 you can tell that in the issue then I won't do it next time.

why bundle skip with success? Aren't they two different states when combined?
A workflow where all jobs where skipped is aggregated to skip and when one job had a different status it's marked with that "stronger" status.

The reason is that "skipped" shouldn't affect "success", just like GitHub:

Update: "all jobs where skipped is aggregated to skip": I think it is right to treat it as "success" since nothing need to do (all skipped), so "success"? Does it make sense?

[wxiaoguang]

Hmm, I see your demo https://github.com/TheFox0x7/repro/actions/workflows/skip.yaml

"skipped-only" is aggregated as "skipped".

So maybe we should follow it (add a allSkipped check and return early if allSkipped).

ps: I am not sure whether it would affect other logics, eg: if "all skipped", should that PR be considered as "mergeable"?

[TheFox0x7]

Sorry that I didn't know you have a PR to port 🤣 you can tell that in the issue then I won't do it next time.

No worries, I should've mentioned that. I sort of assumed someone would click the closed (but not merged) PR in the downstream issue which I'll admit it's hardly obvious so that's fully on me.

I can check how the skipped status works here but I'll need a bit of time to do it. I'll try and demonstrate the waiting and blocked states as well (I think i manged to do it earlier but no promises on that).
Guessing only - I'd treat skip as "success" (or rather not a fail) for any operations.

---

written before your new comment
I do see your point, but on the other hand it's also not a success since it didn't run at all. Having all jobs skipped should be (IMO) displayed as such but not block anything (it's didn't fail after all).
To illustrate my point and looking only at completed jobs:

• statuses are either Skipped or Success - they should be displayed accordingly but for any operations treated as if everything passed.
• statuses have a cancel or failure in the mix, so they should be displayed as overall result

---

Here's a test written for my original PR (with one test case added for pure skips)

```go package actions

import (
"math/rand/v2"
"testing"

"github.com/stretchr/testify/assert"

)

func TestJobStatusAggregation(t *testing.T) {
testCases := []struct {
desc string
runStatuses []Status
expectedStatus Status
}{
{
desc: "All jobs lower than skipped - aggregate skip",
runStatuses: []Status{StatusUnknown, StatusSkipped},
expectedStatus: StatusSkipped,
},
{
desc: "All jobs skipped - aggregate skip",
runStatuses: []Status{StatusSkipped},
expectedStatus: StatusSkipped,
},
{
desc: "Job success, no fails - aggregate success",
runStatuses: []Status{StatusSuccess, StatusSkipped, StatusUnknown},
expectedStatus: StatusSuccess,
},
{
desc: "Any job failure - aggregate failure",
runStatuses: []Status{StatusSuccess, StatusFailure, StatusSkipped, StatusUnknown},
expectedStatus: StatusFailure,
},
{
desc: "Any job cancelled - aggregate cancelled",
runStatuses: []Status{StatusSuccess, StatusFailure, StatusCancelled, StatusSkipped, StatusUnknown},
expectedStatus: StatusCancelled,
},
{
desc: "Any job running - aggregate running",
runStatuses: []Status{StatusSuccess, StatusUnknown, StatusFailure, StatusCancelled, StatusSkipped, StatusWaiting, StatusRunning},
expectedStatus: StatusRunning,
},
{
desc: "No jobs running, jobs waiting - aggregate waiting",
runStatuses: []Status{StatusSuccess, StatusFailure, StatusCancelled, StatusUnknown, StatusSkipped, StatusWaiting},
expectedStatus: StatusWaiting,
},
{
desc: "jobs blocked, jobs waiting - aggregate waiting",
runStatuses: []Status{StatusSuccess, StatusFailure, StatusCancelled, StatusSkipped, StatusWaiting, StatusBlocked},
expectedStatus: StatusWaiting,
},
{
desc: "jobs blocked, jobs running and waiting - aggregate running",
runStatuses: []Status{StatusSuccess, StatusFailure, StatusCancelled, StatusSkipped, StatusRunning, StatusWaiting, StatusBlocked},
expectedStatus: StatusRunning,
},
{
desc: "jobs blocked, finished or unknown - aggregate blocked",
runStatuses: []Status{StatusBlocked, StatusSuccess, StatusFailure, StatusCancelled, StatusSkipped},
expectedStatus: StatusBlocked,
},
{
desc: "job status unknown - aggregate unknown",
runStatuses: []Status{StatusUnknown},
expectedStatus: StatusUnknown,
},
}
for _, tC := range testCases {
mockedJobs := []*ActionRunJob{}
for _, v := range tC.runStatuses {
mockedJobs = append(mockedJobs, &ActionRunJob{Status: v})
}
// Safe guard function against order dependency
rand.Shuffle(len(mockedJobs), func(i, j int) {
mockedJobs[i], mockedJobs[j] = mockedJobs[j], mockedJobs[i]
})

	t.Run(tC.desc, func(t *testing.T) {
		status := AggregateJobStatus(mockedJobs)
		assert.Equal(t, tC.expectedStatus, status, "Expected %s, got %s", tC.expectedStatus.String(), status.String())
	})
}

}

</details>

[wxiaoguang]

I think you are right, I didn't carefully study GitHub's behavior and only wrote the tests by my understanding.

I made a following up PR: Improve Actions status aggregations #32860 , does it look good to you? I think it is the same as your GitHub demo now.