From 0b12b62a0117b69ed9b975984ddcc7f24db7cc2e Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 28 Oct 2026 11:17:45 +0100 Subject: [PATCH] A failure among the change's own tests ends the check ask's level there, so the rest of the suite is not paid for (issue 1289) Quick fix chosen (in askrun.go, the one place every build module's check ask runs): own-test packages already ran first, but the rest ran after their failure (issue 1921). 1921 still holds past them. --- go/buildstep/askrun.go | 14 +++++-- go/buildstep/askrun_test.go | 82 +++++++++++++++++++++++-------------- 2 files changed, 63 insertions(+), 33 deletions(-) diff --git a/go/buildstep/askrun.go b/go/buildstep/askrun.go index f1ef95b..c9fa172 100644 --- a/go/buildstep/askrun.go +++ b/go/buildstep/askrun.go @@ -228,8 +228,10 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string _ = json.Unmarshal(r, &t) return t.Passed, nil } - // Every test step of the level runs, also after one that did not pass, so one check names every failing test of the - // level and not only the first (issue 1921); the level as a whole passes only when each of its steps did. + // The packages (or Node directories) whose tests the change touches run first, and a failure among them ends the level + // there, so a change's own failing test is answered without the rest of the suite (issue 1289; ADR 0444). Past them every + // test step of the level runs, also after one that did not pass, so one check names every failing test of the rest of + // the level and not only the first (issue 1921); the level passes only when each of its steps did. all := true if s.Digest == nil { // A module with no digest (Node) tests each module directory of the tree, those whose tests changed first. @@ -244,6 +246,9 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string } all = all && passed } + if !all { + return false, nil // the change's own tests failed: the others are not run (issue 1289) + } } return all, nil } @@ -279,7 +284,10 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string if err != nil { return false, fmt.Errorf("the %s level: %w", level, err) } - all = all && passed // the packages left still run (issue 1921) + all = all && passed // past the change's own packages the others still run (issue 1921) + if priority && !all { + return false, nil // the change's own tests failed: the others are not run (issue 1289) + } } return all, nil } diff --git a/go/buildstep/askrun_test.go b/go/buildstep/askrun_test.go index 49ea614..a2dbefe 100644 --- a/go/buildstep/askrun_test.go +++ b/go/buildstep/askrun_test.go @@ -113,7 +113,7 @@ func TestRunAskRefusesAnotherToolchain(t *testing.T) { } // 1887/A3, A5a: a check ask runs the levels of one check after one fetch: it digests the unit level, takes the kept -// verdicts, tests the packages whose tests changed first, then the rest also after a failure (issue 1921), and runs no +// verdicts, tests the packages whose tests changed first and, when one of them fails, no other (issue 1289), and runs no // level after one that did not pass (ADR 0484 clause 13). func TestRunAskChecksTheLevelsOfOneCheck(t *testing.T) { r := &recorder{ @@ -129,8 +129,7 @@ func TestRunAskChecksTheLevelsOfOneCheck(t *testing.T) { got := strings.Join(r.calls, "\n") want := `fetch {"base":"` + runCommit + `","commit":"` + runCommit + `","repository":"novox/x"} digest {"level":"unit","workspace":"/w"} -test {"level":"unit","packages":["x/pkg/b"],"workspace":"/w"} -test {"level":"unit","packages":["x/pkg/a"],"workspace":"/w"}` +test {"level":"unit","packages":["x/pkg/b"],"workspace":"/w"}` if got != want { t.Fatalf("calls:\n%s\nwant:\n%s", got, want) } @@ -139,7 +138,7 @@ test {"level":"unit","packages":["x/pkg/a"],"workspace":"/w"}` t.Fatalf("levels %+v, want the unit level alone", a.Levels) } u := a.Levels[0] - if u.Level != delivery.LevelUnit || len(u.Kept) != 1 || u.Kept[0].Package != "x/pkg/k" || u.Kept[0].Tests != 7 || len(u.Tests) != 2 || u.Digest == nil { + if u.Level != delivery.LevelUnit || len(u.Kept) != 1 || u.Kept[0].Package != "x/pkg/k" || u.Kept[0].Tests != 7 || len(u.Tests) != 1 || u.Digest == nil { t.Fatalf("the unit level %+v", u) } @@ -254,41 +253,64 @@ func TestRunAskChecksNodeModulesAndConflicts(t *testing.T) { } } -// Issue 1921: a failure in the packages whose tests changed does not stop the level: the other packages still run, so two -// failing packages, one changed and one not, are two test results of the one level, and the level does not pass. A Node -// check runs every module directory after one fails too. -func TestRunAskRunsEveryPackageAfterAFailure(t *testing.T) { - r := &recorder{ - fetch: map[string]any{"workspace": "/w", "changed": []string{"pkg/b/b_test.go"}}, - digests: map[string]string{"x/pkg/a": "d-a", "x/pkg/b": "d-b"}, - tests: map[string]bool{"x/pkg/a": false, "x/pkg/b": false}, - } - out, err := RunAsk(context.Background(), r.steps(), ask(delivery.CheckJob{Levels: []string{delivery.LevelIntegration, delivery.LevelUnit}, - Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}})) - if err != nil { - t.Fatal(err) - } - a := out.(delivery.CheckAnswer) - if len(a.Levels) != 1 || len(a.Levels[0].Tests) != 2 { - t.Fatalf("levels %+v, want the one level with both packages' results", a.Levels) - } - for i, raw := range a.Levels[0].Tests { - var res struct{ Passed bool } - if err := json.Unmarshal(raw, &res); err != nil || res.Passed { - t.Errorf("result %d %s, want a failure", i, raw) +// Issue 1289: a failure in the packages whose tests changed ends the level there, the others are not run, and no later level +// runs. Issue 1921: past the change's own packages every package still runs, so two failing packages none of which the +// change touched are two test results of the one level. A Node check does the same with its module directories. +func TestRunAskStopsAtTheChangesOwnFailureAndRunsTheRestAfterOthers(t *testing.T) { + for _, row := range []struct { + name string + changed []string + tests map[string]bool + want []string // the packages tested, in order + }{ + {"the change's own test fails: nothing else runs", []string{"pkg/b/b_test.go"}, map[string]bool{"x/pkg/a": false, "x/pkg/b": false}, []string{"x/pkg/b"}}, + {"the change's own test passes: the rest runs, also after a failure", []string{"pkg/b/b_test.go"}, map[string]bool{"x/pkg/a": false, "x/pkg/b": true, "x/pkg/c": false}, []string{"x/pkg/b", "x/pkg/a x/pkg/c"}}, + {"no test changed: every failure is named", nil, map[string]bool{"x/pkg/a": false, "x/pkg/b": false}, []string{"x/pkg/a x/pkg/b"}}, + } { + r := &recorder{fetch: map[string]any{"workspace": "/w", "changed": row.changed}, tests: row.tests, + digests: map[string]string{"x/pkg/a": "d-a", "x/pkg/b": "d-b"}} + if _, ok := row.tests["x/pkg/c"]; ok { + r.digests["x/pkg/c"] = "d-c" + } + out, err := RunAsk(context.Background(), r.steps(), ask(delivery.CheckJob{Levels: []string{delivery.LevelUnit, delivery.LevelIntegration}, + Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}})) + if err != nil { + t.Fatal(err) + } + var got []string + for _, c := range r.calls[1:] { + if strings.HasPrefix(c, "test ") { + var a struct{ Packages []string } + _ = json.Unmarshal([]byte(strings.TrimPrefix(c, "test ")), &a) + got = append(got, strings.Join(a.Packages, " ")) + } + } + // the passing row goes on to the integration level, which tests the same packages again; the first level is what is judged + if n := len(row.want); len(got) < n || fmt.Sprint(got[:n]) != fmt.Sprint(row.want) { + t.Errorf("%s: tested %v, want %v first", row.name, got, row.want) + } + a := out.(delivery.CheckAnswer) + failed := false + for _, raw := range a.Levels[0].Tests { + var res struct{ Passed bool } + _ = json.Unmarshal(raw, &res) + failed = failed || !res.Passed + } + if failed && (len(a.Levels) != 1 || len(got) != len(row.want)) { + t.Errorf("%s: a failed level went on: levels %d, tests %v", row.name, len(a.Levels), got) } } - r = &recorder{fetch: map[string]any{"workspace": "/w", "modules": []string{"a", "b"}, "changed": []string{"b/test/x.test.ts"}}, + r := &recorder{fetch: map[string]any{"workspace": "/w", "modules": []string{"a", "b"}, "changed": []string{"b/test/x.test.ts"}}, tests: map[string]bool{"a": true, "b": false}} s := r.steps() s.Digest = nil - out, err = RunAsk(context.Background(), s, ask(delivery.CheckJob{Levels: []string{delivery.LevelUnit, delivery.LevelIntegration}, + out, err := RunAsk(context.Background(), s, ask(delivery.CheckJob{Levels: []string{delivery.LevelUnit, delivery.LevelIntegration}, Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}})) if err != nil { t.Fatal(err) } - if a := out.(delivery.CheckAnswer); len(r.calls) != 3 || len(a.Levels) != 1 || len(a.Levels[0].Tests) != 2 { - t.Fatalf("calls %v, levels %+v: want both directories of the unit level and no integration level", r.calls, a.Levels) + if a := out.(delivery.CheckAnswer); len(r.calls) != 2 || len(a.Levels) != 1 || len(a.Levels[0].Tests) != 1 { + t.Fatalf("calls %v, levels %+v: want the changed directory b alone and no integration level", r.calls, a.Levels) } }