From aa1d322fe7e908281bd36c4c8dc504df3bd2439c Mon Sep 17 00:00:00 2001 From: Arieh Schneier <15041913+AriehSchneier@users.noreply.github.com> Date: Mon, 11 May 2026 21:05:26 +1000 Subject: [PATCH] Address PR feedback Changes based on code review: 1. Remove redundant strings.Contains check in isExactTestPackage The equality check on the next line handles both cases, making the early return unnecessary. 2. Extract package selection logic into selectBestPackages function This reduces code duplication and allows the test to call the actual implementation rather than copying the logic. 3. Add TestSelectBestPackages to test the new function Comprehensive test covering single packages, test vs production, exact vs nested tests, and multiple packages. Co-Authored-By: Claude Sonnet 4.5 --- go/extractor/extractor.go | 45 +++++++------- go/extractor/extractor_test.go | 110 +++++++++++++++++++++++++++++---- 2 files changed, 123 insertions(+), 32 deletions(-) diff --git a/go/extractor/extractor.go b/go/extractor/extractor.go index c9ac7c6088a..158f0029704 100644 --- a/go/extractor/extractor.go +++ b/go/extractor/extractor.go @@ -65,9 +65,6 @@ func init() { func isExactTestPackage(pkg *packages.Package) bool { // Test packages have IDs in the format: "pkgpath [pkgpath.test]" // or for nested test dependencies: "pkgpath [pkgpath/nested.test]" - if !strings.Contains(pkg.ID, " [") { - return false - } expectedTestID := pkg.PkgPath + " [" + pkg.PkgPath + ".test]" return pkg.ID == expectedTestID } @@ -97,6 +94,28 @@ func isBetterPackage(pkg, current *packages.Package) bool { return len(pkg.ID) > len(current.ID) } +// selectBestPackages builds a map from package paths to their best package variants. +// In the context of a `go test -c` compilation, we see the same package more than +// once, with IDs like "abc.com/pkgname [abc.com/pkgname.test]" to distinguish the version +// that contains and is used by test code. +// We prefer the version with the most complete test coverage, which is typically: +// 1. The exact test package (e.g., "pkg [pkg.test]") over nested test dependencies +// 2. The package with the most Syntax nodes (most files to extract) +// 3. The longest ID string as a tiebreaker +func selectBestPackages(pkgs []*packages.Package) map[string]*packages.Package { + bestPackageIds := make(map[string]*packages.Package) + packages.Visit(pkgs, nil, func(pkg *packages.Package) { + if bestSoFar, present := bestPackageIds[pkg.PkgPath]; present { + if isBetterPackage(pkg, bestSoFar) { + bestPackageIds[pkg.PkgPath] = pkg + } + } else { + bestPackageIds[pkg.PkgPath] = pkg + } + }) + return bestPackageIds +} + // ExtractWithFlags extracts the packages specified by the given patterns and build flags func ExtractWithFlags(buildFlags []string, patterns []string, extractTests bool, sourceRoot string) error { startTime := time.Now() @@ -191,24 +210,8 @@ func ExtractWithFlags(buildFlags []string, patterns []string, extractTests bool, pkgsNotFound := make([]string, 0, len(pkgs)) - // Build a map from package paths to their best IDs-- - // in the context of a `go test -c` compilation, we will see the same package more than - // once, with IDs like "abc.com/pkgname [abc.com/pkgname.test]" to distinguish the version - // that contains and is used by test code. - // We prefer the version with the most complete test coverage, which is typically: - // 1. The exact test package (e.g., "pkg [pkg.test]") over nested test dependencies - // 2. The package with the most Syntax nodes (most files to extract) - // 3. The longest ID string as a tiebreaker - bestPackageIds := make(map[string]*packages.Package) - packages.Visit(pkgs, nil, func(pkg *packages.Package) { - if bestSoFar, present := bestPackageIds[pkg.PkgPath]; present { - if isBetterPackage(pkg, bestSoFar) { - bestPackageIds[pkg.PkgPath] = pkg - } - } else { - bestPackageIds[pkg.PkgPath] = pkg - } - }) + // Build a map from package paths to their best IDs + bestPackageIds := selectBestPackages(pkgs) // Do a post-order traversal and extract the package scope of each package packages.Visit(pkgs, nil, func(pkg *packages.Package) { diff --git a/go/extractor/extractor_test.go b/go/extractor/extractor_test.go index 54f9dac1c12..2b585ec7fa1 100644 --- a/go/extractor/extractor_test.go +++ b/go/extractor/extractor_test.go @@ -190,6 +190,103 @@ func TestIsBetterPackage(t *testing.T) { } } +// TestSelectBestPackages tests the selectBestPackages function +func TestSelectBestPackages(t *testing.T) { + // Helper to create a package with specified properties + makePkg := func(id, path string, syntaxCount int) *packages.Package { + syntax := make([]*ast.File, syntaxCount) + return &packages.Package{ + ID: id, + PkgPath: path, + Syntax: syntax, + } + } + + tests := []struct { + name string + pkgs []*packages.Package + expectedPkgIDs map[string]string // pkgPath -> expected selected ID + }{ + { + name: "single package", + pkgs: []*packages.Package{ + makePkg("example.com/pkg", "example.com/pkg", 10), + }, + expectedPkgIDs: map[string]string{ + "example.com/pkg": "example.com/pkg", + }, + }, + { + name: "test package preferred over production", + pkgs: []*packages.Package{ + makePkg("example.com/pkg", "example.com/pkg", 10), + makePkg("example.com/pkg [example.com/pkg.test]", "example.com/pkg", 15), + }, + expectedPkgIDs: map[string]string{ + "example.com/pkg": "example.com/pkg [example.com/pkg.test]", + }, + }, + { + name: "exact test preferred over nested test", + pkgs: []*packages.Package{ + makePkg("example.com/pkg [example.com/pkg.test]", "example.com/pkg", 20), + makePkg("example.com/pkg [example.com/pkg/nested.test]", "example.com/pkg", 15), + }, + expectedPkgIDs: map[string]string{ + "example.com/pkg": "example.com/pkg [example.com/pkg.test]", + }, + }, + { + name: "multiple packages with different paths", + pkgs: []*packages.Package{ + makePkg("example.com/pkg1", "example.com/pkg1", 10), + makePkg("example.com/pkg1 [example.com/pkg1.test]", "example.com/pkg1", 15), + makePkg("example.com/pkg2", "example.com/pkg2", 8), + makePkg("example.com/pkg2 [example.com/pkg2.test]", "example.com/pkg2", 12), + }, + expectedPkgIDs: map[string]string{ + "example.com/pkg1": "example.com/pkg1 [example.com/pkg1.test]", + "example.com/pkg2": "example.com/pkg2 [example.com/pkg2.test]", + }, + }, + { + name: "more syntax nodes wins among nested tests", + pkgs: []*packages.Package{ + makePkg("example.com/pkg [example.com/pkg/a.test]", "example.com/pkg", 10), + makePkg("example.com/pkg [example.com/pkg/b.test]", "example.com/pkg", 20), + }, + expectedPkgIDs: map[string]string{ + "example.com/pkg": "example.com/pkg [example.com/pkg/b.test]", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := selectBestPackages(tt.pkgs) + + // Check that all expected packages are present + for pkgPath, expectedID := range tt.expectedPkgIDs { + selected, found := result[pkgPath] + if !found { + t.Errorf("Expected package path %q not found in result", pkgPath) + continue + } + if selected.ID != expectedID { + t.Errorf("For package path %q: got ID %q, want %q", + pkgPath, selected.ID, expectedID) + } + } + + // Check that no unexpected packages are present + if len(result) != len(tt.expectedPkgIDs) { + t.Errorf("Expected %d packages in result, got %d", + len(tt.expectedPkgIDs), len(result)) + } + }) + } +} + // TestPackageSelectionRealWorld simulates the real-world go-git scenario func TestPackageSelectionRealWorld(t *testing.T) { // Simulate the actual packages.Load result for go-git repository @@ -221,17 +318,8 @@ func TestPackageSelectionRealWorld(t *testing.T) { }, } - // Simulate the bestPackageIds selection logic - bestPackageIds := make(map[string]*packages.Package) - for _, pkg := range pkgs { - if bestSoFar, present := bestPackageIds[pkg.PkgPath]; present { - if isBetterPackage(pkg, bestSoFar) { - bestPackageIds[pkg.PkgPath] = pkg - } - } else { - bestPackageIds[pkg.PkgPath] = pkg - } - } + // Use the actual selection logic from the extractor + bestPackageIds := selectBestPackages(pkgs) // Verify the correct package was selected selected := bestPackageIds["github.com/go-git/go-git/v6"]