diff --git a/internal/blockers/blockers.go b/internal/blockers/blockers.go index 235d740..a723bb1 100644 --- a/internal/blockers/blockers.go +++ b/internal/blockers/blockers.go @@ -25,6 +25,24 @@ // hold is unresolved, which is a failure and never a pass. A run that resolved // nothing and reported agreement is what this exists to prevent. // +// There is a third reading, and it is the one an issue passes through both +// questions untouched. Neither of them asks whether the issue an issue waits +// for is waiting back. A set in which every member waits on another member of +// the same set is a set nothing outside it can end, and every member of it +// reads as ordinarily blocked, is subtracted from the count of available work +// by its label, and is walked past by every reader for as long as nobody +// notices. Eight of this board's own issues were in one such set when this was +// measured, and the run's last line called them clean. +// +// That reading is REPORTED and refuses nothing, which is the one place this +// package prints a state in capitals without failing on it. The repair is an +// edit to an issue somebody has to decide on, the same repair the two +// questions above ask for, but unlike them it cannot be made by the issue's own +// author alone: which member of the set gives way is a reading of what those +// issues are for. Refusing here would put a red gate on a condition that stands +// until that reading is taken. Whether it should refuse anyway is the open half +// of #234 and is not decided here. +// // The reading is the whole tracker rather than one query per issue. A body // names a number that has closed more often than one that has not, and a // closed issue is absent from the listing an open-issue query answers with, so @@ -211,8 +229,13 @@ func Run(read Reader, out io.Writer) error { } } - fmt.Fprintf(out, "%d issue(s) read, %d naming no issue, %d no longer blocked, %d unresolved.\n", - len(blocked), nameless, cleared, unresolved) + sets := reciprocal(blocked) + for _, set := range sets { + fmt.Fprintf(out, " WAITING ON ITSELF: %s each wait on another issue in this set, so nothing closing outside it makes any of them available\n", numbers(set)) + } + + fmt.Fprintf(out, "%d issue(s) read, %d naming no issue, %d no longer blocked, %d unresolved, %d set(s) waiting on themselves.\n", + len(blocked), nameless, cleared, unresolved, len(sets)) var wrong []string if nameless > 0 { @@ -230,6 +253,95 @@ func Run(read Reader, out io.Writer) error { return nil } +// reciprocal returns every set of the issues given in which each member waits +// on another member of the same set, each set sorted and the sets ordered by +// their lowest member. +// +// Only an issue carrying the label is a member. An issue that does not carry +// it is claiming to wait for nothing, so a reference into it is not a wait and +// no path runs through it, however many blocked issues name it. That is what +// keeps a popular open issue out of a set it is merely mentioned by. +// +// The edges are the references the two questions above already resolve, so +// this reading inherits their bound: a body naming a number in passing is read +// as a wait. What makes the inherited over-reading survivable here is that a +// set needs the reference to run in both directions, so both bodies have to be +// describing something other than a wait, in opposite directions, at once. A +// one-sided mention produces no set. +// +// The walk is a reachability closure per member rather than a linear +// component algorithm. The population is the issues one board has under one +// label, which is tens rather than thousands, and the cost of the closure is +// paid once per run against a reading that took a paged fetch of the whole +// tracker to produce. +func reciprocal(blocked []Issue) [][]int { + member := map[int]bool{} + for _, i := range blocked { + member[i.Number] = true + } + + waits := map[int][]int{} + for _, i := range blocked { + for _, n := range References(i.Body) { + if n == i.Number || !member[n] { + continue + } + waits[i.Number] = append(waits[i.Number], n) + } + } + + // reach[n] is every member a wait from n arrives at, following waits as + // far as they go. n is in its own set when it arrives back at itself, + // which is a path of at least one edge and never the empty one. + reach := map[int]map[int]bool{} + for _, i := range blocked { + seen := map[int]bool{} + stack := append([]int(nil), waits[i.Number]...) + for len(stack) > 0 { + n := stack[len(stack)-1] + stack = stack[:len(stack)-1] + if seen[n] { + continue + } + seen[n] = true + stack = append(stack, waits[n]...) + } + reach[i.Number] = seen + } + + taken := map[int]bool{} + var sets [][]int + for _, i := range blocked { + n := i.Number + if taken[n] || !reach[n][n] { + continue + } + set := []int{n} + taken[n] = true + for _, j := range blocked { + m := j.Number + if taken[m] || !reach[n][m] || !reach[m][n] { + continue + } + set = append(set, m) + taken[m] = true + } + sort.Ints(set) + sets = append(sets, set) + } + return sets +} + +// numbers writes a set the way the rest of this run writes a reference, so a +// reader can take a number out of either line and put it into the same query. +func numbers(set []int) string { + out := make([]string, 0, len(set)) + for _, n := range set { + out = append(out, "#"+strconv.Itoa(n)) + } + return strings.Join(out, ", ") +} + // describe names a reference the way a reader has to see it, because a body // naming a merged pull request and a body naming a closed issue are the same // four characters and are not the same statement. diff --git a/internal/blockers/blockers_test.go b/internal/blockers/blockers_test.go index 7a13366..9bad675 100644 --- a/internal/blockers/blockers_test.go +++ b/internal/blockers/blockers_test.go @@ -85,7 +85,7 @@ func TestRunPassesWhenEveryBlockedIssueNamesSomethingStillOpen(t *testing.T) { if !strings.Contains(out, "#36: blocked, waiting on #35 (open issue)") { t.Fatalf("the run should say what #36 waits on, got:\n%s", out) } - if !strings.Contains(out, "1 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved.") { + if !strings.Contains(out, "1 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should count what it read, got:\n%s", out) } } @@ -228,7 +228,7 @@ func TestRunPassesABoardOnWhichNothingIsBlocked(t *testing.T) { if err != nil { t.Fatalf("a board where nothing carries the label is a real zero, got %v\n%s", err, out) } - if !strings.Contains(out, "0 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved.") { + if !strings.Contains(out, "0 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should say it read nothing, got:\n%s", out) } } @@ -275,7 +275,7 @@ func TestRunCountsEveryStateSeparatelyOnOneBoard(t *testing.T) { if err == nil { t.Fatalf("three of these four are wrong and the run should be red, got:\n%s", out) } - if !strings.Contains(out, "4 issue(s) read, 1 naming no issue, 1 no longer blocked, 1 unresolved.") { + if !strings.Contains(out, "4 issue(s) read, 1 naming no issue, 1 no longer blocked, 1 unresolved, 0 set(s) waiting on themselves.") { t.Fatalf("the run should count the three states apart, got:\n%s", out) } for _, want := range []string{"1 name no issue by number", "1 are no longer blocked and still say they are", "1 name a number this board does not hold"} { @@ -284,3 +284,130 @@ func TestRunCountsEveryStateSeparatelyOnOneBoard(t *testing.T) { } } } + +func TestRunNamesTwoIssuesThatWaitOnEachOther(t *testing.T) { + // Neither of these is reported by any of the states above: both name a + // number, both name one that is open, and both resolve. What no reading + // before this one asked is whether the issue each waits for is waiting + // back, and nothing closing anywhere else ends either of them. + out, err := run(t, board( + blockedIssue(35, "Depends on #63, which pins the browser this leg loads the built output in."), + blockedIssue(63, "The second condition needs one browser-backed leg in the same change, which is #35."), + blockedIssue(58, "Depends on #53, the release workflow."), + openIssue(53), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63 each wait on another issue in this set") { + t.Fatalf("the run should name the set, got:\n%s", out) + } + if strings.Contains(out, "#58,") || strings.Contains(out, ", #58") { + t.Fatalf("#58 waits on something outside any set and does not belong in one, got:\n%s", out) + } + if !strings.Contains(out, "3 issue(s) read, 0 naming no issue, 0 no longer blocked, 0 unresolved, 1 set(s) waiting on themselves.") { + t.Fatalf("the run should count the set, got:\n%s", out) + } +} + +func TestRunNamesASetLongerThanAPair(t *testing.T) { + // The near miss for the row above, and the reason the walk follows waits + // as far as they go rather than comparing two bodies. No two of these + // three name each other, so a reading that only compared pairs would + // report nothing here and the set would stand. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #71."), + blockedIssue(71, "Depends on #35."), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63, #71 each wait on another issue in this set") { + t.Fatalf("the run should name all three, got:\n%s", out) + } +} + +func TestRunNamesEachSetSeparatelyRatherThanMergingThem(t *testing.T) { + // Two sets with no path between them are two repairs, and one line + // carrying five numbers would send a reader looking for a wait that is + // not there. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #35."), + blockedIssue(71, "Depends on #87."), + blockedIssue(87, "Depends on #71."), + )) + if err != nil { + t.Fatalf("a reciprocal wait is reported and refuses nothing, got %v\n%s", err, out) + } + for _, want := range []string{"WAITING ON ITSELF: #35, #63 each", "WAITING ON ITSELF: #71, #87 each"} { + if !strings.Contains(out, want) { + t.Fatalf("the run should carry %q, got:\n%s", want, out) + } + } + if !strings.Contains(out, "0 unresolved, 2 set(s) waiting on themselves.") { + t.Fatalf("the run should count two sets, got:\n%s", out) + } +} + +func TestRunReadsNoWaitBackFromAnIssueThatIsNotBlocked(t *testing.T) { + // An issue carrying no label is claiming to wait for nothing, so a number + // in its body is a mention and not an edge. Reading it as one turns every + // blocked issue named by the thing it depends on into a set, which is the + // false positive this row exists against. + out, err := run(t, board( + blockedIssue(35, "Depends on #63, which pins the browser."), + Issue{Number: 63, Title: "the browser pin", State: "open", Body: "The legs that need this are #35 and #36."}, + )) + if err != nil { + t.Fatalf("one blocked issue and one open one is an ordinary board, got %v\n%s", err, out) + } + if strings.Contains(out, "WAITING ON ITSELF") { + t.Fatalf("#63 carries no label and waits for nothing, so there is no set, got:\n%s", out) + } + if !strings.Contains(out, "#35: blocked, waiting on #63 (open issue)") { + t.Fatalf("#35 is still ordinarily blocked, got:\n%s", out) + } +} + +func TestRunReadsNoWaitBackFromAClosedIssue(t *testing.T) { + // The other half of the row above. A closed issue is not judged by this + // run at all, so its body's numbers are not edges either, and a set built + // through one would be a set nobody is waiting in. + out, err := run(t, board( + blockedIssue(35, "Depends on #63 and on #90, which is still open."), + Issue{Number: 63, Title: "done", State: "closed", Body: "Waits on #35.", Labels: []string{Label}}, + openIssue(90), + )) + if err != nil { + t.Fatalf("one open dependency keeps #35 blocked, got %v\n%s", err, out) + } + if strings.Contains(out, "WAITING ON ITSELF") { + t.Fatalf("#63 has closed and waits for nothing, so there is no set, got:\n%s", out) + } +} + +func TestRunReportsASetWithoutMovingTheVerdictItWouldOtherwiseReach(t *testing.T) { + // The bound this reading is delivered under, in the direction that would + // hide something: a set is printed beside a failure rather than instead of + // one, and the failure still carries every state that refuses. + out, err := run(t, board( + blockedIssue(35, "Depends on #63."), + blockedIssue(63, "Depends on #35."), + blockedIssue(50, "Depends on #48."), + closedIssue(48), + )) + if err == nil { + t.Fatalf("#50 is no longer blocked and should still red the run, got:\n%s", out) + } + if !strings.Contains(err.Error(), "1 are no longer blocked and still say they are") { + t.Fatalf("the error should carry the state that refuses, got %v", err) + } + if strings.Contains(err.Error(), "waiting on themselves") { + t.Fatalf("a set refuses nothing and does not belong in the error, got %v", err) + } + if !strings.Contains(out, "WAITING ON ITSELF: #35, #63 each") { + t.Fatalf("the set should be printed beside the failure, got:\n%s", out) + } +}