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) {