From fa85de8ef6bb32716ada3b3dd299439658ff76e6 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 2 Nov 2026 16:25:33 +0100 Subject: [PATCH] mesh-delivery-checker: write a member's preview whenever its fetch merged, and do not wait on a composition that cannot come Review of 650a1d0: a member that only failed its tests got no preview, so the indexer held no manifest of it, the assigner's composition only said it waits, and the check waited for ever. The preview is now written whenever every build module's fetch merged; a member with no preview of a merge (a conflict, a fetch that did not answer) makes the composition not run, said with the members it names, unawaited. Claude-Session: https://claude.ai/code/session_01NcbgfJ9Gum47zkFKWRaovL --- .../internal/check/composition_test.go | 9 +- .../internal/check/examiner_test.go | 131 ++++++++++-------- .../internal/check/judge.go | 25 +++- 3 files changed, 104 insertions(+), 61 deletions(-) diff --git a/modules/mesh-delivery-checker/internal/check/composition_test.go b/modules/mesh-delivery-checker/internal/check/composition_test.go index 9320456..bd5d357 100644 --- a/modules/mesh-delivery-checker/internal/check/composition_test.go +++ b/modules/mesh-delivery-checker/internal/check/composition_test.go @@ -14,10 +14,17 @@ type fakeCompositions struct { v delivery.CompositionVerdict err error asked []string + // waits answers only that the composition waits, as the assigner does for a member the indexer holds no manifest of: + // Await returns only when its context ends. + waits bool } -func (f *fakeCompositions) Await(_ context.Context, key string) (delivery.CompositionVerdict, error) { +func (f *fakeCompositions) Await(ctx context.Context, key string) (delivery.CompositionVerdict, error) { f.asked = append(f.asked, key) + if f.waits { + <-ctx.Done() + return delivery.CompositionVerdict{}, ctx.Err() + } return f.v, f.err } diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 112e4aa..8a4ea45 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "sort" "strings" "sync" @@ -433,66 +434,86 @@ func TestANodePackageIsNeverKeptAndRunsEveryCheck(t *testing.T) { // 1288/AC4, 1 } // The operator, 2026-11-02 ("A failed check shows only its first fault"): a check runs every member, every level and the -// composition to its end and its verdict names every failure: a member's head that does not merge, the other member's -// failing test at the unit level and again at the integration level, and the failed composition, in the order judged. -// Failed keeps the first whole; the change's own tests still run first (issue 1289), and the rest of the level after them. +// composition to its end and its verdict names every failure in the order judged, Failed keeping the first whole; the +// change's own tests still run first (issue 1289), and the rest of the level after them. A member that only failed its +// tests still gets its preview, so the composition is read and its failure named after the tests'. A member whose head +// does not merge has no preview of a merge, so the assigner's composition would only wait: the check does not wait for it +// and says why it was not run (review of 650a1d0). func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { - b := newBench(t) - b.ex.Compositions = &fakeCompositions{v: delivery.CompositionVerdict{Group: "feat/x", Heads: "0123456789ab", State: delivery.CompositionFail, SDK: "x", - Machine: "g14", Words: "route-proxy is defined twice"}} - b.fetch.Changed = []string{"modules/m/internal/b/b_test.go"} - b.digest[delivery.LevelUnit] = Digested{Level: "unit", Packages: map[string]string{"m/internal/a": digA, "m/internal/b": digB}} - conflict := captured[Fetched](t, "go-fetch-conflict") - b.steps.handlers["build_go_fetch"] = func(c stepCall) (any, error) { - if c.args["repository"] == repoSDK { - return conflict, nil + for _, conflicts := range []bool{false, true} { + b := newBench(t) + comps := &fakeCompositions{v: delivery.CompositionVerdict{Group: "feat/x", Heads: "0123456789ab", State: delivery.CompositionFail, SDK: "x", + Machine: "g14", Words: "route-proxy is defined twice"}, waits: conflicts} + b.ex.Compositions = comps + b.fetch.Changed = []string{"modules/m/internal/b/b_test.go"} + b.digest[delivery.LevelUnit] = Digested{Level: "unit", Packages: map[string]string{"m/internal/a": digA, "m/internal/b": digB}} + conflict := captured[Fetched](t, "go-fetch-conflict") + b.steps.handlers["build_go_fetch"] = func(c stepCall) (any, error) { + if conflicts && c.args["repository"] == repoSDK { + return conflict, nil + } + return b.fetch, nil } - return b.fetch, nil - } - failing := captured[Tested](t, "go-test-failing") - b.test = func(c stepCall) (any, error) { - if got := packagesOf(c); c.args["level"] == "integration" || (len(got) == 1 && got[0] == "m/internal/b") { - r := failing - r.Level = c.args["level"].(string) - return r, nil + failing := captured[Tested](t, "go-test-failing") + b.test = func(c stepCall) (any, error) { + if got := packagesOf(c); c.args["level"] == "integration" || (len(got) == 1 && got[0] == "m/internal/b") { + r := failing + r.Level = c.args["level"].(string) + return r, nil + } + return captured[Tested](t, "go-test-slow"), nil } - return captured[Tested](t, "go-test-slow"), nil - } - v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1), pullOf(repoSDK, 12, head2))).Verdict - var got []string - for _, f := range v.Failures { - got = append(got, firstFailed(delivery.Verdict{Failed: f})) - } - want := []string{"shanks: novox/mesh-sdk#12 does not merge into the trunk: files in conflict: a.go", "unit: fixture/failing/bad.TestThreeIsFour", - "integration: fixture/failing/bad.TestThreeIsFour", "g14: route-proxy is defined twice"} - if v.State != delivery.VerdictFail || len(got) != len(want) { - t.Fatalf("verdict %+v: failures\n%s\nwant\n%s", v, strings.Join(got, "\n"), strings.Join(want, "\n")) - } - for i := range want { - if !strings.HasPrefix(got[i], want[i]) { - t.Errorf("failure %d is %q, want %q", i, got[i], want[i]) + cs := caseOf(t, pullOf(repoA, 7, head1), pullOf(repoSDK, 12, head2)) + done := make(chan delivery.Verdict, 1) + go func() { done <- b.judge(t, cs).Verdict }() + var v delivery.Verdict + select { + case v = <-done: + case <-time.After(5 * time.Second): + t.Fatalf("conflicts %v: the check waits on a composition that only waits", conflicts) } - } - if v.Failed.Machine != "shanks" || !strings.Contains(v.Failed.Words, "files in conflict") || v.Composition == nil || v.Composition.State != delivery.CompositionFail { - t.Errorf("failed %+v, composition %+v", v.Failed, v.Composition) - } - var unit []string - for _, c := range b.steps.tool("test") { - if c.args["level"] == "unit" { - unit = append(unit, strings.Join(packagesOf(c), ",")) + var got []string + for _, f := range v.Failures { + got = append(got, firstFailed(delivery.Verdict{Failed: f})) + } + tests := []string{"unit: fixture/failing/bad.TestThreeIsFour", "integration: fixture/failing/bad.TestThreeIsFour"} + want := append(append(slices.Clone(tests), tests...), "g14: route-proxy is defined twice") // both members fail their tests + if conflicts { + want = append([]string{"shanks: novox/mesh-sdk#12 does not merge into the trunk: files in conflict: a.go"}, tests...) + } + if v.State != delivery.VerdictFail || len(got) != len(want) { + t.Fatalf("conflicts %v: verdict %+v: failures\n%s\nwant\n%s", conflicts, v, strings.Join(got, "\n"), strings.Join(want, "\n")) + } + for i := range want { + if !strings.HasPrefix(got[i], want[i]) { + t.Errorf("conflicts %v: failure %d is %q, want %q", conflicts, i, got[i], want[i]) + } + } + keys, _ := b.prev.Keys(context.Background()) + switch { + case !conflicts && (len(keys) != 2 || v.Composition == nil || v.Composition.State != delivery.CompositionFail): + t.Errorf("tests alone failed: previews %v, composition %+v, want both previews and the composition read", keys, v.Composition) + case conflicts && (len(comps.asked) != 0 || v.Composition == nil || v.Composition.State != delivery.CompositionNotRun || + !strings.Contains(v.Composition.Words, "novox/mesh-sdk#12")): + t.Errorf("a member did not merge: asked %v, composition %+v, want not run naming novox/mesh-sdk#12", comps.asked, v.Composition) + } + var unit []string + for _, c := range b.steps.tool("test") { + if c.args["level"] == "unit" && c.args["workspace"] != nil { + unit = append(unit, strings.Join(packagesOf(c), ",")) + } + } + if conflicts && (strings.Join(unit, " | ") != "m/internal/b | m/internal/a" || len(b.steps.tool("digest")) != 2) { + t.Errorf("the unit level ran %v and %d digests, want the changed test's package, then the rest, at both levels", unit, len(b.steps.tool("digest"))) + } + v.Group, v.Heads = "feat/x", "0123456789ab" + if _, err := delivery.EncodeVerdict(v); err != nil { + t.Errorf("conflicts %v: the verdict cannot be written: %v", conflicts, err) + } + if line := Line(v, cs, cs.Pulls[0], nil, nil, nil); !strings.HasPrefix(line, fmt.Sprintf("failed (%d): %s", len(want), want[0][:20])) || + !strings.Contains(line, fmt.Sprintf("; and %d more | ", len(want)-1)) || utf8.RuneCountInString(line) > MaxLine { + t.Errorf("conflicts %v: line %q", conflicts, line) } - } - if strings.Join(unit, " | ") != "m/internal/b | m/internal/a" || len(b.steps.tool("digest")) != 2 { - t.Errorf("the unit level ran %v and %d digests, want the changed test's package, then the rest, at both levels", unit, len(b.steps.tool("digest"))) - } - v.Group, v.Heads = "feat/x", "0123456789ab" - if _, err := delivery.EncodeVerdict(v); err != nil { - t.Errorf("the verdict cannot be written: %v", err) - } - cs := caseOf(t, pullOf(repoA, 7, head1), pullOf(repoSDK, 12, head2)) - if line := Line(v, cs, cs.Pulls[0], nil, nil, nil); !strings.HasPrefix(line, "failed (4): shanks: novox/mesh-sdk#12 does not merge") || - !strings.Contains(line, "; and 3 more | ") || utf8.RuneCountInString(line) > MaxLine { - t.Errorf("line %q", line) } } diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index 8d425ed..5547d47 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -232,6 +232,9 @@ type state struct { judgeHead string // predefinedHead is the catalogue member's head when its merge changes predefined.json: the list is read there. predefinedHead string + // unpreviewed are the members (pullKey) with no preview of a merge: a head that does not merge, or a fetch that did not + // answer. The indexer holds no manifest of them, so the assigner's composition would only wait: it is not awaited. + unpreviewed []string } // Judge runs the case (it is the Judge of a Checker). A failure of the change, of a merge or of a step is a verdict; an error is only a case that cannot @@ -271,6 +274,12 @@ func (st *state) composed(ctx context.Context, v *delivery.Verdict) { if st.j.Compositions == nil || st.c.Try != nil { return } + if len(st.unpreviewed) > 0 { + // The assigner would only say it waits for a manifest the indexer will not have: the check does not wait for it. + v.Composition = &delivery.Composition{State: delivery.CompositionNotRun, Words: "no preview of a merge of " + + strings.Join(st.unpreviewed, ", ") + " (its head does not merge or its fetch did not answer), so the change cannot be composed"} + return + } comp, err := st.j.Compositions.Await(ctx, st.c.Key) switch { case errors.Is(err, ErrAssignerNotRunning): @@ -378,8 +387,8 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { break } var merged []Fetched - memberFailed := false - for _, bm := range st.j.Modules.BuildModules(p.Repository) { + modules := st.j.Modules.BuildModules(p.Repository) + for _, bm := range modules { t := &target{pull: p, module: bm} if tc, none := delivery.AgreedToolchain(st.j.Toolchains.Entries(buildrun.LanguageOf[bm])); none == "" { t.toolchain = tc.Version @@ -392,7 +401,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { if bm == buildrun.GoBuildModule { beside, err := st.j.besideFor(ctx, st.c, p) if err != nil { - failures, memberFailed = append(failures, fail(forgeMachine(err, bm), err.Error()+unheardWords(err))), true + failures = append(failures, fail(forgeMachine(err, bm), err.Error()+unheardWords(err))) continue } if len(beside) > 0 { @@ -407,11 +416,17 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { t.fetchArgs = args st.targets = append(st.targets, t) if v := st.ask(ctx, t, &merged); v != nil { - failures, memberFailed = append(failures, v), true + failures = append(failures, v) } } - if st.c.Try == nil && !memberFailed { // a try owes the indexer no preview: no pull request holds its head + // Every build module's fetch merged (each is in merged once): the preview is written, also when the member's tests + // failed, so the indexer holds its manifests and the assigner can compose the change (review of 650a1d0). + switch { + case st.c.Try != nil: // a try owes the indexer no preview: no pull request holds its head + case len(merged) == len(modules): st.j.writePreview(ctx, p, merged) + default: + st.unpreviewed = append(st.unpreviewed, pullKey(p)) } st.judgedByMember(p, merged) if p.Repository == SDKRepository && len(st.c.Pulls) > 1 && st.j.changesPackage(p, merged) {