Fix errant packages found via test dep. graph

cmd/gta/main.go

@@ -34,6 +34,7 @@ func main() {
 	flagBuildableOnly := flag.Bool("buildable-only", true, "keep buildable changed packages only")
 	flagChangedFiles := flag.String("changed-files", "", "path to a file containing a newline separated list of files that have changed")
 	flagTags := flag.String("tags", "", "a list of build tags to consider")
+	flagTestTransitive := flag.Bool("test-transitive", true, "legacy behavior; include transitive test dependencies in the reverse dependency graph traversal")
 
 	flag.Parse()
 
@@ -53,6 +54,7 @@ func main() {
 	options := []gta.Option{
 		gta.SetPrefixes(parseStringSlice(*flagInclude)...),
 		gta.SetTags(tags...),
+		gta.SetIncludeTransitiveTestDeps(*flagTestTransitive),
 	}
 
 	if len(*flagChangedFiles) == 0 {

gta.go

@@ -87,18 +87,20 @@ func (p *Packages) UnmarshalJSON(b []byte) error {
 // A GTA provides a method of building dirty packages, and their dependent
 // packages.
 type GTA struct {
-	differ   Differ
-	packager Packager
-	prefixes []string
-	tags     []string
-	roots    []string
+	differ                    Differ
+	packager                  Packager
+	prefixes                  []string
+	tags                      []string
+	roots                     []string
+	includeTransitiveTestDeps bool
 }
 
 // New returns a new GTA with various options passed to New. Options will be
 // applied in order so that later options can override earlier options.
 func New(opts ...Option) (*GTA, error) {
 	gta := &GTA{
-		differ: NewGitDiffer(),
+		differ:                    NewGitDiffer(),
+		includeTransitiveTestDeps: true,
 	}
 
 	for _, opt := range opts {
@@ -388,6 +390,12 @@ func (g *GTA) markedPackages() (map[string]map[string]bool, error) {
 		return nil, fmt.Errorf("building dependency graph, %v", err)
 	}
 
+	// we also get the test-only dependent graph
+	testOnlyGraph, err := g.packager.TestOnlyDependentGraph()
+	if err != nil {
+		return nil, fmt.Errorf("building test-only dependency graph, %v", err)
+	}
+
 	paths := map[string]map[string]bool{}
 	for change := range changed {
 		marked := make(map[string]bool)
@@ -398,8 +406,38 @@ func (g *GTA) markedPackages() (map[string]map[string]bool, error) {
 			continue
 		}
 
-		// we traverse the graph and build our list of mark all dependents
-		graph.Traverse(change, marked)
+		// Traverse dependents. When includeTransitiveTestDeps is true, test-only
+		// edges are traversed the same as production edges (replicating the old
+		// behavior where all imports were in a single reverse graph).
+		var traverse func(node string)
+		traverse = func(node string) {
+			if marked[node] {
+				return
+			}
+			marked[node] = true
+
+			if edges, ok := graph.graph[node]; ok {
+				for edge := range edges {
+					traverse(edge)
+				}
+			}
+			if g.includeTransitiveTestDeps {
+				if edges, ok := testOnlyGraph.graph[node]; ok {
+					for edge := range edges {
+						traverse(edge)
+					}
+				}
+			}
+		}
+		traverse(change)
+
+		// When not traversing transitive test deps, still mark direct test-only
+		// dependents (their tests need re-running, but we don't traverse further).
+		if !g.includeTransitiveTestDeps {
+			for testDep := range testOnlyGraph.graph[change] {
+				marked[testDep] = true
+			}
+		}
 
 		// clear the boolean value on the paths that no longer contain packages (i.e.
 		// the Go files were deleted...).

gta_test.go

@@ -79,6 +79,11 @@ func (t *testPackager) DependentGraph() (*Graph, error) {
 	return t.graph, nil
 }
 
+func (t *testPackager) TestOnlyDependentGraph() (*Graph, error) {
+	// For legacy tests, return an empty graph
+	return &Graph{graph: make(map[string]map[string]bool)}, nil
+}
+
 func (_ *testPackager) EmbeddedBy(_ string) []string {
 	return nil
 }
@@ -587,6 +592,15 @@ func TestGTA_ChangedPackages(t *testing.T) {
 
 		testChangedPackages(t, diff, nil, want)
 	})
+
+	// "bar_test" is a package with a name ending in "_test", not a test
+	// package, and ensuring the weird name is still found is the point of this
+	// test.
+	//
+	// fooclient imports bar_test only in its test file, making it a test-only
+	// dependent. With -test-transitive=true (default), we traverse test-only
+	// dependents and their production dependents, so fooclientclient is
+	// included.
 	t.Run("change badly named package", func(t *testing.T) {
 		diff := map[string]Directory{
 			"bar_test": {Exists: true, Files: []string{"util.go"}},
@@ -670,6 +684,44 @@ func TestGTA_ChangedPackages(t *testing.T) {
 
 		testChangedPackages(t, diff, nil, want)
 	})
+
+	// Test-only dependencies propagate through the reverse dependency graph
+	// when -test-transitive=true (the default).
+	//
+	// Test data setup:
+	//   - testmock: a mock package that is only used in tests
+	//   - usesmock: imports testmock only in its test file, usesmock_test.go
+	//   - usesmockclient: imports usesmock for production, does not import testmock
+	//
+	// When testmock changes with -test-transitive=true, usesmock is found affected
+	// because its tests use testmock, and usesmockclient is also found affected
+	// because it imports usesmock (and we traverse all dependents when
+	// -test-transitive=true).
+	//
+	t.Run("change test-only mock package affects production dependents with test=true", func(t *testing.T) {
+		diff := map[string]Directory{
+			"testmock": {Exists: true, Files: []string{"testmock.go"}},
+		}
+
+		want := &Packages{
+			Dependencies: map[string][]Package{
+				"testmock": {
+					{ImportPath: "usesmock", Dir: "usesmock"},
+					{ImportPath: "usesmockclient", Dir: "usesmockclient"},
+				},
+			},
+			Changes: []Package{
+				{ImportPath: "testmock", Dir: "testmock"},
+			},
+			AllChanges: []Package{
+				{ImportPath: "testmock", Dir: "testmock"},
+				{ImportPath: "usesmock", Dir: "usesmock"},
+				{ImportPath: "usesmockclient", Dir: "usesmockclient"},
+			},
+		}
+
+		testChangedPackages(t, diff, nil, want)
+	})
 }
 
 func TestGTA_Prefix(t *testing.T) {

options.go

@@ -40,3 +40,14 @@ func SetTags(tags ...string) Option {
 		return nil
 	}
 }
+
+// SetIncludeTransitiveTestDeps sets whether to include test dependencies in the
+// dependency graph traversal. When true (the default), packages that are only
+// imported by test code are included in the full dependency traversal. When
+// false, such test-only dependents are marked but not traversed further.
+func SetIncludeTransitiveTestDeps(include bool) Option {
+	return func(g *GTA) error {
+		g.includeTransitiveTestDeps = include
+		return nil
+	}
+}

packager.go

@@ -51,6 +51,11 @@ type Packager interface {
 	// DependentGraph returns the DependentGraph for the current
 	// Golang workspace as defined by their import paths.
 	DependentGraph() (*Graph, error)
+	// TestOnlyDependentGraph returns a graph of test-only dependencies.
+	// These are packages whose tests import a given package, but whose
+	// production code does not. When a package changes, its test-only
+	// dependents should be marked, but their production dependents should not.
+	TestOnlyDependentGraph() (*Graph, error)
 	// EmbeddedBy returns the package import paths of packages that embed a file.
 	EmbeddedBy(string) []string
 }
@@ -61,13 +66,14 @@ func NewPackager(patterns, tags []string) Packager {
 }
 
 func newPackager(cfg *packages.Config, ctx build.Context, patterns []string) Packager {
-	moduleNamesByDir, forward, reverse, packagesByEmbedFile, err := dependencyGraph(cfg, patterns)
+	moduleNamesByDir, forward, reverse, testOnlyReverse, packagesByEmbedFile, err := dependencyGraph(cfg, patterns)
 	return &packageContext{
 		ctx:                 &ctx,
 		err:                 err,
 		packages:            make(map[string]struct{}),
 		forward:             forward,
 		reverse:             reverse,
+		testOnlyReverse:     testOnlyReverse,
 		modulesNamesByDir:   moduleNamesByDir,
 		packagesByEmbedFile: packagesByEmbedFile,
 	}
@@ -82,7 +88,8 @@ func newLoadConfig(tags []string) *packages.Config {
 			packages.NeedEmbedFiles |
 			packages.NeedImports |
 			packages.NeedDeps |
-			packages.NeedModule,
+			packages.NeedModule |
+			packages.NeedForTest,
 		BuildFlags: []string{
 			fmt.Sprintf(`-tags=%s`, strings.Join(tags, ",")),
 		},
@@ -100,6 +107,9 @@ type packageContext struct {
 	forward map[string]map[string]struct{}
 	// reverse is a reverse dependency graph (import path -> (dependent import path -> struct{}{}))
 	reverse map[string]map[string]struct{}
+	// testOnlyReverse is a reverse dependency graph for test-only imports.
+	// These are imports that only appear in test files, not production code.
+	testOnlyReverse map[string]map[string]struct{}
 	// modulesNamesByDir is a map of directories to import paths. absolute path
 	// directory -> import path/module name
 	modulesNamesByDir map[string]string
@@ -175,6 +185,24 @@ func (p *packageContext) DependentGraph() (*Graph, error) {
 	return &Graph{graph: graph}, nil
 }
 
+// TestOnlyDependentGraph returns a graph of test-only dependencies.
+func (p *packageContext) TestOnlyDependentGraph() (*Graph, error) {
+	if p.err != nil {
+		return nil, p.err
+	}
+
+	graph := make(map[string]map[string]bool)
+	for k := range p.testOnlyReverse {
+		inner := make(map[string]bool)
+		for k2 := range p.testOnlyReverse[k] {
+			inner[k2] = true
+		}
+		graph[k] = inner
+	}
+
+	return &Graph{graph: graph}, nil
+}
+
 func packageFrom(pkg *build.Package) *Package {
 	return &Package{
 		ImportPath: pkg.ImportPath,
@@ -228,7 +256,7 @@ func resolveLocal(pkg *Package, dir string, modulesByDir map[string]string) {
 // module aware mode and flattened forward and reverse transitive dependency
 // graphs. When in GOPATH mode the map of directories to import paths will be
 // empty.
-func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir map[string]string, forward map[string]map[string]struct{}, reverse map[string]map[string]struct{}, packagesByEmbedFile map[string][]string, err error) {
+func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir map[string]string, forward map[string]map[string]struct{}, reverse map[string]map[string]struct{}, testOnlyReverse map[string]map[string]struct{}, packagesByEmbedFile map[string][]string, err error) {
 	loadAllPackages := true
 	for i, pat := range patterns {
 		if strings.HasPrefix(pat, "file=") {
@@ -250,14 +278,20 @@ func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir
 
 	loadedPackages, err := packages.Load(cfg, patterns...)
 	if err != nil {
-		return nil, nil, nil, nil, fmt.Errorf("loading packages: %w", err)
+		return nil, nil, nil, nil, nil, fmt.Errorf("loading packages: %w", err)
 	}
 
 	moduleNamesByDir = make(map[string]string)
 	forward = make(map[string]map[string]struct{})
 	reverse = make(map[string]map[string]struct{})
+	testOnlyReverse = make(map[string]map[string]struct{})
 	packagesByEmbedFile = make(map[string][]string)
 
+	// productionImports tracks the imports from the main (not-for-test) variant
+	// of each package. This is used to distinguish test-only imports from
+	// production imports when processing for-test package variants.
+	productionImports := make(map[string]map[string]struct{})
+
 	seen := make(map[string]struct{})
 	var addPackage func(pkg *packages.Package)
 	addPackage = func(pkg *packages.Package) {
@@ -286,6 +320,10 @@ func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir
 		// the package path of the primary package.
 		pkgPath := normalizeImportPath(pkg)
 
+		// Detect if this is a "for-test" variant. These variants include test
+		// imports that should be tracked separately from production imports.
+		forTest := pkg.ForTest != ""
+
 		for _, f := range pkg.EmbedFiles {
 			sl := packagesByEmbedFile[f]
 			packagesByEmbedFile[f] = append(sl, pkgPath)
@@ -295,6 +333,13 @@ func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir
 			forward[pkgPath] = make(map[string]struct{})
 		}
 
+		// For not-for-test packages, record their imports as production imports.
+		if !forTest {
+			if _, ok := productionImports[pkgPath]; !ok {
+				productionImports[pkgPath] = make(map[string]struct{})
+			}
+		}
+
 		for _, importedPkg := range pkg.Imports {
 			addPackage(importedPkg)
 
@@ -310,19 +355,40 @@ func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir
 				continue
 			}
 
-			if _, ok := reverse[importedPath]; !ok {
-				reverse[importedPath] = make(map[string]struct{})
+			// For not-for-test packages, record this as a production import and
+			// add to the main reverse graph.
+			if !forTest {
+				productionImports[pkgPath][importedPath] = struct{}{}
+
+				if _, ok := reverse[importedPath]; !ok {
+					reverse[importedPath] = make(map[string]struct{})
+				}
+				revm := reverse[importedPath]
+				revm[pkgPath] = struct{}{}
+			} else {
+				// For for-test variants, check if this import is test-only.
+				// Test-only imports go to testOnlyReverse, not the main reverse graph.
+				if prodImports, ok := productionImports[pkgPath]; ok {
+					if _, isProdImport := prodImports[importedPath]; !isProdImport {
+						// This is a test-only import; add to testOnlyReverse.
+						if _, ok := testOnlyReverse[importedPath]; !ok {
+							testOnlyReverse[importedPath] = make(map[string]struct{})
+						}
+						testOnlyReverse[importedPath][pkgPath] = struct{}{}
+						continue
+					}
+				}
+				// Production imports from for-test variants are already in reverse
+				// from when the main package was processed.
 			}
-			revm := reverse[importedPath]
-			revm[pkgPath] = struct{}{}
 		}
 	}
 
 	for _, pkg := range loadedPackages {
 		addPackage(pkg)
 	}
 
-	return moduleNamesByDir, forward, reverse, packagesByEmbedFile, nil
+	return moduleNamesByDir, forward, reverse, testOnlyReverse, packagesByEmbedFile, nil
 }
 
 // normalizeImportPath will return the import path of pkg. The import path may

testdata/gtatest/testmock/testmock.go

@@ -0,0 +1,10 @@
+// This package should only be imported by test files
+package testmock
+
+// MockService is a mock implementation for testing
+type MockService struct{}
+
+// DoSomething is a mock method
+func (m *MockService) DoSomething() string {
+	return "mocked"
+}

testdata/gtatest/usesmock/usesmock.go

@@ -0,0 +1,10 @@
+// Only the test file imports testmock
+package usesmock
+
+// Service provides real functionality
+type Service struct{}
+
+// DoSomething does real work
+func (s *Service) DoSomething() string {
+	return "real"
+}

testdata/gtatest/usesmock/usesmock_test.go

@@ -0,0 +1,17 @@
+package usesmock
+
+import (
+	"testing"
+
+	// This test file imports testmock - a test-only dependency: 
+	// the production code in usesmock.go notably does not import
+	// testmock
+	"gta.test/testmock"
+)
+
+func TestService(t *testing.T) {
+	mock := &testmock.MockService{}
+	if got := mock.DoSomething(); got != "mocked" {
+		t.Errorf("got %q, want %q", got, "mocked")
+	}
+}