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)

2026-10-21 19:19:14 +02:00
parent 4375ed8b2c
commit 6d0c2076a3
2 changed files with 168 additions and 1 deletions
@@ -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 {
@@ -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)
+	}
+}