diff --git a/go/buildstep/askrun.go b/go/buildstep/askrun.go index 44d2fc1..f0cef51 100644 --- a/go/buildstep/askrun.go +++ b/go/buildstep/askrun.go @@ -208,6 +208,9 @@ 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. + all := true if s.Digest == nil { // A module with no digest (Node) tests each module directory of the tree, those whose tests changed first. for _, priority := range []bool{true, false} { @@ -219,14 +222,12 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string if err != nil { return false, fmt.Errorf("the %s level of %s: %w", level, dir, err) } - if !passed { - return false, nil - } + all = all && passed } } - return true, nil + return all, nil } - todo, all, keepRunner := s.plan(ctx, job, level, f, runner, withRunner, la) + todo, whole, keepRunner := s.plan(ctx, job, level, f, runner, withRunner, la) if !keepRunner { runner = "" // an installed module that ignores the runner judges the level as it judges any other (ADR 0535 ยง2) } @@ -235,7 +236,7 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string if job.Run != "" { args["run"] = job.Run } - if all { + if whole { if priority { continue // nothing to put first: the level runs whole after } @@ -255,11 +256,9 @@ 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) } - if !passed { - return false, nil - } + all = all && passed // the packages left still run (issue 1921) } - return true, nil + return all, nil } // release removes an ask's workspace, saying a failure: the module's sweep removes it later. diff --git a/go/buildstep/askrun_test.go b/go/buildstep/askrun_test.go index fa1bf24..8ceb678 100644 --- a/go/buildstep/askrun_test.go +++ b/go/buildstep/askrun_test.go @@ -109,9 +109,9 @@ func TestRunAskRefusesAnotherToolchain(t *testing.T) { } } -// 1887/A3, A5a, issue 1887 design limit 1: 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, and stops at the first failure, running -// no later level. +// 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 +// level after one that did not pass (ADR 0484 clause 13). func TestRunAskChecksTheLevelsOfOneCheck(t *testing.T) { r := &recorder{ fetch: map[string]any{"workspace": "/w", "changed": []string{"pkg/b/b_test.go"}}, @@ -126,7 +126,8 @@ 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/b"],"workspace":"/w"} +test {"level":"unit","packages":["x/pkg/a"],"workspace":"/w"}` if got != want { t.Fatalf("calls:\n%s\nwant:\n%s", got, want) } @@ -135,7 +136,7 @@ test {"level":"unit","packages":["x/pkg/b"],"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) != 1 || 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) != 2 || u.Digest == nil { t.Fatalf("the unit level %+v", u) } } @@ -203,3 +204,42 @@ func TestRunAskChecksNodeModulesAndConflicts(t *testing.T) { t.Fatalf("a conflict: %v %v", err, r.calls) } } + +// 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) + } + } + + 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}, + 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) + } +} diff --git a/go/delivery/checker.go b/go/delivery/checker.go index 8b7a95c..e23a51e 100644 --- a/go/delivery/checker.go +++ b/go/delivery/checker.go @@ -139,16 +139,25 @@ type FailedFirst struct { Words string `json:"words,omitempty"` } +// MaxFailures is the most entries a verdict's Failures holds (issue 1921): fifty tests named one by one, and an entry +// after them saying how many more failed. +const MaxFailures = 50 + // Verdict is the value of a verdict key: the group and its heads hash, the trunk head of each repository it was judged // against (repository to full sha), the State (running, pass or fail), what failed first, the tests run and kept per // level, each part's time, the machines composed and the mesh-sdk version the composition ran with (ADR 0446 clause 5). type Verdict struct { - V int `json:"v"` - Group string `json:"group"` - Heads string `json:"heads"` - Trunk map[string]string `json:"trunk,omitempty"` - State string `json:"state"` - Failed FailedFirst `json:"failed,omitzero"` + V int `json:"v"` + Group string `json:"group"` + Heads string `json:"heads"` + Trunk map[string]string `json:"trunk,omitempty"` + State string `json:"state"` + Failed FailedFirst `json:"failed,omitzero"` + // Failures names every test that failed, in the order judged, each with the first line of its words (issue 1921): + // Failed keeps the first one's words whole, and a check that ran every test says all it found here. At most + // MaxFailures entries, the last of a longer list saying how many more failed. Absent in a value written before it, + // and from a verdict whose failure is no test. + Failures []FailedFirst `json:"failures,omitempty"` Levels map[string]LevelResult `json:"levels,omitempty"` Took Took `json:"took,omitzero"` Machines []string `json:"machines,omitempty"` @@ -222,6 +231,12 @@ func (v Verdict) check() error { } } named := v.Failed.Test != "" || v.Failed.Machine != "" + if len(v.Failures) > 0 && v.State != VerdictFail { + return fmt.Errorf("delivery: a %s verdict names no failures", v.State) + } + if len(v.Failures) > MaxFailures+1 { + return fmt.Errorf("delivery: the verdict names %d failures, more than %d and the count of the rest", len(v.Failures), MaxFailures) + } switch v.State { case VerdictRunning: if v.Failed != (FailedFirst{}) { diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index 22d4611..815577a 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -3,6 +3,7 @@ package delivery import ( "encoding/json" "errors" + "strconv" "strings" "testing" "time" @@ -600,3 +601,38 @@ func TestPreviewOfAHeadInConflict(t *testing.T) { t.Errorf("the old reader: %+v %v", old, err) } } + +// Issue 1921: a failed verdict names every failed test in Failures and reads back the same; an older reader's value +// without it reads too; Failures on a verdict that is no fail, and more than MaxFailures and the count of the rest, are +// refused. +func TestVerdictNamesEveryFailure(t *testing.T) { + v := passVerdict() + v.State = VerdictFail + v.Failed = FailedFirst{Test: "integration: x/a.TestA", Words: "a_test.go:9: want 1, got 2\nmore"} + v.Failures = []FailedFirst{{Test: "integration: x/a.TestA", Words: "a_test.go:9: want 1, got 2"}, {Test: "integration: x/b.TestB", Words: "b_test.go:3: boom"}} + data, err := EncodeVerdict(v) + if err != nil { + t.Fatal(err) + } + got, err := DecodeVerdict(data) + v.V = Version + if err != nil || !SameVerdict(got, v) || !strings.Contains(string(data), `"failures":[{"test":"integration: x/a.TestA"`) { + t.Fatalf("%s: %+v %v", data, got, err) + } + old := strings.Replace(string(data), string(data[strings.Index(string(data), `,"failures":`):strings.Index(string(data), `,"levels":`)]), "", 1) + if got, err := DecodeVerdict([]byte(old)); err != nil || len(got.Failures) != 0 || got.Failed != v.Failed { + t.Fatalf("a value without failures: %+v %v (%s)", got, err, old) + } + p := passVerdict() + p.Failures = v.Failures + if _, err := EncodeVerdict(p); err == nil { + t.Error("a pass with failures was written") + } + v.Failures = make([]FailedFirst, MaxFailures+2) + for i := range v.Failures { + v.Failures[i] = FailedFirst{Test: "unit: x.T" + strconv.Itoa(i)} + } + if _, err := EncodeVerdict(v); err == nil { + t.Error("a verdict naming more than MaxFailures and the rest was written") + } +}