From 6d0c2076a3ba930a8e1960b6301d93cf1e6e86bd Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:19:14 +0200 Subject: [PATCH] Give the checker's verdict a skipped count, a composition and the over-bound levels, so the line never hides skips or claims a composition not run (issue 1672) --- go/delivery/checker.go | 61 +++++++++++++++++++- go/delivery/checker_test.go | 108 ++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 1 deletion(-) diff --git a/go/delivery/checker.go b/go/delivery/checker.go index bdb57e9..acc287b 100644 --- a/go/delivery/checker.go +++ b/go/delivery/checker.go @@ -88,6 +88,25 @@ func ParseCheckerVerdictKey(key string) (group, headsHash string, err error) { type LevelResult struct { Run int `json:"run"` Kept int `json:"kept"` + // Skipped is the tests the run skipped, so a pass never hides them (issue 1251, ADR 0484 clause 7). + Skipped int `json:"skipped,omitempty"` +} + +// The states of a Composition (ADR 0484 clause 6): the composition check is the assigner's, so until it delivers a +// verdict says CompositionNotRun, and a line never says the composition passed on that. +const ( + CompositionNotRun = "not-run" + CompositionPass = "pass" + CompositionFail = "fail" +) + +// Composition is what the check of the machines' composition made of a verdict: its State (not-run, pass or fail), the +// Words of a refusal, the mesh-sdk commit it composed with and the machines composed (ADR 0482 clause 9, ADR 0484 clause 6). +type Composition struct { + State string `json:"state"` + Words string `json:"words,omitempty"` + SDK string `json:"sdk,omitempty"` + Machines []string `json:"machines,omitempty"` } // Took is the time of each part of a check in integer milliseconds (ADR 0447 clause 3): waiting in the queue, cloning, @@ -123,6 +142,33 @@ type Verdict struct { Took Took `json:"took,omitzero"` Machines []string `json:"machines,omitempty"` SDK string `json:"sdk,omitempty"` + // Composition is the composition check's result; absent in a value written before it (ADR 0484 clause 6). + Composition *Composition `json:"composition,omitempty"` + // OverBound names the levels whose running went over their bound; it does not change the State (ADR 0453 clause 9). + OverBound []string `json:"over-bound,omitempty"` +} + +func (c *Composition) check() error { + if c == nil { + return nil + } + switch c.State { + case CompositionNotRun: + if c.Words != "" || c.SDK != "" || len(c.Machines) > 0 { + return fmt.Errorf("delivery: the verdict's composition is not run and says nothing of words, the mesh-sdk commit or machines") + } + case CompositionPass: + if c.SDK == "" { + return fmt.Errorf("delivery: the verdict's composition passed and names no mesh-sdk commit") + } + case CompositionFail: + if c.Words == "" { + return fmt.Errorf("delivery: the verdict's composition failed and gives no words") + } + default: + return fmt.Errorf("delivery: the verdict's composition state %q is not %s, %s or %s", c.State, CompositionNotRun, CompositionPass, CompositionFail) + } + return nil } func (v Verdict) check() error { @@ -141,10 +187,23 @@ func (v Verdict) check() error { if err := checkLevel(level); err != nil { return err } - if r.Run < 0 || r.Kept < 0 { + if r.Run < 0 || r.Kept < 0 || r.Skipped < 0 { return fmt.Errorf("delivery: the verdict's %s level counts a negative number of tests", level) } } + if err := v.Composition.check(); err != nil { + return err + } + seen := map[string]bool{} + for _, level := range v.OverBound { + if err := checkLevel(level); err != nil { + return fmt.Errorf("delivery: the verdict's over-bound names: %w", err) + } + if seen[level] { + return fmt.Errorf("delivery: the verdict's over-bound names the level %s twice", level) + } + seen[level] = true + } for part, ms := range map[string]int64{"queue": v.Took.Queue, "clone": v.Took.Clone, "unit": v.Took.Unit, "integration-setup": v.Took.IntegrationSetup, "integration-run": v.Took.IntegrationRun, "composition": v.Took.Composition} { if ms < 0 { diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index 2dd939f..f8542c4 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -1,6 +1,7 @@ package delivery import ( + "encoding/json" "errors" "strings" "testing" @@ -220,3 +221,110 @@ func TestCheckerVersions(t *testing.T) { } } } + +// oldVerdictJSON is a verdict as the SDK before issue 1672 wrote it (copied from EncodeVerdict of passVerdict's shape +// at the commit before this change): no skipped count, no composition, no over-bound names. +const oldVerdictJSON = `{"v":1,"group":"feat/x","heads":"4b5491557853","trunk":{"novox/mesh-control":"bbbb","novox/mesh-sdk":"aaaa"},"state":"pass","levels":{"integration":{"run":3,"kept":40},"unit":{"run":12,"kept":301}},"took":{"queue":1200,"clone":800,"unit":950,"integration-setup":2100,"integration-run":7400,"composition":3300},"machines":["ace","g14","novox","shanks"],"sdk":"v0.1.14-0.20261019091011-aa68b67389c9"}` + +// oldVerdict is the reader of today's shape: the struct as it stood before the new fields. +type oldVerdict struct { + V int `json:"v"` + Group string `json:"group"` + Heads string `json:"heads"` + State string `json:"state"` + Levels map[string]struct{ Run, Kept int } + Machines []string + SDK string +} + +// 1672/AC1: the skipped count, the composition and the over-bound names round trip and appear in the bytes. +func TestVerdictCarriesSkippedCompositionAndOverBound(t *testing.T) { + v := passVerdict() + v.Levels[LevelUnit] = LevelResult{Run: 12, Kept: 301, Skipped: 4} + v.Composition = &Composition{State: CompositionPass, SDK: v.SDK, Machines: []string{"ace", "novox"}} + v.OverBound = []string{LevelIntegration} + data, err := EncodeVerdict(v) + if err != nil { + t.Fatal(err) + } + for _, field := range []string{`"skipped":4`, `"composition":{"state":"pass"`, `"over-bound":["integration"]`} { + if !strings.Contains(string(data), field) { + t.Errorf("the value lacks %s: %s", field, data) + } + } + got, err := DecodeVerdict(data) + v.V = Version + if err != nil || !SameVerdict(got, v) || got.Levels[LevelUnit].Skipped != 4 || got.Composition == nil || got.Composition.State != CompositionPass { + t.Errorf("round trip = %+v, %v", got, err) + } + notRun := passVerdict() + notRun.Composition = &Composition{State: CompositionNotRun} + if _, err := EncodeVerdict(notRun); err != nil { + t.Errorf("a composition not yet run refused: %v", err) + } + failed := passVerdict() + failed.State, failed.Failed = VerdictFail, FailedFirst{Machine: "ace", Words: "x"} + failed.Composition = &Composition{State: CompositionFail, Words: "path /etc/x is owned by two modules", SDK: failed.SDK, Machines: []string{"ace"}} + if _, err := EncodeVerdict(failed); err != nil { + t.Errorf("a failed composition refused: %v", err) + } +} + +// 1672/AC1: an invalid new value is refused, naming the field. +func TestVerdictNewFieldsChecked(t *testing.T) { + cases := map[string]struct { + edit func(*Verdict) + words []string + }{ + "a negative skipped count": {func(v *Verdict) { v.Levels[LevelUnit] = LevelResult{Run: 1, Skipped: -1} }, []string{"unit", "negative"}}, + "an unknown composition": {func(v *Verdict) { v.Composition = &Composition{State: "maybe"} }, []string{"composition", "maybe"}}, + "a composition with no state": {func(v *Verdict) { v.Composition = &Composition{} }, []string{"composition"}}, + "a failed composition without words": {func(v *Verdict) { v.Composition = &Composition{State: CompositionFail} }, []string{"composition", "words"}}, + "a passed composition without the sdk": {func(v *Verdict) { v.Composition = &Composition{State: CompositionPass, Machines: []string{"ace"}} }, []string{"composition", "sdk"}}, + "a not-run composition naming machines": {func(v *Verdict) { v.Composition = &Composition{State: CompositionNotRun, Machines: []string{"ace"}} }, []string{"composition", "not run"}}, + "an unknown over-bound level": {func(v *Verdict) { v.OverBound = []string{"e2e"} }, []string{"e2e"}}, + "an over-bound level named twice": {func(v *Verdict) { v.OverBound = []string{LevelUnit, LevelUnit} }, []string{"over-bound", "unit"}}, + } + for name, c := range cases { + v := passVerdict() + c.edit(&v) + _, err := EncodeVerdict(v) + if err == nil { + t.Errorf("%s accepted", name) + continue + } + for _, w := range c.words { + if !strings.Contains(err.Error(), w) { + t.Errorf("%s: error %q does not name %q", name, err, w) + } + } + } +} + +// 1672/AC2: a value written by the SDK before this change decodes with the new reader, with the new fields empty, and +// the new SDK writes the same bytes for it; a value with the new fields decodes under a reader of today's shape. +func TestVerdictOldAndNewReadersAgree(t *testing.T) { + v, err := DecodeVerdict([]byte(oldVerdictJSON)) + if err != nil { + t.Fatalf("the new reader of an old value: %v", err) + } + if v.Composition != nil || v.OverBound != nil || v.Levels[LevelUnit].Skipped != 0 || v.Levels[LevelUnit].Run != 12 { + t.Errorf("an old value grew fields: %+v", v) + } + again, err := EncodeVerdict(v) + if err != nil || string(again) != oldVerdictJSON { + t.Errorf("the new SDK writes %s, %v for an old value, want today's bytes", again, err) + } + n := passVerdict() + n.Levels[LevelUnit] = LevelResult{Run: 12, Kept: 301, Skipped: 4} + n.Composition = &Composition{State: CompositionNotRun} + n.OverBound = []string{LevelUnit} + data, err := EncodeVerdict(n) + if err != nil { + t.Fatal(err) + } + var ov oldVerdict + if err := json.Unmarshal(data, &ov); err != nil || ov.Group != "feat/x" || ov.State != VerdictPass || ov.Levels[LevelUnit].Run != 12 || ov.SDK == "" { + t.Errorf("the old reader of a new value: %+v, %v", ov, err) + } +}