diff --git a/cmd/gta/main.go b/cmd/gta/main.go index 5f52e0a0..d37b4ba2 100644 --- a/cmd/gta/main.go +++ b/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 { diff --git a/gta.go b/gta.go index d27e830c..60f69c4e 100644 --- a/gta.go +++ b/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 := >A{ - 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...). diff --git a/gta_test.go b/gta_test.go index e814b6d1..5ffc25c6 100644 --- a/gta_test.go +++ b/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) { diff --git a/options.go b/options.go index 3afaaea7..6e20bca8 100644 --- a/options.go +++ b/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 + } +} diff --git a/packager.go b/packager.go index 31608583..9ddb325a 100644 --- a/packager.go +++ b/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,11 +355,32 @@ 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{}{} } } @@ -322,7 +388,7 @@ func dependencyGraph(cfg *packages.Config, patterns []string) (moduleNamesByDir 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 diff --git a/testdata/gtatest/testmock/testmock.go b/testdata/gtatest/testmock/testmock.go new file mode 100644 index 00000000..56c2c561 --- /dev/null +++ b/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" +} diff --git a/testdata/gtatest/usesmock/usesmock.go b/testdata/gtatest/usesmock/usesmock.go new file mode 100644 index 00000000..2ff51cdd --- /dev/null +++ b/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" +} diff --git a/testdata/gtatest/usesmock/usesmock_test.go b/testdata/gtatest/usesmock/usesmock_test.go new file mode 100644 index 00000000..6ac9169c --- /dev/null +++ b/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") + } +} diff --git a/testdata/gtatest/usesmockclient/usesmockclient.go b/testdata/gtatest/usesmockclient/usesmockclient.go new file mode 100644 index 00000000..6003887b --- /dev/null +++ b/testdata/gtatest/usesmockclient/usesmockclient.go @@ -0,0 +1,20 @@ +// Package usesmockclient imports usesmock for production use +// It does not import testmock at all (neither in production nor tests) +package usesmockclient + +import "gta.test/usesmock" + +// Client uses the real service. +type Client struct { + svc *usesmock.Service +} + +// New creates a new Client. +func New() *Client { + return &Client{svc: &usesmock.Service{}} +} + +// Run uses the service. +func (c *Client) Run() string { + return c.svc.DoSomething() +}