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