diff --git a/CHANGELOG.md b/CHANGELOG.md index e53d1b09..21df68d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,15 @@ ### Added +- **`context_for_task` names the tests that run your seed, as a ready + `run_task` call.** The affected-tests estimate named only packages + (`./internal/tools/... — 1808 test(s)`), so agents built their own `-run` + filter or ran the whole package. Under a package row, one line now names up to + eight tests a call edge reaches: tests that call a seed, directly or through a + caller, come first and closest first, then tests of the declarations it calls. + With a Go test target the line is a `run_task(slot:"test", target:…, run:…)` + call whose filter `run_task` accepts, with the rest counted. Tests that only + sit in the package are never named, and a stale index names none. - **The Go call graph resolves method calls on local variables of evident type.** `x.M()` now links to the method of `x`'s type when the enclosing function declares `x` once as `x := T{}`, `&T{}`, `new(T)`, `var x T`, `x := y.(T)`, or diff --git a/internal/tools/context_affected.go b/internal/tools/context_affected.go index 15cd7c4a..9939c8e1 100644 --- a/internal/tools/context_affected.go +++ b/internal/tools/context_affected.go @@ -4,8 +4,12 @@ import ( "cmp" "context" "fmt" + "math" "path" "slices" + "strings" + "unicode" + "unicode/utf8" "github.com/plumbkit/plumb/internal/textfmt" "github.com/plumbkit/plumb/internal/topology" @@ -16,8 +20,15 @@ import ( // This is topology_affected's own gather (collectAffected) run over the pack's // seeds, kept to one row per test package: the directory (or the run_task target // the calling agent's test command accepts), how many tests live there and why the -// package is implicated. It names no individual test, and it runs none (Invariant -// 10): every word here says "estimate" and "implicated", never "passed" or "ran". +// package is implicated. It runs none (Invariant 10): every word here says +// "estimate" and "implicated", never "passed" or "ran". +// +// Under a package row it may name a few tests, but only the ones a call edge +// reaches: a test that calls a seed, or a declaration next to one, directly or +// through callers. Co-location says only that a test sits in the package, so a +// co-located test is counted, never named. With a Go test target the names come as +// a ready run_task filter, which is the step agents otherwise spell by hand before +// their first test run (PLAN-543: every arm D agent built its own -run filter). // // The estimate rests on the index, so a missing, failing or foreign index gives // none at all (the index gap already says so), and the scope filter applies to @@ -30,6 +41,10 @@ const ( // contextAffectedNeighbours bounds the neighbouring declarations a change may // extend into that the estimate also starts from. contextAffectedNeighbours = 8 + // contextNamedPerPackage and contextNamedPackages bound the named tests: past a + // handful the line stops being a first run and becomes the package again. + contextNamedPerPackage = 8 + contextNamedPackages = 3 ) // contextAffected is the estimate and what bounded it. @@ -37,11 +52,12 @@ type contextAffected struct { Ran bool Packages []affectedPackage Scope TestScope - Neighbour map[string]bool // directories implicated only through a neighbour of a seed - Excluded int // tests left out by within, corpora or plumb's own state - Cut bool // more packages than the cap - GraphCut bool // the dependent walk hit its traversal budget - Deadline bool // the graph deadline passed, so the estimate is partial + Neighbour map[string]bool // directories implicated only through a neighbour of a seed + Named map[string]namedTests // per package directory: the tests a call edge reaches + Excluded int // tests left out by within, corpora or plumb's own state + Cut bool // more packages than the cap + GraphCut bool // the dependent walk hit its traversal budget + Deadline bool // the graph deadline passed, so the estimate is partial } // affectedTests gathers the estimate. ctx carries the call's graph deadline. @@ -53,8 +69,8 @@ func (c *ContextCollector) affectedTests(ctx context.Context, pack *contextPack, if len(roots)+len(dirs) == 0 { return // no code seed: nothing to estimate from, and nothing to disclaim } - near := affectedNeighbours(pack) - res, err := collectAffected(ctx, index.store, append(roots, near...), dirs, contextAffectedPackages) + near, aside := affectedNeighbours(pack) + res, err := collectAffectedNear(ctx, index.store, roots, near, aside, dirs, contextAffectedPackages) if err != nil || res == nil { return } @@ -64,14 +80,94 @@ func (c *ContextCollector) affectedTests(ctx context.Context, pack *contextPack, Ran: true, Packages: seedPackagesFirst(aggregateTestsByPackage(kept), nearDirs), Excluded: excluded, Neighbour: nearDirs, Cut: res.Truncated, GraphCut: res.GraphTruncated, Deadline: ctx.Err() != nil, } + pack.Affected.Named = edgeReachedTests(pack.Affected.Packages, indexFreshness(index.store.Health())) for i := range pack.Affected.Packages { - pack.Affected.Packages[i].Tests = nil // the estimate names packages, never tests + pack.Affected.Packages[i].Tests = nil // only edgeReachedTests' names survive } if c.testScope != nil { pack.Affected.Scope = c.testScope(ctx) } } +// namedTests are the tests of one package that a call edge reaches, those that +// exercise a seed first and the closest first among them (fewest hops, then +// name), and how many there were. +type namedTests struct { + Names []string + Total int +} + +// edgeReachedTests names, per package, the tests the dependent walk reached by +// an edge rather than by sitting in an implicated directory. Only test functions +// go test -run can select are named; the first contextNamedPackages packages that +// have any are kept, in the rows' order. +// +// freshness is the index's (indexFreshness). A stale graph may name a test that +// no longer reaches the seed, and a wrong name costs the agent a run, so a stale +// index names none; its gap already says it is stale, and the package rows stay. +func edgeReachedTests(pkgs []affectedPackage, freshness string) map[string]namedTests { + out := map[string]namedTests{} + if freshness == "stale" { + return out + } + for _, pk := range pkgs { + if len(out) == contextNamedPackages { + break + } + var reached []affectedTest + seen := map[string]bool{} + for _, t := range pk.Tests { + if t.Confidence <= colocatedConfidence || !runnableTestName(t.Node.Name) || seen[t.Node.Name] { + continue + } + seen[t.Node.Name] = true + reached = append(reached, t) + } + if len(reached) == 0 { + continue + } + // A test that exercises a seed comes before one that only reaches a callee or + // sibling of it; then fewest hops, where a test whose connecting edge was + // filtered out of the subgraph has no hop count (0) and goes last. Name breaks + // ties, so the line is the same on every call. + slices.SortFunc(reached, func(a, b affectedTest) int { + if c := cmp.Compare(boolInt(a.Aside), boolInt(b.Aside)); c != 0 { + return c + } + if c := cmp.Compare(hopRank(a.Hops), hopRank(b.Hops)); c != 0 { + return c + } + return cmp.Compare(a.Node.Name, b.Node.Name) + }) + n := namedTests{Total: len(reached)} + for _, t := range reached[:min(len(reached), contextNamedPerPackage)] { + n.Names = append(n.Names, t.Node.Name) + } + out[pk.Dir] = n + } + return out +} + +// hopRank orders a known hop count before an unknown one (0). +func hopRank(h int) int { + if h == 0 { + return math.MaxInt + } + return h +} + +// runnableTestName is a top-level function go test -run selects: Test, Example or +// Fuzz followed by nothing or a non-lowercase rune, as the testing package +// requires. A benchmark needs -bench, and a helper is not a test at all. +func runnableTestName(name string) bool { + for _, p := range []string{"Test", "Example", "Fuzz"} { + if rest, ok := strings.CutPrefix(name, p); ok { + return rest == "" || !unicode.IsLower([]rune(rest)[0]) + } + } + return false +} + // affectedRoots lists the code symbols the estimate starts from, and the // directories whose co-located tests it may add. A symbol seed starts from its own // node, a covered code file from its declarations; a file the index cannot parse @@ -100,20 +196,37 @@ func affectedRoots(ctx context.Context, store *topology.Store, seeds []contextSe // away) that a change to the seeds may extend into, so their tests are implicated // too. They matter only for intent=change: understanding code changes nothing. // Each already passed the scope filter in the walk. -func affectedNeighbours(p *contextPack) []topology.Node { +// +// aside holds the neighbours that are not callers of a seed (its callees and +// siblings): a test of one of those does not run the seed, so it ranks after +// every test that does. A caller's tests do run the seed, one hop further out. +// +// near is in walk order, callers first and then by name, whatever the ranking +// chose them in: a path that does not run the seed is then never recorded +// before one that does, and the same pack walks in the same order every time. +func affectedNeighbours(p *contextPack) (near []topology.Node, aside map[int64]bool) { if p.Intent != contextIntentChange { - return nil + return nil, nil } - var out []topology.Node + aside = map[int64]bool{} for _, r := range p.Related { - if len(out) == contextAffectedNeighbours { + if len(near) == contextAffectedNeighbours { break } if r.Dist == 1 && !r.Gap && !r.Withheld && r.Evidence >= evidenceDerived && !isTestNode(r.Node) { - out = append(out, r.Node) + near = append(near, r.Node) + if r.CallerOf == 0 { + aside[r.Node.ID] = true + } } } - return out + slices.SortStableFunc(near, func(a, b topology.Node) int { + if c := cmp.Compare(boolInt(aside[a.ID]), boolInt(aside[b.ID])); c != 0 { + return c + } + return cmp.Compare(a.Name, b.Name) + }) + return near, aside } // neighbourDirs are the directories only a neighbour of a seed puts in play: not @@ -193,10 +306,76 @@ func (p *contextPack) affectedLines() []packLine { text := fmt.Sprintf(" %s — %d test(s); %s", textfmt.TerminalSafeLine(packageRunLabel(p.Affected.Scope, pk.Dir)), pk.Count, affectedReasonWords(pk.Reason, pk.Dir, p.Affected)) ls = append(ls, packLine{text: text, prio: prioAffected, section: secAffected, class: classAffected}) + if named, ok := p.Affected.Named[pk.Dir]; ok { + ls = append(ls, packLine{text: namedTestsLine(p.Affected.Scope, pk.Dir, named), prio: prioAffected, section: secAffected, class: classAffected}) + } } return ls } +// namedTestsReach is how a named test is connected to the seeds. It is the walk's +// own claim and no more: the edges are the index's, and nothing was run. +const namedTestsReach = "call a seed or a declaration next to one, directly or through callers (index call edges)" + +// namedTestsLine renders one package's edge-reached tests. With a Go package +// target it is a run_task call ready to paste, its filter cut to the names that +// fit the filter's length limit; otherwise it is the names. The line never says +// the tests ran or passed. +func namedTestsLine(scope TestScope, dir string, n namedTests) string { + names := n.Names + if target, ok := testTarget(scope, dir); ok && scope.Style == TargetGoPackage { + for len(names) > 0 && validateRunFilter("run", runFilterFor(names)) != nil { + names = names[:len(names)-1] + } + if len(names) > 0 { + return fmt.Sprintf(" run first: run_task(slot:\"test\", target:%q, run:%q) — %s %s%s", + target, runFilterFor(names), namedCount(len(names), n.Total), namedTestsReach, namedMore(len(names), n.Total)) + } + names = n.Names + } + return fmt.Sprintf(" %s %s: %s%s", namedCount(len(names), n.Total), namedTestsReach, + textfmt.TerminalSafeLine(strings.Join(names, ", ")), namedMore(len(names), n.Total)) +} + +// runFilterFor anchors the names as one group: go test -run splits a pattern only +// at a top-level |, so the alternation inside the parentheses stays whole. The +// names' common prefix is written once (^TestMoveSymbol_(Refusals|CreateDestination)$), +// which fits more of them in run_task's filter limit. Test names are identifiers, +// so nothing in them needs escaping. +func runFilterFor(names []string) string { + if len(names) == 1 { + return "^(" + names[0] + ")$" + } + prefix := names[0] + for _, n := range names[1:] { + for !strings.HasPrefix(n, prefix) { + prefix = prefix[:len(prefix)-1] + } + } + for !utf8.ValidString(prefix) { // never split a rune between prefix and rest + prefix = prefix[:len(prefix)-1] + } + rests := make([]string, len(names)) + for i, n := range names { + rests[i] = n[len(prefix):] + } + return "^" + prefix + "(" + strings.Join(rests, "|") + ")$" +} + +func namedCount(shown, total int) string { + if shown == total { + return fmt.Sprintf("%d test(s)", total) + } + return fmt.Sprintf("%d of %d test(s) that", shown, total) +} + +func namedMore(shown, total int) string { + if shown == total { + return "" + } + return fmt.Sprintf("; %d more are not named", total-shown) +} + // affectedGaps words what bounded the estimate. An estimate with no test in it is // not proof that nothing exercises the seeds. func (p *contextPack) affectedGaps() []string { diff --git a/internal/tools/context_affected_named_test.go b/internal/tools/context_affected_named_test.go new file mode 100644 index 00000000..0a1d2ef2 --- /dev/null +++ b/internal/tools/context_affected_named_test.go @@ -0,0 +1,261 @@ +package tools + +import ( + "context" + "fmt" + "maps" + "regexp" + "slices" + "strings" + "testing" + + "github.com/plumbkit/plumb/internal/topology" +) + +// context_affected_named_test.go — the named tests under an affected-package row +// (PLAN-543): only tests a call edge reaches, closest first, as a run_task filter. + +// C01's cart tests reach Total only through a same-package NAME match, which is no +// call edge, so the cart row names none; C02's pricing test calls Apply through a +// resolved edge and is named (the control: the same fixture, the same section). +func TestContextForTask_Affected_NamesOnlyTestsACallEdgeReaches(t *testing.T) { + s := newShop(t) + _, out := runCase(t, s, oracle(t, "C01")) + rows, sub := sectionRowsAndSubRows(out, affectedHeading) + if len(rows) != 1 || !strings.HasPrefix(rows[0], " ./cart/... — ") { + t.Fatalf("control: C01 should have the one cart row, got %q", rows) + } + if len(sub[rows[0]]) != 0 { + t.Errorf("tests no call edge reaches were named: %q\n%s", sub[rows[0]], out) + } + + _, out = runCase(t, s, oracle(t, "C02")) + rows, sub = sectionRowsAndSubRows(out, affectedHeading) + var pricing string + for _, r := range rows { + if strings.HasPrefix(r, " ./pricing/... — ") { + pricing = r + } + } + want := []string{` run first: run_task(slot:"test", target:"./pricing/...", run:"^(TestApplyPercent)$") — 1 test(s) ` + namedTestsReach} + if !slices.Equal(sub[pricing], want) { + t.Errorf("pricing's named tests = %q, want %q\n%s", sub[pricing], want, out) + } + for _, r := range rows { + if r != pricing && len(sub[r]) != 0 { + t.Errorf("row %q names tests no call edge reaches: %q", r, sub[r]) + } + } +} + +// Without a Go package target there is no run_task filter to offer: the names +// stand alone, and the line still claims only the call edges. +func TestContextForTask_Affected_NamedTestsWithoutATargetAreJustNames(t *testing.T) { + root := copyShopFixture(t) + s := newShopTool(t, openShopStore(t, root), root) + s.collector = s.collector.WithTestScope(func(context.Context) TestScope { return TestScope{} }) + s.tool = NewContextForTask(s.collector) + _, out := runCase(t, s, oracle(t, "C06")) + rows, sub := sectionRowsAndSubRows(out, affectedHeading) + if len(rows) == 0 || !strings.HasPrefix(rows[0], " pricing — ") { + t.Fatalf("control: C06's first row should name the pricing directory, got %q\n%s", rows, out) + } + want := []string{fmt.Sprintf(" 1 test(s) %s: TestApplyPercent", namedTestsReach)} + if !slices.Equal(sub[rows[0]], want) { + t.Errorf("named tests = %q, want %q", sub[rows[0]], want) + } +} + +func reachedTest(name string, conf float64, hops int) affectedTest { + return affectedTest{Node: topology.Node{Name: name, Kind: topology.KindTest, Path: "p/x_test.go"}, Confidence: conf, Hops: hops} +} + +// The naming rules, one at a time: edge-reached only, runnable names only, closest +// first with the name breaking ties and an unknown hop count last, one name per +// test, the per-package and package caps, and nothing at all from a stale index. +func TestEdgeReachedTests_Rules(t *testing.T) { + pkg := affectedPackage{Dir: "p", Tests: []affectedTest{ + reachedTest("TestCoLocated", colocatedConfidence, 0), // no edge: never named + reachedTest("TestFar", graphEdgeBaseline, 2), + reachedTest("TestUnknownHops", graphEdgeBaseline, 0), // edge filtered from the subgraph + reachedTest("TestNearB", 0.85, 1), + reachedTest("TestNearA", 0.85, 1), + reachedTest("TestNearA", 0.85, 1), // the same test twice: named once + reachedTest("BenchmarkNear", 0.85, 1), // needs -bench, not -run + reachedTest("Testify", 0.85, 1), // a lowercase rune after Test: not a test + reachedTest("helperNear", 0.85, 1), // a helper is not a test + reachedTest("Test", 0.85, 1), // bare Test is a test + reachedTest("ExampleNear", 0.85, 1), // go test -run selects examples + reachedTest("FuzzNear", 0.85, 1), // and fuzz targets + reachedTest("Test_underscore", 0.85, 1), // a non-letter after Test is fine + reachedTest("TestÉclair", 0.85, 1), // so is a non-lowercase letter + reachedTest("TestNearC", 0.85, 1), // ninth runnable: past the cap + reachedTest("TestNearD", 0.85, 1), // tenth + reachedTest("TestCoLocatedToo", colocatedConfidence, 0), + }} + got := edgeReachedTests([]affectedPackage{pkg}, "fresh")["p"] + // Byte order: "_" (0x5f) sorts before "É" (0xc3), so TestÉclair is the one cut. + wantNames := []string{"ExampleNear", "FuzzNear", "Test", "TestNearA", "TestNearB", "TestNearC", "TestNearD", "Test_underscore"} + if !slices.Equal(got.Names, wantNames) { + t.Errorf("names = %q, want %q", got.Names, wantNames) + } + if got.Total != 11 { + t.Errorf("total = %d, want 11 (9 one-hop runnable, TestFar, TestUnknownHops)", got.Total) + } + + asideOne := reachedTest("TestAsideOne", 0.85, 1) + asideOne.Aside = true // reaches only a callee of the seed: it does not run the seed + far := edgeReachedTests([]affectedPackage{{Dir: "q", Tests: []affectedTest{ + asideOne, reachedTest("TestUnknown", graphEdgeBaseline, 0), reachedTest("TestThree", 0.85, 3), reachedTest("TestOne", 0.85, 1), + }}}, "building")["q"] + if want := []string{"TestOne", "TestThree", "TestUnknown", "TestAsideOne"}; !slices.Equal(far.Names, want) { + t.Errorf("order = %q, want %q: tests that run a seed first, by hops, an aside test last however close "+ + "(and an index mid-build still names: its edges are real)", far.Names, want) + } + + var many []affectedPackage + for _, d := range []string{"a", "b", "c", "d"} { + many = append(many, affectedPackage{Dir: d, Tests: []affectedTest{reachedTest("TestX", 0.85, 1)}}) + } + many = append([]affectedPackage{{Dir: "none", Tests: []affectedTest{reachedTest("TestY", colocatedConfidence, 0)}}}, many...) + if got := slices.Sorted(maps.Keys(edgeReachedTests(many, "fresh"))); !slices.Equal(got, []string{"a", "b", "c"}) { + t.Errorf("packages named = %q, want the first %d that have any, in row order", got, contextNamedPackages) + } + + if got := edgeReachedTests([]affectedPackage{pkg}, "stale"); len(got) != 0 { + t.Errorf("a stale index named tests: %v", got) + } +} + +// The run filter must be one run_task accepts: names are dropped from the end +// until it fits, and the count says how many are left out. +func TestNamedTestsLine_FilterFitsWhatRunTaskAccepts(t *testing.T) { + names := make([]string, 0, contextNamedPerPackage) + for i := range contextNamedPerPackage { + // Distinct after "Test0", so the shared prefix saves almost nothing. + names = append(names, fmt.Sprintf("Test%02dAVeryLongNameThatDescribesBehaviourInDetail", i)) + } + line := namedTestsLine(TestScope{Language: "go", Style: TargetGoPackage}, "pkg", namedTests{Names: names, Total: 20}) + _, rest, ok := strings.Cut(line, `run:"`) + if !ok { + t.Fatalf("no run filter in %q", line) + } + filter, _, _ := strings.Cut(rest, `")`) + if err := validateRunFilter("run", filter); err != nil { + t.Fatalf("the offered filter is one run_task refuses: %v\n%s", err, line) + } + kept := strings.Count(filter, "|") + 1 + if kept >= len(names) || kept == 0 { + t.Fatalf("control: %d names should not all fit in the filter limit, kept %d", len(names), kept) + } + if want := fmt.Sprintf("%d of 20 test(s) that %s; %d more are not named", kept, namedTestsReach, 20-kept); !strings.HasSuffix(line, want) { + t.Errorf("line = %q,\nwant the suffix %q", line, want) + } +} + +// The filter writes the common prefix once and selects exactly the names: go +// test's own matcher (one part, since the alternation sits in a group) accepts +// each name and rejects a near miss. +func TestRunFilterFor(t *testing.T) { + for _, tc := range []struct { + names []string + want string + miss string + }{ + {[]string{"TestMoveSymbol_Refusals", "TestMoveSymbol_CreateDestination"}, "^TestMoveSymbol_(Refusals|CreateDestination)$", "TestMoveSymbol_Refusals2"}, + {[]string{"TestOne"}, "^(TestOne)$", "TestOneMore"}, + {[]string{"TestA", "TestAB"}, "^TestA(|B)$", "TestABC"}, + {[]string{"ExampleX", "TestX"}, "^(ExampleX|TestX)$", "TestXY"}, + {[]string{"TestÉa", "TestÈb"}, "^Test(Éa|Èb)$", "TestÉb"}, // É and È share their first byte + } { + got := runFilterFor(tc.names) + if got != tc.want { + t.Errorf("runFilterFor(%q) = %q, want %q", tc.names, got, tc.want) + continue + } + re := regexp.MustCompile(got) + for _, n := range tc.names { + if !re.MatchString(n) { + t.Errorf("%q does not select %q", got, n) + } + } + if re.MatchString(tc.miss) { + t.Errorf("%q also selects %q", got, tc.miss) + } + } +} + +// A test reached twice keeps the shorter path of the same kind. The walk records +// paths that run the change before aside ones, so neither kind replaces the other. +func TestBetterPath(t *testing.T) { + seed2 := affectedTest{Hops: 2} + aside3 := affectedTest{Hops: 3, Aside: true} + for _, tc := range []struct { + name string + have affectedTest + hops int + aside bool + want bool + }{ + {"an aside path never replaces one that runs the change", seed2, 1, true, false}, + {"nor the other way: the walk never records the aside one first", aside3, 1, false, false}, + {"fewer hops win", seed2, 1, false, true}, + {"fewer hops win among aside paths too", aside3, 2, true, true}, + {"equal hops do not", seed2, 2, false, false}, + {"more hops do not", seed2, 3, false, false}, + {"any count beats an unknown one", affectedTest{}, 4, false, true}, + } { + if got := betterPath(tc.have, tc.hops, tc.aside); got != tc.want { + t.Errorf("%s: betterPath = %v, want %v", tc.name, got, tc.want) + } + } +} + +// A test reached from two roots keeps the shorter path; nodes the edges do not +// connect to the root have no hop count. +func TestInwardHops(t *testing.T) { + const root, a, b, c, loose = 1, 2, 3, 4, 5 + edges := []topology.Edge{ + {FromID: a, ToID: root}, // a depends on root: 1 hop + {FromID: b, ToID: a}, // b depends on a: 2 hops + {FromID: c, ToID: b}, // c: 3 hops through b ... + {FromID: c, ToID: root}, // ... but 1 directly + {FromID: loose, ToID: 99}, + } + got := inwardHops(root, edges) + want := map[int64]int{a: 1, b: 2, c: 1} + if !maps.Equal(got, want) { + t.Errorf("inwardHops = %v, want %v", got, want) + } +} + +// The neighbours walk callers of a seed first, then by name, whatever order the +// ranking put them in: that is what keeps a path that runs the seed ahead of an +// aside one. Only resolved, non-test, one-hop neighbours are walked at all. +func TestAffectedNeighbours_WalkCallersFirstThenByName(t *testing.T) { + rel := func(id int64, name string, callerOf int64) contextRelated { + return contextRelated{Node: topology.Node{ID: id, Name: name, Path: "p/p.go"}, Dist: 1, Evidence: evidenceDerived, CallerOf: callerOf} + } + test := rel(9, "TestSeed", 1) + test.Node.Kind = topology.KindTest + far := rel(8, "Far", 1) + far.Dist = 2 + p := &contextPack{Intent: contextIntentChange, Related: []contextRelated{ + rel(2, "Beta", 0), rel(3, "Gamma", 1), test, rel(4, "Alpha", 0), far, rel(5, "Delta", 1), + }} + near, aside := affectedNeighbours(p) + names := make([]string, 0, len(near)) + for _, n := range near { + names = append(names, n.Name) + } + // Name order alone would walk the callees Alpha and Beta first. + if want := []string{"Delta", "Gamma", "Alpha", "Beta"}; !slices.Equal(names, want) { + t.Errorf("walk order = %q, want %q", names, want) + } + if want := map[int64]bool{2: true, 4: true}; !maps.Equal(aside, want) { + t.Errorf("aside = %v, want the two callees %v", aside, want) + } + if n, a := affectedNeighbours(&contextPack{Intent: contextIntentUnderstand, Related: p.Related}); n != nil || a != nil { + t.Errorf("understanding code changes nothing, yet neighbours were walked: %v %v", n, a) + } +} diff --git a/internal/tools/context_affected_named_wiring_test.go b/internal/tools/context_affected_named_wiring_test.go new file mode 100644 index 00000000..2e4841d9 --- /dev/null +++ b/internal/tools/context_affected_named_wiring_test.go @@ -0,0 +1,80 @@ +package tools + +import ( + "os" + "path/filepath" + "slices" + "testing" +) + +// context_affected_named_wiring_test.go — the named tests' order end to end, over +// a real index, so each piece of the gather that decides it is pinned where it is +// wired: the walk from a neighbour starting one hop out, the aside mark for a +// neighbour the seed only calls, and which neighbours count as callers. + +// namedFixture is one package around Seed. Callers of Seed: B, X. Its callees: +// helperA, helperB (aside: their tests never run Seed). Each name is chosen so +// that a wrong hop count or a wrong kind reorders the line. +// +// TestDirect -> Seed 1 hop +// TestATwo -> X -> Seed 2 hops; the walk from the caller X would say 1 +// if a neighbour did not start one hop out +// TestADeep -> A -> B -> Seed 3 hops: past the walk's depth from Seed, +// reached only through the caller B +// TestBBoth -> A -> B -> Seed, and -> helperA: runs Seed, 3 hops +// TestACallees -> f -> helperA (3), and -> helperB (2): aside, and the shorter +// path is found second (helperA walks first) +// TestBHelper -> helperA aside, 2 hops +const namedFixtureSeed = `package seed + +func Seed() int { return helperA() + helperB() } + +func helperA() int { return 1 } + +func helperB() int { return 1 } + +func f() int { return helperA() } + +func B() int { return Seed() } + +func A() int { return B() } + +func X() int { return Seed() } +` + +const namedFixtureTests = `package seed + +import "testing" + +func TestDirect(t *testing.T) { Seed() } +func TestATwo(t *testing.T) { X() } +func TestADeep(t *testing.T) { A() } +func TestBBoth(t *testing.T) { A(); helperA() } +func TestACallees(t *testing.T) { f(); helperB() } +func TestBHelper(t *testing.T) { helperA() } +` + +func TestContextForTask_Affected_NamedTestsRunTheSeedFirstAndClosestFirst(t *testing.T) { + root := t.TempDir() + for name, body := range map[string]string{ + "go.mod": "module example.com/fixture\n\ngo 1.22\n", + "seed/seed.go": namedFixtureSeed, + "seed/seed_test.go": namedFixtureTests, + } { + p := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + s := newShopTool(t, openContextStore(t, root, 2), root) + pack := s.collect(t, map[string]any{"symbols": []string{"seed/seed.go#Seed"}, "intent": contextIntentChange, "max_bytes": 32000}) + got := pack.Affected.Named["seed"] + want := []string{"TestDirect", "TestATwo", "TestADeep", "TestBBoth", "TestACallees", "TestBHelper"} + if !slices.Equal(got.Names, want) { + t.Errorf("named = %q, want %q: tests that run Seed first and closest first, then the tests of Seed's "+ + "callees, closest first (see namedFixture)", got.Names, want) + } +} diff --git a/internal/tools/context_affected_test.go b/internal/tools/context_affected_test.go index 14bbcffd..bab0b7a4 100644 --- a/internal/tools/context_affected_test.go +++ b/internal/tools/context_affected_test.go @@ -14,9 +14,16 @@ import ( // measured against the gold's test_packages and test_package_counts. // sectionRows are the two-space rows under heading in a rendered pack, up to the -// next line that is not one. +// next line that is neither a row nor a deeper-indented sub-row of one (the +// affected section's named tests sit under their package row). func sectionRows(out, heading string) []string { - var rows []string + rows, _ := sectionRowsAndSubRows(out, heading) + return rows +} + +// sectionRowsAndSubRows also returns each row's sub-rows, keyed by the row. +func sectionRowsAndSubRows(out, heading string) (rows []string, sub map[string][]string) { + sub = map[string][]string{} in := false for _, l := range strings.Split(out, "\n") { switch { @@ -24,11 +31,13 @@ func sectionRows(out, heading string) []string { in = true case in && strings.HasPrefix(l, " ") && !strings.HasPrefix(l, " "): rows = append(rows, l) + case in && len(rows) > 0 && strings.HasPrefix(l, " "): + sub[rows[len(rows)-1]] = append(sub[rows[len(rows)-1]], l) case in: - return rows + return rows, sub } } - return rows + return rows, sub } // goldCounts reads a gold key that maps names to counts. @@ -59,14 +68,16 @@ func affectedTargets(rows []string) []string { // executionWords would claim a test was run. Nothing in the estimate may say one. var executionWords = regexp.MustCompile(`(?i)\b(passed|passes|failed|fails|green|ran|results?|succeeded)\b`) -// C07: a broad test package is one row with its count, never its individual tests, -// and nothing in the estimate says a test was run (Invariant 10). +// C07: a broad test package is one row with its count, never a list of its tests, +// and nothing in the estimate says a test was run (Invariant 10). Under the row, +// one line names the closest of the tests a call edge reaches, as a ready run_task +// filter, and counts the rest (PLAN-543): 60 tests call Summarise, so 8 are named. func TestContextForTask_Affected_C07_OneRowWithTheCountAndNoTestNames(t *testing.T) { s := newShop(t) c := oracle(t, "C07") _, out := runCase(t, s, c) - rows := sectionRows(out, affectedHeading) + rows, sub := sectionRowsAndSubRows(out, affectedHeading) counts := c.goldCounts(t, "test_package_counts") want := []string{fmt.Sprintf(" ./reports/... — %d test(s); holds a seed", counts["./reports/..."])} if !slices.Equal(rows, want) { @@ -93,7 +104,16 @@ func TestContextForTask_Affected_C07_OneRowWithTheCountAndNoTestNames(t *testing if !strings.Contains(out, cut) { t.Errorf("the tests beyond the cap are not counted (want %q):\n%s", cut, out) } - for _, line := range append([]string{affectedHeading}, rows...) { + names := make([]string, 0, contextNamedPerPackage) + for i := range contextNamedPerPackage { + names = append(names, fmt.Sprintf("TestSummariseCase%02d", i)) + } + wantSub := []string{fmt.Sprintf(` run first: run_task(slot:"test", target:"./reports/...", run:%q) — %d of %d test(s) that %s; %d more are not named`, + runFilterFor(names), contextNamedPerPackage, counts["./reports/..."], namedTestsReach, counts["./reports/..."]-contextNamedPerPackage)} + if !slices.Equal(sub[want[0]], wantSub) { + t.Errorf("named tests under the row = %q,\nwant %q", sub[want[0]], wantSub) + } + for _, line := range append(append([]string{affectedHeading}, rows...), sub[want[0]]...) { if m := executionWords.FindString(line); m != "" { t.Errorf("the estimate implies a test was run (%q): %q", m, line) } diff --git a/internal/tools/topology_affected.go b/internal/tools/topology_affected.go index 827ac112..8283f2a8 100644 --- a/internal/tools/topology_affected.go +++ b/internal/tools/topology_affected.go @@ -109,6 +109,13 @@ type affectedTest struct { Node topology.Node Confidence float64 Reason string // "dependency edge", "changed package", "imports the changed package" + // Hops is the fewest edges from the change to this test in the dependent walk, + // or 0 for a test found only by co-location. + Hops int + // Aside marks a test reached only through a neighbour the change does not run + // (a callee or sibling of a root): it is implicated, but it does not exercise + // the change itself. + Aside bool } // Reasons a directory is worth scanning for tests. They are ordered by how @@ -349,7 +356,16 @@ func declarationRoots(nodes []topology.Node) []topology.Node { } func collectAffected(ctx context.Context, store *topology.Store, roots []topology.Node, seedDirs []string, maxResults int) (*affectedResult, error) { - g := &affectedGather{store: store, maxResults: maxResults, seen: map[int64]bool{}, dirs: map[string]string{}} + return collectAffectedNear(ctx, store, roots, nil, nil, seedDirs, maxResults) +} + +// collectAffectedNear is collectAffected that also walks near: declarations one +// edge from a root, which a change to the roots may extend into. They are walked +// exactly as roots are, except that a test reached through one is a hop further +// from the change than the walk from that neighbour says, and a test reached only +// through a neighbour in aside is marked Aside. +func collectAffectedNear(ctx context.Context, store *topology.Store, roots, near []topology.Node, aside map[int64]bool, seedDirs []string, maxResults int) (*affectedResult, error) { + g := &affectedGather{store: store, maxResults: maxResults, seen: map[int64]bool{}, dirs: map[string]string{}, testAt: map[int64]int{}} for _, d := range seedDirs { g.dirs[d] = reasonChanged } @@ -363,7 +379,11 @@ func collectAffected(ctx context.Context, store *topology.Store, roots []topolog // bounded on packages in fromColocation. for _, root := range roots { g.dirs[filepath.Dir(root.Path)] = reasonChanged - g.fromGraph(ctx, root) + g.fromGraph(ctx, root, 0, false) + } + for _, n := range near { + g.dirs[filepath.Dir(n.Path)] = reasonChanged + g.fromGraph(ctx, n, 1, aside[n.ID]) } g.fromColocation(ctx) g.sortTests() @@ -381,6 +401,7 @@ type affectedGather struct { store *topology.Store maxResults int seen map[int64]bool + testAt map[int64]int // test node ID -> its index in tests, to keep the fewest hops dirs map[string]string // directory -> why it is implicated dependents []topology.Node tests []affectedTest @@ -416,7 +437,11 @@ func graphTraversalOpts(includeDerived bool) topology.ImpactOpts { // fromGraph adds inward (dependedOnBy) neighbours of root: tests are flagged // with their incident-edge confidence, other nodes become affected files and // seed more directories for the co-location pass. -func (g *affectedGather) fromGraph(ctx context.Context, root topology.Node) { +// +// away is how many hops root itself is from the change: 0 for a root, 1 for a +// neighbour of one; it is added to each test's hop count. aside marks the tests +// root reaches as not exercising the change (root is a callee or sibling of it). +func (g *affectedGather) fromGraph(ctx context.Context, root topology.Node, away int, aside bool) { // ImpactFrom, not Impact: root is ALREADY resolved, and Impact would throw it // away and re-resolve the bare name against the whole index. That lookup has no // tie-break, so a common name lands on an arbitrary row in an unrelated package @@ -437,8 +462,17 @@ func (g *affectedGather) fromGraph(ctx context.Context, root topology.Node) { g.graphTruncated = true } conf := incidentConfidence(nb.DependedOnBy.Edges) + hops := inwardHops(root.ID, nb.DependedOnBy.Edges) + for id, h := range hops { + hops[id] = h + away + } for _, n := range nb.DependedOnBy.Nodes { if g.seen[n.ID] { + // Another root reached this test first; keep the shorter path of the same + // kind (betterPath). + if i, ok := g.testAt[n.ID]; ok && hops[n.ID] > 0 && betterPath(g.tests[i], hops[n.ID], aside) { + g.tests[i].Hops = hops[n.ID] + } continue } g.seen[n.ID] = true @@ -455,7 +489,8 @@ func (g *affectedGather) fromGraph(ctx context.Context, root topology.Node) { if g.dirs[filepath.Dir(n.Path)] == reasonChanged { reason = reasonChanged } - g.tests = append(g.tests, affectedTest{Node: n, Confidence: c, Reason: reason}) + g.testAt[n.ID] = len(g.tests) + g.tests = append(g.tests, affectedTest{Node: n, Confidence: c, Reason: reason, Hops: hops[n.ID], Aside: aside}) } else { g.dependents = append(g.dependents, n) // Do not demote a directory that is already the changed one: a package diff --git a/internal/tools/topology_affected_paths.go b/internal/tools/topology_affected_paths.go new file mode 100644 index 00000000..b0a61b4e --- /dev/null +++ b/internal/tools/topology_affected_paths.go @@ -0,0 +1,42 @@ +package tools + +import "github.com/plumbkit/plumb/internal/topology" + +// How far a test is from the change in the dependent walk, and which of two +// paths to the same test to keep. context_for_task's named tests rank by these. + +// betterPath reports whether reaching t again in hops beats the path it was +// recorded with. Only a path of the same kind can: the walk visits roots, then +// callers of a seed, then the aside neighbours, so a path that runs the change +// is always recorded first, and an aside path never replaces one. Among paths of +// one kind, fewer hops beat more, and any count beats none (0). +func betterPath(t affectedTest, hops int, aside bool) bool { + return t.Aside == aside && (t.Hops == 0 || hops < t.Hops) +} + +// inwardHops is the fewest inward edges from root to each node of an inward +// neighbourhood. An edge FromID -> ToID means FromID depends on ToID, so the walk +// steps from a node to everything that depends on it. A node the edges do not +// connect to root is absent (0). +func inwardHops(root int64, edges []topology.Edge) map[int64]int { + dependents := map[int64][]int64{} + for _, e := range edges { + dependents[e.ToID] = append(dependents[e.ToID], e.FromID) + } + hops := map[int64]int{root: 0} + frontier := []int64{root} + for len(frontier) > 0 { + var next []int64 + for _, id := range frontier { + for _, d := range dependents[id] { + if _, ok := hops[d]; !ok { + hops[d] = hops[id] + 1 + next = append(next, d) + } + } + } + frontier = next + } + delete(hops, root) + return hops +}