From 9b408db24a585bd698adabd9f1cef8ee503761a3 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 25 Oct 2026 01:22:50 +0200 Subject: [PATCH] buildstep: run a check's levels side by side and answer them in level order, so the integration level no longer waits for the unit level and the first failing level is still named first (issue 1921, try speed S10) --- go/buildstep/askrun.go | 39 ++++++++++++++----- go/buildstep/askrun_levels_test.go | 60 ++++++++++++++++++++++++++++++ go/buildstep/askrun_test.go | 28 +++++++++----- 3 files changed, 109 insertions(+), 18 deletions(-) create mode 100644 go/buildstep/askrun_levels_test.go diff --git a/go/buildstep/askrun.go b/go/buildstep/askrun.go index f0cef51..6c5ebf8 100644 --- a/go/buildstep/askrun.go +++ b/go/buildstep/askrun.go @@ -8,6 +8,7 @@ import ( "slices" "sort" "strings" + "sync" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -170,16 +171,36 @@ func (s AskSteps) check(ctx context.Context, job delivery.CheckJob) (any, error) if job.Runner != "" && (job.RunnerWhen == "" || changesUnder(f.Changed, job.RunnerWhen)) { runner = job.Runner } - for _, level := range job.Levels { - la := delivery.LevelAnswer{Level: level} - passed, err := s.level(ctx, job, level, f, runner, &la) - answer.Levels = append(answer.Levels, la) - if err != nil { - return nil, err - } - if !passed { - return answer, nil // the first level that did not pass ends the check (ADR 0484 clause 13) + // The levels run side by side (issue 1921, try speed S10; the operator's yes to D1, ADR 0541): the integration level + // does not wait for the unit level. They are answered in their order, so the first level that did not pass is still + // the first failure the asker reads (ADR 0484 clause 13); the levels after it are kept in the answer for the log. + type ran struct { + la delivery.LevelAnswer + passed bool + err error + } + runs := make([]ran, len(job.Levels)) + var wg sync.WaitGroup + for i, level := range job.Levels { + wg.Add(1) + go func() { + defer wg.Done() + runs[i].la = delivery.LevelAnswer{Level: level} + runs[i].passed, runs[i].err = s.level(ctx, job, level, f, runner, &runs[i].la) + }() + } + wg.Wait() + failed := false + for _, r := range runs { + if r.err != nil { + if failed { + continue // a level after the first failure that could not run: the failure is the answer + } + answer.Levels = append(answer.Levels, r.la) + return nil, r.err } + answer.Levels = append(answer.Levels, r.la) + failed = failed || !r.passed } return answer, nil } diff --git a/go/buildstep/askrun_levels_test.go b/go/buildstep/askrun_levels_test.go new file mode 100644 index 0000000..41e1f03 --- /dev/null +++ b/go/buildstep/askrun_levels_test.go @@ -0,0 +1,60 @@ +package buildstep + +import ( + "context" + "encoding/json" + "sync" + "testing" + "time" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// 1921/S10 (the operator's yes to D1, ADR 0541): a check's levels run side by side: the integration level has started +// before the unit level ends. The answer holds the levels in their order, so with the unit level failing it still names +// the unit level first; the integration level's answer is kept after it for the log. +func TestACheckRunsItsLevelsSideBySideAndAnswersThemInOrder(t *testing.T) { + integrationStarted := make(chan struct{}) + var once sync.Once + var mu sync.Mutex + unitWaited := false + steps := AskSteps{ + Fetch: func(context.Context, map[string]any) (any, error) { + return map[string]any{"workspace": "/w", "modules": []string{"."}}, nil + }, + Test: func(ctx context.Context, args map[string]any) (any, error) { + switch args["level"] { + case delivery.LevelUnit: + select { + case <-integrationStarted: + mu.Lock() + unitWaited = true + mu.Unlock() + case <-time.After(2 * time.Second): + } + return map[string]any{"passed": false, "level": "unit"}, nil + default: + once.Do(func() { close(integrationStarted) }) + return map[string]any{"passed": true, "level": "integration"}, nil + } + }, + } + out, err := RunAsk(context.Background(), 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) + } + mu.Lock() + defer mu.Unlock() + if !unitWaited { + t.Error("the integration level did not start while the unit level ran") + } + a := out.(delivery.CheckAnswer) + if len(a.Levels) != 2 || a.Levels[0].Level != delivery.LevelUnit || a.Levels[1].Level != delivery.LevelIntegration { + t.Fatalf("levels %+v, want unit then integration", a.Levels) + } + var first struct{ Passed bool } + if err := json.Unmarshal(a.Levels[0].Tests[0], &first); err != nil || first.Passed { + t.Errorf("the first level's result %s, want the unit level's failure", a.Levels[0].Tests[0]) + } +} diff --git a/go/buildstep/askrun_test.go b/go/buildstep/askrun_test.go index 8ceb678..ca725db 100644 --- a/go/buildstep/askrun_test.go +++ b/go/buildstep/askrun_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "strings" + "sync" "testing" "time" @@ -16,6 +17,7 @@ const runCommit = "0123456789abcdef0123456789abcdef01234567" // recorder is a build module's step tools that record each call. type recorder struct { + mu sync.Mutex // a check's levels run side by side (issue 1921, S10) calls []string fetch map[string]any tests map[string]bool // package or module directory -> passed @@ -26,7 +28,9 @@ func (r *recorder) steps() AskSteps { call := func(name string, answer func(args map[string]any) any) StepFunc { return func(ctx context.Context, args map[string]any) (any, error) { b, _ := json.Marshal(args) + r.mu.Lock() r.calls = append(r.calls, name+" "+string(b)) + r.mu.Unlock() return answer(args), nil } } @@ -110,8 +114,8 @@ 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 -// level after one that did not pass (ADR 0484 clause 13). +// verdicts, tests the packages whose tests changed first, then the rest also after a failure (issue 1921); the levels run +// side by side and are answered in order, the first that did not pass first (ADR 0484 clause 13; issue 1921 S10). func TestRunAskChecksTheLevelsOfOneCheck(t *testing.T) { r := &recorder{ fetch: map[string]any{"workspace": "/w", "changed": []string{"pkg/b/b_test.go"}}, @@ -123,7 +127,13 @@ func TestRunAskChecksTheLevelsOfOneCheck(t *testing.T) { if err != nil { t.Fatal(err) } - got := strings.Join(r.calls, "\n") + var unit []string + for _, c := range r.calls { + if !strings.Contains(c, `"level":"integration"`) { + unit = append(unit, c) + } + } + got := strings.Join(unit, "\n") want := `fetch {"base":"` + runCommit + `","commit":"` + runCommit + `","repository":"novox/x"} digest {"level":"unit","workspace":"/w"} test {"level":"unit","packages":["x/pkg/b"],"workspace":"/w"} @@ -132,8 +142,8 @@ test {"level":"unit","packages":["x/pkg/a"],"workspace":"/w"}` t.Fatalf("calls:\n%s\nwant:\n%s", got, want) } a := out.(delivery.CheckAnswer) - if len(a.Levels) != 1 { - t.Fatalf("levels %+v, want the unit level alone", a.Levels) + if len(a.Levels) != 2 || a.Levels[1].Level != delivery.LevelIntegration { + t.Fatalf("levels %+v, want the unit level first, the integration level after it", 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 { @@ -220,8 +230,8 @@ func TestRunAskRunsEveryPackageAfterAFailure(t *testing.T) { 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) + if len(a.Levels) != 2 || len(a.Levels[0].Tests) != 2 || a.Levels[0].Level != delivery.LevelIntegration { + t.Fatalf("levels %+v, want the first level with both packages' results, in the order asked", a.Levels) } for i, raw := range a.Levels[0].Tests { var res struct{ Passed bool } @@ -239,7 +249,7 @@ func TestRunAskRunsEveryPackageAfterAFailure(t *testing.T) { 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) != 5 || len(a.Levels) != 2 || len(a.Levels[0].Tests) != 2 || a.Levels[0].Level != delivery.LevelUnit { + t.Fatalf("calls %v, levels %+v: want both directories of each level, the unit level first", r.calls, a.Levels) } }