A placed check ask names the packages it tests, so the first ask's other packages are not run or counted again (issue 890, ADR 0552 §4; review round 1)

This commit is contained in:
2026-10-26 14:01:05 +01:00
parent bb71310337
commit 45564e69a6
4 changed files with 39 additions and 0 deletions
+10
View File
@@ -234,6 +234,9 @@ func (s AskSteps) level(ctx context.Context, job delivery.CheckJob, level string
if !keepRunner {
runner = "" // an installed module that ignores the runner judges the level as it judges any other (ADR 0535 §2)
}
if whole && len(job.Packages) > 0 {
todo, whole = job.Packages, false // a placed ask tests its packages alone, whatever the digest said (ADR 0552 §4)
}
for _, priority := range []bool{true, false} {
args := withRunner(map[string]any{"workspace": f.Workspace, "level": level})
if job.Run != "" {
@@ -310,9 +313,16 @@ func (s AskSteps) plan(ctx context.Context, job delivery.CheckJob, level string,
return nil, true, runner != "" // a digest that names nothing or not the siblings keys no verdict: the level runs whole
}
la.Digest = &d
only := map[string]bool{}
for _, p := range job.Packages {
only[p] = true
}
var todo []string
for _, pd := range d.Packages {
p := pd.Package
if len(only) > 0 && !only[p] {
continue // a placed ask's other packages were judged by the first ask (ADR 0552 §4)
}
if job.Kept && s.Kept != nil {
tests, found, err := s.Kept(ctx, level, pd.Digest)
if err != nil && s.Logf != nil {
+16
View File
@@ -163,6 +163,22 @@ test {"level":"unit","packages":["x/pkg/a"],"workspace":"/w"}`
t.Fatalf("has %v: tested x/pkg/a %v, unmet %+v, calls %v", has, tested, u.Unmet, r.calls)
}
}
// A placed ask naming its packages tests those alone and takes no other's kept verdict, also when the level would
// run whole (ADR 0552 §4).
for _, digests := range []map[string]string{r.digests, nil} {
r.calls, r.digests = nil, digests
steps := r.steps()
steps.Has = func(string) bool { return true }
out, err := RunAsk(context.Background(), steps, ask(delivery.CheckJob{Levels: []string{delivery.LevelUnit}, Kept: true, Packages: []string{"x/pkg/a"},
Fetch: map[string]any{"repository": "novox/x", "commit": runCommit}}))
if err != nil {
t.Fatal(err)
}
u := out.(delivery.CheckAnswer).Levels[0]
if last := r.calls[len(r.calls)-1]; !strings.HasSuffix(last, `"packages":["x/pkg/a"],"workspace":"/w"}`) || len(u.Kept) != 0 || len(u.Tests) != 1 {
t.Fatalf("digests %v: calls %v, kept %+v: want x/pkg/a alone tested, nothing taken kept", digests != nil, r.calls, u.Kept)
}
}
}
// ADR 0535 §2, §4: a merge that changes the build module's own code is judged by the candidate's runner: its digest says
+8
View File
@@ -337,6 +337,9 @@ type CheckJob struct {
RunnerWhen string `json:"runner_when,omitempty"`
// Run is a try's test pattern (go test -run): each level runs it alone, digesting nothing.
Run string `json:"run,omitempty"`
// Packages, when set, are the only packages each level tests (issue 890, ADR 0552 §4): a placed ask names the
// packages the first ask left unmet, and the others are neither tested nor taken kept, so nothing is judged twice.
Packages []string `json:"packages,omitempty"`
}
func (a StepAsk) check() error {
@@ -439,6 +442,11 @@ func (c CheckJob) check(id string) error {
return err
}
}
for _, p := range c.Packages {
if p == "" || strings.ContainsAny(p, " \t\n") {
return fmt.Errorf("delivery: the check ask %s names a package %q that is no import path", id, p)
}
}
return checkFetchArgs(id, c.Fetch)
}
+5
View File
@@ -83,6 +83,11 @@ func TestAskRefusesUnknownKindAndLanguage(t *testing.T) {
}
// Issue 890: a check names a known need and round-trips it; an unknown need is refused.
c = checkAsk()
c.Check.Packages = []string{"a b"}
if _, err := EncodeStepAsk(c); err == nil {
t.Error("a package that is no import path encoded")
}
c.Check.Packages = []string{"x/pkg/a"}
c.Needs = []string{"gpu"}
if _, err := EncodeStepAsk(c); err == nil {
t.Error("an unknown need encoded")