diff --git a/internal/sweep/sweep.go b/internal/sweep/sweep.go index c640a93..88889d9 100644 --- a/internal/sweep/sweep.go +++ b/internal/sweep/sweep.go @@ -226,18 +226,49 @@ func (f Failure) Key() string { return f.Workflow + " " + f.Conclusion } // Select reduces the runs to the distinct failures worth raising. // -// Three filters, and each of them is a way this would otherwise be noise. Only +// Four filters, and each of them is a way this would otherwise be noise. Only // a watched workflow, so a run of something nobody scheduled is not swept up. // Only the schedule event, because a run somebody asked for has somebody // looking at it and this exists for the runs nobody asked for. Only the default // branch, because a scheduled run is a run of that branch and anything else // arriving here is a fact about the reader rather than about the tree. +// +// And only a failure no later scheduled run has recovered from. The question +// this package asks is whether the thing is failing now, which is the same +// question the window in github.go is sized for, and the raised issue says so +// in its own words: it closes on a run of the same workflow ending in success. +// Without this filter the failure stays selectable for as long as it is in the +// window, so the issue is raised again on the next sweep after it is closed, +// under the same key, and the loop ends when the failure scrolls out rather +// than when anything is fixed. A recovery is a scheduled run of the default +// branch, because those are the runs this reports on; a run somebody asked for +// has somebody in front of it and clears nothing here. +// +// The recovery cannot be outside the window while the failure is inside it, +// because the window holds the newest runs and the recovery is the newer of +// the two. func Select(watched []string, runs []Run, defaultBranch string) []Failure { inSet := map[string]bool{} for _, w := range watched { inSet[w] = true } + // The newest run of each watched workflow that reported the schedule + // working again. Run numbers are per workflow and increase, so this is the + // line a failure has to be newer than to still be worth raising. + recovered := map[string]int{} + for _, r := range runs { + switch { + case !inSet[r.Workflow], r.Event != "schedule", r.Branch != defaultBranch: + continue + case !r.Ended(), r.Conclusion != "success": + continue + } + if r.Number > recovered[r.Workflow] { + recovered[r.Workflow] = r.Number + } + } + grouped := map[string]*Failure{} for _, r := range runs { switch { @@ -245,6 +276,8 @@ func Select(watched []string, runs []Run, defaultBranch string) []Failure { continue case !r.Ended(), r.Conclusion == "success": continue + case r.Number <= recovered[r.Workflow]: + continue } f, ok := grouped[r.Workflow+" "+r.Conclusion] diff --git a/internal/sweep/sweep_test.go b/internal/sweep/sweep_test.go index bff15ef..c1d66fa 100644 --- a/internal/sweep/sweep_test.go +++ b/internal/sweep/sweep_test.go @@ -179,6 +179,45 @@ func TestOneFailurePerWorkflowAndVerdictRatherThanOnePerRun(t *testing.T) { } } +func TestAFailureALaterScheduledRunRecoveredFromIsNotSelected(t *testing.T) { + watched := []string{".github/workflows/nightly.yml"} + failed := run(watched[0], "failure", 18) + + // The failure on its own is what the sweep exists to report, so the filter + // below is not one that reports nothing. + if got := Select(watched, []Run{failed}, "main"); len(got) != 1 { + t.Fatalf("a failure with no later run selected %d failure(s), want 1", len(got)) + } + + recovered := run(watched[0], "success", 19) + if got := Select(watched, []Run{failed, recovered}, "main"); len(got) != 0 { + t.Errorf("a failure a later scheduled run recovered from is still selected, so closing its issue raises the same one again: %+v", got) + } + + // Three runs that are not a recovery, each for a different reason. A + // success before the failure says nothing about it, and a run off the + // schedule or off the default branch is outside the population this sweep + // reports on at all. + for _, c := range []struct { + name string + in Run + }{ + {"a success older than the failure", run(watched[0], "success", 17)}, + {"a success somebody asked for", askedFor(run(watched[0], "success", 20))}, + {"a success off the default branch", offBranch(run(watched[0], "success", 21))}, + } { + t.Run(c.name, func(t *testing.T) { + if got := Select(watched, []Run{failed, c.in}, "main"); len(got) != 1 { + t.Errorf("selected %d failure(s), want 1: %+v", len(got), got) + } + }) + } +} + +func askedFor(r Run) Run { r.Event = "workflow_dispatch"; return r } + +func offBranch(r Run) Run { r.Branch = "a-branch"; return r } + func TestAFailureAnOpenIssueAlreadyHoldsIsNotRaisedAgain(t *testing.T) { failures := Select( []string{".github/workflows/nightly.yml"},