From 3ef4a5836c9aa5b1034cf27c1fa67509b5a93764 Mon Sep 17 00:00:00 2001 From: Arieh Schneier <15041913+AriehSchneier@users.noreply.github.com> Date: Mon, 11 May 2026 13:42:17 +1000 Subject: [PATCH 1/5] Fix Go extractor to extract root internal test files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When CODEQL_EXTRACTOR_GO_OPTION_EXTRACT_TESTS=true is set, the Go extractor was incorrectly skipping internal test files (package foo) at repository roots when the project contains nested test packages. Root Cause: The extractor selected package variants by longest ID string, but this heuristic fails when nested packages have tests. For a package like "github.com/go-git/go-git/v6", packages.Load returns multiple variants: 1. "github.com/go-git/go-git/v6" (19 files, production only) 2. "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]" (39 files, production + 20 root tests) ← Should select this 3. "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/format/packfile.test]" (19 files, test dependency) ← Was incorrectly selected (longest string) The old logic selected variant #3 (76 chars) over #2 (68 chars), causing 20 root test files to be missing from the database. Fix: Replace string length comparison with a better heuristic that prefers: 1. Exact test packages (e.g., "pkg [pkg.test]") over nested dependencies 2. Packages with more Syntax nodes (more files to extract) 3. String length as a tiebreaker This ensures the extractor selects the variant with the most complete test coverage, particularly for root-level internal tests. Testing: - Added comprehensive unit tests covering the selection logic - Tests simulate the real-world go-git scenario - All tests pass Impact: Root-level external tests (package foo_test) were already extracted correctly. This fix ensures internal tests (package foo) at the root are now also extracted when they exist alongside nested test packages. Co-Authored-By: Claude Sonnet 4.5 --- go/extractor/extractor.go | 68 +++++++-- go/extractor/extractor_test.go | 255 +++++++++++++++++++++++++++++++++ 2 files changed, 309 insertions(+), 14 deletions(-) create mode 100644 go/extractor/extractor_test.go diff --git a/go/extractor/extractor.go b/go/extractor/extractor.go index bbcd32c10d24..c9ac7c6088a8 100644 --- a/go/extractor/extractor.go +++ b/go/extractor/extractor.go @@ -59,6 +59,44 @@ func init() { } } +// isExactTestPackage checks if a package ID represents an exact test match. +// Returns true for IDs like "github.com/foo/bar [github.com/foo/bar.test]" +// Returns false for IDs like "github.com/foo/bar [github.com/foo/bar/nested.test]" +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 +} + +// isBetterPackage determines if pkg is a better choice than current for extraction. +// Preferences: +// 1. Exact test package (e.g., "pkg [pkg.test]") over nested test dependencies +// 2. More Syntax nodes (more files to extract) +// 3. Longer ID string as tiebreaker +func isBetterPackage(pkg, current *packages.Package) bool { + pkgIsExact := isExactTestPackage(pkg) + currentIsExact := isExactTestPackage(current) + + // Prefer exact test packages + if pkgIsExact != currentIsExact { + return pkgIsExact + } + + // Prefer packages with more syntax nodes (more files) + pkgSyntaxCount := len(pkg.Syntax) + currentSyntaxCount := len(current.Syntax) + if pkgSyntaxCount != currentSyntaxCount { + return pkgSyntaxCount > currentSyntaxCount + } + + // Fall back to string length + return len(pkg.ID) > len(current.ID) +} + // 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() @@ -153,20 +191,22 @@ func ExtractWithFlags(buildFlags []string, patterns []string, extractTests bool, pkgsNotFound := make([]string, 0, len(pkgs)) - // Build a map from package paths to their longest IDs-- + // 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. - // For our purposes it is simplest to just ignore the non-test version, since the test - // version seems to be a superset of it. - longestPackageIds := make(map[string]string) + // 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 longestIDSoFar, present := longestPackageIds[pkg.PkgPath]; present { - if len(pkg.ID) > len(longestIDSoFar) { - longestPackageIds[pkg.PkgPath] = pkg.ID + if bestSoFar, present := bestPackageIds[pkg.PkgPath]; present { + if isBetterPackage(pkg, bestSoFar) { + bestPackageIds[pkg.PkgPath] = pkg } } else { - longestPackageIds[pkg.PkgPath] = pkg.ID + bestPackageIds[pkg.PkgPath] = pkg } }) @@ -257,15 +297,15 @@ func ExtractWithFlags(buildFlags []string, patterns []string, extractTests bool, // extract AST information for all packages packages.Visit(pkgs, nil, func(pkg *packages.Package) { - // If this is a variant of a package that also occurs with a longer ID, skip it; + // If this is a variant of a package that also occurs with a better ID, skip it; // otherwise we would extract the same file more than once including extracting the // body of methods twice, causing database inconsistencies. // - // We prefer the version with the longest ID because that is (so far as I know) always - // the version that defines more entities -- the only case I'm aware of being a test - // variant of a package, which includes test-only functions in addition to the complete - // contents of the main variant. - if pkg.ID != longestPackageIds[pkg.PkgPath] { + // We prefer the version with the most complete test coverage, prioritizing: + // 1. Exact test packages (e.g., "pkg [pkg.test]") over nested test dependencies + // 2. Packages with more Syntax nodes (more files to extract) + // 3. Longer ID strings as a tiebreaker + if pkg.ID != bestPackageIds[pkg.PkgPath].ID { return } diff --git a/go/extractor/extractor_test.go b/go/extractor/extractor_test.go new file mode 100644 index 000000000000..54f9dac1c128 --- /dev/null +++ b/go/extractor/extractor_test.go @@ -0,0 +1,255 @@ +package extractor + +import ( + "go/ast" + "testing" + + "golang.org/x/tools/go/packages" +) + +func TestIsExactTestPackage(t *testing.T) { + tests := []struct { + name string + pkgID string + pkgPath string + expected bool + }{ + { + name: "exact test package", + pkgID: "github.com/foo/bar [github.com/foo/bar.test]", + pkgPath: "github.com/foo/bar", + expected: true, + }, + { + name: "nested test package", + pkgID: "github.com/foo/bar [github.com/foo/bar/nested.test]", + pkgPath: "github.com/foo/bar", + expected: false, + }, + { + name: "deeply nested test package", + pkgID: "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/format/packfile.test]", + pkgPath: "github.com/go-git/go-git/v6", + expected: false, + }, + { + name: "exact test package with version", + pkgID: "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]", + pkgPath: "github.com/go-git/go-git/v6", + expected: true, + }, + { + name: "non-test package", + pkgID: "github.com/foo/bar", + pkgPath: "github.com/foo/bar", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pkg := &packages.Package{ + ID: tt.pkgID, + PkgPath: tt.pkgPath, + } + result := isExactTestPackage(pkg) + if result != tt.expected { + t.Errorf("isExactTestPackage(%q) = %v, want %v", tt.pkgID, result, tt.expected) + } + }) + } +} + +func TestIsBetterPackage(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 + pkg *packages.Package + current *packages.Package + expected bool // true if pkg is better than current + }{ + { + name: "exact test package beats nested test package", + pkg: makePkg( + "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]", + "github.com/go-git/go-git/v6", + 39, // 19 production + 20 test files + ), + current: makePkg( + "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/format/packfile.test]", + "github.com/go-git/go-git/v6", + 19, // production files only + ), + expected: true, + }, + { + name: "nested test package loses to exact test package", + pkg: makePkg( + "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/format/packfile.test]", + "github.com/go-git/go-git/v6", + 19, + ), + current: makePkg( + "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]", + "github.com/go-git/go-git/v6", + 39, + ), + expected: false, + }, + { + name: "more syntax nodes wins when both are exact tests", + pkg: makePkg( + "github.com/foo/bar [github.com/foo/bar.test]", + "github.com/foo/bar", + 50, + ), + current: makePkg( + "github.com/foo/bar [github.com/foo/bar.test]", + "github.com/foo/bar", + 30, + ), + expected: true, + }, + { + name: "fewer syntax nodes loses when both are exact tests", + pkg: makePkg( + "github.com/foo/bar [github.com/foo/bar.test]", + "github.com/foo/bar", + 30, + ), + current: makePkg( + "github.com/foo/bar [github.com/foo/bar.test]", + "github.com/foo/bar", + 50, + ), + expected: false, + }, + { + name: "more syntax nodes wins when both are nested tests", + pkg: makePkg( + "github.com/foo/bar [github.com/foo/bar/pkg1.test]", + "github.com/foo/bar", + 25, + ), + current: makePkg( + "github.com/foo/bar [github.com/foo/bar/pkg2.test]", + "github.com/foo/bar", + 20, + ), + expected: true, + }, + { + name: "longer ID wins when same syntax count", + pkg: makePkg( + "github.com/foo/bar [github.com/foo/bar/verylongpackagename.test]", + "github.com/foo/bar", + 20, + ), + current: makePkg( + "github.com/foo/bar [github.com/foo/bar/short.test]", + "github.com/foo/bar", + 20, + ), + expected: true, + }, + { + name: "test package beats non-test with same syntax count", + pkg: makePkg( + "github.com/foo/bar [github.com/foo/bar.test]", + "github.com/foo/bar", + 20, + ), + current: makePkg( + "github.com/foo/bar", + "github.com/foo/bar", + 20, + ), + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isBetterPackage(tt.pkg, tt.current) + if result != tt.expected { + t.Errorf("isBetterPackage() = %v, want %v\n pkg: %q (%d syntax nodes)\n current: %q (%d syntax nodes)", + result, tt.expected, + tt.pkg.ID, len(tt.pkg.Syntax), + tt.current.ID, len(tt.current.Syntax)) + } + }) + } +} + +// TestPackageSelectionRealWorld simulates the real-world go-git scenario +func TestPackageSelectionRealWorld(t *testing.T) { + // Simulate the actual packages.Load result for go-git repository + // when EXTRACT_TESTS=true + pkgs := []*packages.Package{ + // Production package only + { + ID: "github.com/go-git/go-git/v6", + PkgPath: "github.com/go-git/go-git/v6", + Syntax: make([]*ast.File, 19), // 19 production files + }, + // Root test package - this is what we want! + { + ID: "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]", + PkgPath: "github.com/go-git/go-git/v6", + Syntax: make([]*ast.File, 39), // 19 production + 20 test files + }, + // Nested test dependency 1 + { + ID: "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/format/packfile.test]", + PkgPath: "github.com/go-git/go-git/v6", + Syntax: make([]*ast.File, 19), // production files only (dependency) + }, + // Nested test dependency 2 + { + ID: "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6/plumbing/object.test]", + PkgPath: "github.com/go-git/go-git/v6", + Syntax: make([]*ast.File, 19), // production files only (dependency) + }, + } + + // 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 + } + } + + // Verify the correct package was selected + selected := bestPackageIds["github.com/go-git/go-git/v6"] + expectedID := "github.com/go-git/go-git/v6 [github.com/go-git/go-git/v6.test]" + expectedSyntaxCount := 39 + + if selected.ID != expectedID { + t.Errorf("Wrong package selected!\n got: %q (%d syntax nodes)\n want: %q (%d syntax nodes)", + selected.ID, len(selected.Syntax), + expectedID, expectedSyntaxCount) + } + + if len(selected.Syntax) != expectedSyntaxCount { + t.Errorf("Wrong syntax count: got %d, want %d", len(selected.Syntax), expectedSyntaxCount) + } + + // Verify it's recognized as an exact test package + if !isExactTestPackage(selected) { + t.Errorf("Selected package %q should be recognized as exact test package", selected.ID) + } +} From b94ab8d186bbb5b6bbc19827fb517757fa95ae46 Mon Sep 17 00:00:00 2001 From: Arieh Schneier <15041913+AriehSchneier@users.noreply.github.com> Date: Mon, 11 May 2026 15:10:54 +1000 Subject: [PATCH 2/5] Add integration test for root internal test extraction This test verifies that root internal test files (package foo, not foo_test) are correctly extracted when the repository has both: 1. Root-level internal tests (main_test.go with package main) 2. Nested packages with tests (nested/nested_test.go) This scenario reproduces the bug that was fixed: the old extractor would select the wrong package variant and miss root internal test files. The test ensures: - main_test.go (root internal test) is extracted - nested/nested_test.go (nested test) is extracted - All test functions from both files are present in the database This prevents regression of the bug fix. Co-Authored-By: Claude Sonnet 4.5 --- .../root-internal-tests/src/go.mod | 3 +++ .../root-internal-tests/src/main.go | 13 +++++++++++++ .../root-internal-tests/src/main_test.go | 16 ++++++++++++++++ .../root-internal-tests/src/nested/nested.go | 5 +++++ .../src/nested/nested_test.go | 9 +++++++++ .../root-internal-tests/test.expected | 7 +++++++ .../root-internal-tests/test.py | 5 +++++ .../root-internal-tests/test.ql | 15 +++++++++++++++ 8 files changed, 73 insertions(+) create mode 100644 go/ql/integration-tests/root-internal-tests/src/go.mod create mode 100644 go/ql/integration-tests/root-internal-tests/src/main.go create mode 100644 go/ql/integration-tests/root-internal-tests/src/main_test.go create mode 100644 go/ql/integration-tests/root-internal-tests/src/nested/nested.go create mode 100644 go/ql/integration-tests/root-internal-tests/src/nested/nested_test.go create mode 100644 go/ql/integration-tests/root-internal-tests/test.expected create mode 100644 go/ql/integration-tests/root-internal-tests/test.py create mode 100644 go/ql/integration-tests/root-internal-tests/test.ql diff --git a/go/ql/integration-tests/root-internal-tests/src/go.mod b/go/ql/integration-tests/root-internal-tests/src/go.mod new file mode 100644 index 000000000000..12e11856e552 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/src/go.mod @@ -0,0 +1,3 @@ +module example.com/testpkg + +go 1.26 diff --git a/go/ql/integration-tests/root-internal-tests/src/main.go b/go/ql/integration-tests/root-internal-tests/src/main.go new file mode 100644 index 000000000000..fff083caa0ac --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/src/main.go @@ -0,0 +1,13 @@ +package main + +func PublicFunc() int { + return 42 +} + +func privateFunc() int { + return 24 +} + +func main() { + PublicFunc() +} diff --git a/go/ql/integration-tests/root-internal-tests/src/main_test.go b/go/ql/integration-tests/root-internal-tests/src/main_test.go new file mode 100644 index 000000000000..7c38d61d4c87 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/src/main_test.go @@ -0,0 +1,16 @@ +package main + +import "testing" + +// Root internal test - tests private functions +func TestPrivateFunc(t *testing.T) { + if privateFunc() != 24 { + t.Error("privateFunc failed") + } +} + +func TestPublicFunc(t *testing.T) { + if PublicFunc() != 42 { + t.Error("PublicFunc failed") + } +} diff --git a/go/ql/integration-tests/root-internal-tests/src/nested/nested.go b/go/ql/integration-tests/root-internal-tests/src/nested/nested.go new file mode 100644 index 000000000000..427af1e44b63 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/src/nested/nested.go @@ -0,0 +1,5 @@ +package nested + +func NestedFunc() string { + return "nested" +} diff --git a/go/ql/integration-tests/root-internal-tests/src/nested/nested_test.go b/go/ql/integration-tests/root-internal-tests/src/nested/nested_test.go new file mode 100644 index 000000000000..a7e063c61859 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/src/nested/nested_test.go @@ -0,0 +1,9 @@ +package nested + +import "testing" + +func TestNestedFunc(t *testing.T) { + if NestedFunc() != "nested" { + t.Error("NestedFunc failed") + } +} diff --git a/go/ql/integration-tests/root-internal-tests/test.expected b/go/ql/integration-tests/root-internal-tests/test.expected new file mode 100644 index 000000000000..9f3d7b762c72 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/test.expected @@ -0,0 +1,7 @@ +#select +| src/main_test.go:0:0:0:0 | src/main_test.go | +| src/nested/nested_test.go:0:0:0:0 | src/nested/nested_test.go | +testFunctions +| TestNestedFunc | src/nested/nested_test.go | +| TestPrivateFunc | src/main_test.go | +| TestPublicFunc | src/main_test.go | diff --git a/go/ql/integration-tests/root-internal-tests/test.py b/go/ql/integration-tests/root-internal-tests/test.py new file mode 100644 index 000000000000..a8f376e33975 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/test.py @@ -0,0 +1,5 @@ +import os + +def test(codeql, go): + # Test that root internal test files are extracted when nested packages have tests + codeql.database.create(source_root="src", extractor_option = ["extract_tests=true"]) diff --git a/go/ql/integration-tests/root-internal-tests/test.ql b/go/ql/integration-tests/root-internal-tests/test.ql new file mode 100644 index 000000000000..234fd1a04203 --- /dev/null +++ b/go/ql/integration-tests/root-internal-tests/test.ql @@ -0,0 +1,15 @@ +import go + +// Verify that root internal test files are extracted +// when nested packages also have tests +from File f +where f.getBaseName().matches("%_test.go") +select f.getRelativePath() + +query predicate testFunctions(string name, string file) { + exists(FuncDecl fn | + fn.getName().matches("Test%") and + name = fn.getName() and + file = fn.getFile().getRelativePath() + ) +} From 151a332f0a0de581688ed6ae811d0b5639470875 Mon Sep 17 00:00:00 2001 From: Arieh Schneier <15041913+AriehSchneier@users.noreply.github.com> Date: Mon, 11 May 2026 20:55:11 +1000 Subject: [PATCH 3/5] Add Bazel build target for extractor_test.go Generated by manually applying the output from CI's Gazelle check. This adds the go_test target for the new extractor_test.go file. Co-Authored-By: Claude Sonnet 4.5 --- go/extractor/BUILD.bazel | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/go/extractor/BUILD.bazel b/go/extractor/BUILD.bazel index fbc53f20720b..23158e25b15f 100644 --- a/go/extractor/BUILD.bazel +++ b/go/extractor/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") load("@rules_java//java:defs.bzl", "java_library") load("@rules_pkg//pkg:mappings.bzl", "pkg_files") @@ -60,3 +60,10 @@ pkg_files( }, visibility = ["//go:__pkg__"], ) + +go_test( + name = "extractor_test", + srcs = ["extractor_test.go"], + embed = [":extractor"], + deps = ["@org_golang_x_tools//go/packages"], +) 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 4/5] 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 c9ac7c6088a8..158f0029704d 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 54f9dac1c128..2b585ec7fa1f 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"] From 0aaa7d0631943d77e862f47fadf0c4e7ccd05d1a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Mon, 11 May 2026 16:15:50 +0100 Subject: [PATCH 5/5] Update expected test output --- .../root-internal-tests/test.expected | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/go/ql/integration-tests/root-internal-tests/test.expected b/go/ql/integration-tests/root-internal-tests/test.expected index 9f3d7b762c72..f68c14d1338b 100644 --- a/go/ql/integration-tests/root-internal-tests/test.expected +++ b/go/ql/integration-tests/root-internal-tests/test.expected @@ -1,7 +1,7 @@ #select -| src/main_test.go:0:0:0:0 | src/main_test.go | -| src/nested/nested_test.go:0:0:0:0 | src/nested/nested_test.go | +| main_test.go | +| nested/nested_test.go | testFunctions -| TestNestedFunc | src/nested/nested_test.go | -| TestPrivateFunc | src/main_test.go | -| TestPublicFunc | src/main_test.go | +| TestNestedFunc | nested/nested_test.go | +| TestPrivateFunc | main_test.go | +| TestPublicFunc | main_test.go |