fix: Actions log view stops refreshing after the displayed job is finished, even if other jobs are still running (#9158)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9158
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

routers/web/repo/actions/view.go

@@ -112,8 +112,16 @@ type ViewRequest struct {
 }
 
 type ViewResponse struct {
-	State struct {
+	State ViewState `json:"state"`
-		Run struct {
+	Logs  ViewLogs  `json:"logs"`
+}
+
+type ViewState struct {
+	Run        ViewRunInfo    `json:"run"`
+	CurrentJob ViewCurrentJob `json:"currentJob"`
+}
+
+type ViewRunInfo struct {
 	Link              string        `json:"link"`
 	Title             string        `json:"title"`
 	TitleHTML         template.HTML `json:"titleHTML"`
@@ -125,16 +133,16 @@ type ViewResponse struct {
 	Done              bool          `json:"done"`
 	Jobs              []*ViewJob    `json:"jobs"`
 	Commit            ViewCommit    `json:"commit"`
-		} `json:"run"`
+}
-		CurrentJob struct {
+
+type ViewCurrentJob struct {
 	Title  string         `json:"title"`
 	Detail string         `json:"detail"`
 	Steps  []*ViewJobStep `json:"steps"`
-		} `json:"currentJob"`
+}
-	} `json:"state"`
+
-	Logs struct {
+type ViewLogs struct {
 	StepsLog []*ViewStepLog `json:"stepsLog"`
-	} `json:"logs"`
 }
 
 type ViewJob struct {
@@ -211,10 +219,20 @@ func ViewPost(ctx *context_module.Context) {
 	resp.State.Run.CanApprove = run.NeedApproval && ctx.Repo.CanWrite(unit.TypeActions)
 	resp.State.Run.CanRerun = run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions)
 	resp.State.Run.CanDeleteArtifact = run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions)
-	resp.State.Run.Done = run.Status.IsDone()
 	resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead of 'null' in json
 	resp.State.Run.Status = run.Status.String()
+
+	// It's possible for the run to be marked with a finalized status (eg. failure) because of a  single job within the
+	// run; eg. one job fails, the run fails. But other jobs can still be running. The frontend RepoActionView uses the
+	// `done` flag to indicate whether to stop querying the run's status -- so even though the run has reached a final
+	// state, it may not be time to stop polling for updates.
+	done := run.Status.IsDone()
+
 	for _, v := range jobs {
+		if !v.Status.IsDone() {
+			// Ah, another job is still running. Keep the frontend polling enabled then.
+			done = false
+		}
 		resp.State.Run.Jobs = append(resp.State.Run.Jobs, &ViewJob{
 			ID:       v.ID,
 			Name:     v.Name,
@@ -223,6 +241,7 @@ func ViewPost(ctx *context_module.Context) {
 			Duration: v.Duration().String(),
 		})
 	}
+	resp.State.Run.Done = done
 
 	pusher := ViewUser{
 		DisplayName: run.TriggerUser.GetDisplayName(),

routers/web/repo/actions/view_test.go

@@ -5,14 +5,19 @@ package actions
 
 import (
 	"fmt"
+	"html/template"
+	"net/http"
 	"testing"
 
 	actions_model "forgejo.org/models/actions"
 	repo_model "forgejo.org/models/repo"
 	unittest "forgejo.org/models/unittest"
+	"forgejo.org/modules/json"
+	"forgejo.org/modules/web"
 	"forgejo.org/services/contexttest"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_getRunByID(t *testing.T) {
@@ -135,3 +140,164 @@ func Test_artifactsFindByNameOrID(t *testing.T) {
 		})
 	}
 }
+
+func baseExpectedResponse() *ViewResponse {
+	return &ViewResponse{
+		State: ViewState{
+			Run: ViewRunInfo{
+				Link:              "/user5/repo4/actions/runs/187",
+				Title:             "update actions",
+				TitleHTML:         template.HTML("update actions"),
+				Status:            "success",
+				CanCancel:         false,
+				CanApprove:        false,
+				CanRerun:          false,
+				CanDeleteArtifact: false,
+				Done:              true,
+				Jobs: []*ViewJob{
+					{
+						ID:       192,
+						Name:     "job_2",
+						Status:   "success",
+						CanRerun: false,
+						Duration: "1m38s",
+					},
+				},
+				Commit: ViewCommit{
+					LocaleCommit:   "actions.runs.commit",
+					LocalePushedBy: "actions.runs.pushed_by",
+					LocaleWorkflow: "actions.runs.workflow",
+					ShortSha:       "c2d72f5484",
+					Link:           "/user5/repo4/commit/c2d72f548424103f01ee1dc02889c1e2bff816b0",
+					Pusher: ViewUser{
+						DisplayName: "user1",
+						Link:        "/user1",
+					},
+					Branch: ViewBranch{
+						Name:      "master",
+						Link:      "/user5/repo4/src/branch/master",
+						IsDeleted: false,
+					},
+				},
+			},
+			CurrentJob: ViewCurrentJob{
+				Title:  "job_2",
+				Detail: "actions.status.success",
+				Steps: []*ViewJobStep{
+					{
+						Summary: "Set up job",
+						Status:  "running",
+					},
+					{
+						Summary: "Complete job",
+						Status:  "waiting",
+					},
+				},
+			},
+		},
+		Logs: ViewLogs{
+			StepsLog: []*ViewStepLog{},
+		},
+	}
+}
+
+func TestActionsViewViewPost(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	tests := []struct {
+		name           string
+		runIndex       int64
+		jobIndex       int64
+		expected       *ViewResponse
+		expectedTweaks func(*ViewResponse)
+	}{
+		{
+			name:     "base case",
+			runIndex: 187,
+			jobIndex: 0,
+			expected: baseExpectedResponse(),
+			expectedTweaks: func(resp *ViewResponse) {
+			},
+		},
+		{
+			name:     "run with waiting jobs",
+			runIndex: 189,
+			jobIndex: 0,
+			expected: baseExpectedResponse(),
+			expectedTweaks: func(resp *ViewResponse) {
+				// Variations from runIndex 187 -> runIndex 189 that are not the subject of this test...
+				resp.State.Run.Link = "/user5/repo4/actions/runs/189"
+				resp.State.Run.Title = "job output"
+				resp.State.Run.TitleHTML = "job output"
+				resp.State.Run.Jobs = []*ViewJob{
+					{
+						ID:     194,
+						Name:   "job1 (1)",
+						Status: "success",
+					},
+					{
+						ID:     195,
+						Name:   "job1 (2)",
+						Status: "success",
+					},
+					{
+						ID:     196,
+						Name:   "job2",
+						Status: "waiting",
+					},
+				}
+				resp.State.CurrentJob.Title = "job1 (1)"
+				resp.State.CurrentJob.Steps = []*ViewJobStep{
+					{
+						Summary: "Set up job",
+						Status:  "success",
+					},
+					{
+						Summary: "Complete job",
+						Status:  "success",
+					},
+				}
+
+				// Under test in this case: verify that Done is set to false; in the fixture data, job.ID=195 is status
+				// Success, but job.ID=196 is status Waiting, and so we expect to signal Done=false to indicate to the
+				// UI to continue refreshing the page.
+				resp.State.Run.Done = false
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, resp := contexttest.MockContext(t, "user2/repo1/actions/runs/0")
+			contexttest.LoadUser(t, ctx, 2)
+			contexttest.LoadRepo(t, ctx, 4)
+			ctx.SetParams(":run", fmt.Sprintf("%d", tt.runIndex))
+			ctx.SetParams(":job", fmt.Sprintf("%d", tt.jobIndex))
+			web.SetForm(ctx, &ViewRequest{})
+
+			ViewPost(ctx)
+			require.Equal(t, http.StatusOK, resp.Result().StatusCode)
+
+			var actual ViewResponse
+			err := json.Unmarshal(resp.Body.Bytes(), &actual)
+			require.NoError(t, err)
+
+			// `Duration` field is dynamic based upon current time, so eliminate it from comparison -- but check that it
+			// has the right format at least.
+			zeroDurations := func(vr *ViewResponse) {
+				for _, job := range vr.State.Run.Jobs {
+					assert.Regexp(t, `^(\d+[hms]){1,3}$`, job.Duration)
+					job.Duration = ""
+				}
+				for _, step := range vr.State.CurrentJob.Steps {
+					step.Duration = ""
+				}
+			}
+			zeroDurations(&actual)
+			zeroDurations(tt.expected)
+			tt.expectedTweaks(tt.expected)
+
+			assert.Equal(t, *tt.expected, actual)
+		})
+	}
+}