diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 1c5f9146..be9f44e6 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -245,11 +245,12 @@ func (b *bench) answer(a buildstep.StepAsk) buildstep.Step { } out, err := gobuild.RunAsk(context.Background(), steps, a) step := buildstep.Step{ID: a.ID, Tool: a.Kind + " ask", Node: node, State: buildstep.StepPassed, Started: began, Ended: began.Add(2 * time.Second)} + if out != nil { + step.Result, _ = json.Marshal(out) // a failed step keeps what it answered beside its error, as the step runner does + } if err != nil { step.State, step.Reason = buildstep.StepFailed, err.Error() - return step } - step.Result, _ = json.Marshal(out) return step } @@ -589,9 +590,11 @@ func TestANodePackageIsNeverKeptAndRunsEveryCheck(t *testing.T) { // 1288/AC4, 1 // 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). +// and says why it was not run (review of 650a1d0). A member whose step failed after its fetch merged is previewed from the +// fetch the failed step answered, and the composition is read (design 47 G A-286). func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { - for _, conflicts := range []bool{false, true} { + for _, kind := range []string{"tests", "conflicts", "errors"} { + conflicts, errs := kind == "conflicts", kind == "errors" b := newBench(t) comps := &fakeCompositions{v: refusedOn("x", "g14", "route-proxy is defined twice"), waits: conflicts} b.ex.Compositions = comps @@ -606,6 +609,9 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { } failing := captured[Tested](t, "go-test-failing") b.test = func(c stepCall) (any, error) { + if errs && c.args["level"] == "unit" { + return nil, errors.New("the seat definitions in . do not generate") + } if got := packagesOf(c); c.args["level"] == "integration" || (len(got) == 1 && got[0] == "m/internal/b") { r := failing r.Level = c.args["level"].(string) @@ -622,6 +628,14 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { case <-time.After(5 * time.Second): t.Fatalf("conflicts %v: the check waits on a composition that only waits", conflicts) } + if errs { + keys, _ := b.prev.Keys(context.Background()) + if len(keys) != 2 || len(comps.asked) == 0 || v.Composition == nil || v.Composition.State == meshdeliverycheckerclient.CompositionNotRun { + t.Errorf("a step failed after its fetch merged: previews %v, asked %v, composition %+v, want both previews and the composition read", + keys, comps.asked, v.Composition) + } + continue + } var got []string for _, f := range v.Failures { got = append(got, firstFailed(meshdeliverychecker.VerdictsByGroupHeadsValue{Failed: &FailedFirst{Test: f.Test, Machine: f.Machine, Words: f.Words}})) @@ -644,7 +658,7 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { case !conflicts && (len(keys) != 2 || v.Composition == nil || v.Composition.State != meshdeliverycheckerclient.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 != meshdeliverycheckerclient.CompositionNotRun || - !strings.Contains(v.Composition.Words, "novox/mesh-sdk#12")): + !strings.Contains(v.Composition.Words, "novox/mesh-sdk#12 (its head does not merge)")): 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 diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index bc6adc0a..5d8c25cc 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -284,11 +284,14 @@ 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 are the members (pullKey) with no preview of a merge, each with which of the two it is: a head that does not + // merge, or a fetch that did not answer (design 47 G A-286). The indexer holds no manifest of them, so the assigner's + // composition would only wait: it is not awaited. A member whose fetch merged is previewed whatever its later steps ended with. unpreviewed []string // tags are the group's tagged versions its members are built against (ADR 0607 §3), once the check heard them. tags groupVersions + // conflicted are the members (pullKey) whose head does not merge into its trunk. + conflicted map[string]bool // outOfStep are, by pull request (pullKey), the generated files a level's compare step found out of step (design 26). outOfStep map[string][]string // noCandidate says why the case has no previewed candidate of its heads from the indexer within the bound: for a whole @@ -344,7 +347,7 @@ func (st *state) takeTryCandidate(ctx context.Context) { return } if len(st.unpreviewed) > 0 { - no("no preview of a merge of " + strings.Join(st.unpreviewed, ", ") + " (its head does not merge or its fetch did not answer)") + no("no preview of a merge of " + strings.Join(st.unpreviewed, ", ")) return } if st.c.Wait != nil { @@ -425,7 +428,7 @@ func (st *state) compose(ctx context.Context) { if len(st.unpreviewed) > 0 { // An engine would only say it cannot read a manifest the indexer will not have: the check does not ask it. st.composition = &rules.Composition{NotRun: "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"} + ", so the change cannot be composed"} return } // ADR 0603 §10: the check composes with the candidate's builds; whether it failed already is the running rules' word @@ -593,13 +596,16 @@ func (st *state) judgeAll(ctx context.Context) { st.ask(ctx, t, &merged) } // 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). + // or a later step failed, so the indexer holds its manifests and the assigner can compose the change (review of + // 650a1d0; design 47 G A-286). switch { case st.narrowTry(): // a try of a level or some tests owes the indexer no preview: it judges no manifest case len(merged) == len(modules): st.j.writePreview(ctx, p, merged) + case st.conflicted[pullKey(p)]: + st.unpreviewed = append(st.unpreviewed, pullKey(p)+" (its head does not merge)") default: - st.unpreviewed = append(st.unpreviewed, pullKey(p)) + st.unpreviewed = append(st.unpreviewed, pullKey(p)+" (its fetch did not answer)") } st.judgedByMember(p, merged) // The old form (ADR 0500 §5): the group's package member packed by the Node fetch, kept only while no tagged @@ -730,6 +736,14 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l st.facts = append(st.facts, rules.Step{Repository: p.Repository, Number: p.Number, Module: t.module, Node: step.Node}) st.c.Progress.Set(fmt.Sprintf("judge the check of %s#%d", p.Repository, p.Number), step.Node) if step.State != buildstep.StepPassed { + // A step that failed after its fetch merged answers the fetch beside its error (design 47 G A-286): the merge is + // previewed from it, so its manifests are judged and the change composed whatever the step ended with. + var failed buildstep.CheckAnswer + if !placed && json.Unmarshal(step.Result, &failed) == nil && failed.Fetch.Workspace != "" && len(failed.Fetch.Conflicts) == 0 { + t.fetched, t.changed = failed.Fetch, failed.Fetch.Changed + *merged = append(*merged, t.fetched) + st.checkBinariesOf(t) + } st.fail(step.Node, fmt.Sprintf("the check of %s#%d %s: %s", p.Repository, p.Number, step.State, step.Reason)) return } @@ -743,6 +757,10 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l return } if len(t.fetched.Conflicts) > 0 { + if st.conflicted == nil { + st.conflicted = map[string]bool{} + } + st.conflicted[pullKey(p)] = true if !st.narrowTry() { st.j.writePreview(ctx, p, append(*merged, t.fetched)) }