From e0677783c27ce82b8056808f2543ac0242230f8f Mon Sep 17 00:00:00 2001 From: myzktkyk Date: Thu, 21 May 2026 23:23:26 +0900 Subject: [PATCH] Resolve black box test import path from go.mod The generated gobco_bridge_test.go must import the target package by its real Go import path. The previous implementation scanned imports in the target directory and copied one whose base name matched the target package's name. That heuristic failed when: - no '_test' file imported the target package directly (it was only reached through a helper package), and - another import in the same directory happened to share the same base name as the target package (for example, an interface package "iface/target" imported from "target"). In that case, the bridge file imported the wrong package and the test build failed with 'undefined: target.GobcoCover'. Compute the import path instead by combining the module path read directly from go.mod with the relative path from the module root, so the result no longer depends on which imports happen to appear in neighbouring files. The added testdata/issue33 reproduces the failure scenario described in issue #33. Fixes https://github.com/rillig/gobco/issues/33 --- instrumenter.go | 96 +++++++++++++++++++------- main.go | 26 +++++-- main_test.go | 20 ++++++ testdata/issue33/helper/helper.go | 10 +++ testdata/issue33/iface/target/iface.go | 8 +++ testdata/issue33/target/factory.go | 11 +++ testdata/issue33/target/target.go | 10 +++ testdata/issue33/target/target_test.go | 13 ++++ 8 files changed, 163 insertions(+), 31 deletions(-) create mode 100644 testdata/issue33/helper/helper.go create mode 100644 testdata/issue33/iface/target/iface.go create mode 100644 testdata/issue33/target/factory.go create mode 100644 testdata/issue33/target/target.go create mode 100644 testdata/issue33/target/target_test.go diff --git a/instrumenter.go b/instrumenter.go index 8fe71c9..9f2366e 100644 --- a/instrumenter.go +++ b/instrumenter.go @@ -11,6 +11,7 @@ import ( "go/token" "go/types" "os" + "path" "path/filepath" "reflect" "runtime" @@ -106,7 +107,7 @@ func (i *instrumenter) instrument(srcDir, singleFile, dstDir string) bool { i.instrumentFile(name, file, dstDir) }) } - i.writeGobcoFiles(dstDir, pkgs) + i.writeGobcoFiles(srcDir, dstDir, pkgs) return true } @@ -629,7 +630,7 @@ var fixedTemplate string //go:embed templates/gobco_no_testmain_test.go var noTestMainTemplate string -func (i *instrumenter) writeGobcoFiles(tmpDir string, pkgs []*ast.Package) { +func (i *instrumenter) writeGobcoFiles(srcDir, tmpDir string, pkgs []*ast.Package) { pkgname := pkgs[0].Name fixPkgname := func(str string) string { str = strings.TrimPrefix(str, "//go:build ignore\n// +build ignore\n\n") @@ -642,7 +643,7 @@ func (i *instrumenter) writeGobcoFiles(tmpDir string, pkgs []*ast.Package) { writeFile(filepath.Join(tmpDir, "gobco_no_testmain_test.go"), fixPkgname(noTestMainTemplate)) } - i.writeGobcoBlackBox(pkgs, tmpDir) + i.writeGobcoBlackBox(pkgs, srcDir, tmpDir) } func (i *instrumenter) writeGobcoGo(filename, pkgname string) { @@ -667,35 +668,80 @@ func (i *instrumenter) writeGobcoGo(filename, pkgname string) { writeFile(filename, sb.String()) } +// findPackagePath returns the canonical Go import path of the package +// located at srcDir, by combining the enclosing module path +// (read from go.mod) with the relative directory. +func findPackagePath(srcDir string) (string, error) { + moduleRoot, moduleRel, err := findInModule(srcDir) + if err != nil { + return "", err + } + if moduleRoot == "" { + return "", fmt.Errorf("no go.mod found for %q", srcDir) + } + + moduleName, err := readModuleName(moduleRoot) + if err != nil { + return "", err + } + + if moduleRel == "." { + return moduleName, nil + } + // Import paths always use '/' as separator, + // while moduleRel uses the OS separator. + return path.Join(moduleName, filepath.ToSlash(moduleRel)), nil +} + +// readModuleName parses the 'module' directive from go.mod in moduleRoot. +// +// Reading go.mod directly avoids depending on the 'go' command at instrumentation time +// and on any external module like golang.org/x/mod. +func readModuleName(moduleRoot string) (string, error) { + data, err := os.ReadFile(filepath.Join(moduleRoot, "go.mod")) + if err != nil { + return "", err + } + + for _, line := range strings.Split(string(data), "\n") { + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, "module") { + continue + } + rest := strings.TrimSpace(trimmed[len("module"):]) + if rest == "" { + continue + } + // The module path may optionally be quoted. + if rest[0] == '"' || rest[0] == '`' { + unquoted, err := strconv.Unquote(rest) + if err != nil { + return "", fmt.Errorf("parsing module directive: %w", err) + } + return unquoted, nil + } + // Strip any inline comment. + if idx := strings.Index(rest, "//"); idx >= 0 { + rest = strings.TrimSpace(rest[:idx]) + } + return rest, nil + } + + return "", fmt.Errorf("no module directive in %s", filepath.Join(moduleRoot, "go.mod")) +} + // writeGobcoBlackBox makes the function 'GobcoCover' available // to black box tests (those in 'package x_test' instead of 'package x') // by delegating to the function of the same name in the main package. -func (i *instrumenter) writeGobcoBlackBox(pkgs []*ast.Package, dstDir string) { +func (i *instrumenter) writeGobcoBlackBox(pkgs []*ast.Package, srcDir, dstDir string) { if len(pkgs) < 2 { return } - // Copy the 'import' directive from one of the existing files. - pkgName, pkgPath := "", "" - for _, pkg := range pkgs { - forEachFile(pkg, func(name string, file *ast.File) { - for _, imp := range file.Imports { - var impName string - p, err := strconv.Unquote(imp.Path.Value) - ok(err) - if imp.Name != nil { - impName = imp.Name.Name - } else { - impName = filepath.Base(p) - } - - if impName == pkgs[0].Name { - pkgName = impName - pkgPath = p - } - } - }) - } + pkgPath, err := findPackagePath(srcDir) + ok(err) + // Import paths use '/', so path.Base is correct on all platforms. + pkgName := path.Base(pkgPath) text := "" + "package " + pkgs[0].Name + "_test\n" + diff --git a/main.go b/main.go index afd9bff..3bf52d0 100644 --- a/main.go +++ b/main.go @@ -205,26 +205,40 @@ func (g *gobco) gopaths() string { } func (g *gobco) findInModule(dir string) (moduleRoot, moduleRel string) { - absDir, err := filepath.Abs(dir) + moduleRoot, moduleRel, err := findInModule(dir) g.check(err) + return moduleRoot, moduleRel +} + +// findInModule searches dir and its ancestors for a go.mod file. +// It returns the directory containing that file (moduleRoot) +// and the relative path from moduleRoot to dir (moduleRel). +// If no go.mod is found, both returned paths are empty. +func findInModule(dir string) (moduleRoot, moduleRel string, err error) { + absDir, err := filepath.Abs(dir) + if err != nil { + return "", "", err + } abs := absDir for { - if _, err := os.Lstat(filepath.Join(abs, "go.mod")); err == nil { - rel, err := filepath.Rel(abs, absDir) - g.check(err) + if _, statErr := os.Lstat(filepath.Join(abs, "go.mod")); statErr == nil { + rel, relErr := filepath.Rel(abs, absDir) + if relErr != nil { + return "", "", relErr + } root := abs if rel == "." { root = dir } - return root, rel + return root, rel, nil } parent := filepath.Dir(abs) if parent == abs { - return "", "" + return "", "", nil } abs = parent } diff --git a/main_test.go b/main_test.go index e1a1caf..9d9189e 100644 --- a/main_test.go +++ b/main_test.go @@ -579,3 +579,23 @@ func Test_gobcoMain__issue38(t *testing.T) { }) s.CheckEquals(stderr, "") } + +// Test_gobcoMain__issue33 covers the case where the target package's +// own directory contains an import of another package with the same +// base name (here, "iface/target"), and no '_test' file imports the +// target package directly. The previous implementation scanned imports +// in the directory and would pick the wrong import path, producing a +// gobco_bridge_test.go that failed to compile. +func Test_gobcoMain__issue33(t *testing.T) { + s := NewSuite(t) + defer s.TearDownTest() + + stdout, stderr := s.RunMain(0, "gobco", "./testdata/issue33/target") + + s.CheckEquals(s.GobcoLines(stdout), []string{ + "Condition coverage: 1/2", + "testdata/issue33/target/target.go:6:5: " + + "condition \"a > 0\" was once true but never false", + }) + s.CheckEquals(stderr, "") +} diff --git a/testdata/issue33/helper/helper.go b/testdata/issue33/helper/helper.go new file mode 100644 index 0000000..51215d3 --- /dev/null +++ b/testdata/issue33/helper/helper.go @@ -0,0 +1,10 @@ +// Package helper provides an indirection used by the issue #33 +// regression test, so that no file in the target package's directory +// directly imports the target package by its real import path. +package helper + +import "github.com/rillig/gobco/testdata/issue33/target" + +func AddViaTarget(a, b int) int { + return target.New().Add(a, b) +} diff --git a/testdata/issue33/iface/target/iface.go b/testdata/issue33/iface/target/iface.go new file mode 100644 index 0000000..6053de0 --- /dev/null +++ b/testdata/issue33/iface/target/iface.go @@ -0,0 +1,8 @@ +// Package target intentionally shares its name +// with github.com/rillig/gobco/testdata/issue33/target, +// to reproduce the import-resolution bug from issue #33. +package target + +type Adder interface { + Add(a, b int) int +} diff --git a/testdata/issue33/target/factory.go b/testdata/issue33/target/factory.go new file mode 100644 index 0000000..c467919 --- /dev/null +++ b/testdata/issue33/target/factory.go @@ -0,0 +1,11 @@ +package target + +// The unaliased import here has filepath.Base("iface/target") == "target", +// which equals the current package name. +// The old import-scanning logic in writeGobcoBlackBox mistook this +// for the target package's own import path. +import "github.com/rillig/gobco/testdata/issue33/iface/target" + +func New() target.Adder { + return &T{} +} diff --git a/testdata/issue33/target/target.go b/testdata/issue33/target/target.go new file mode 100644 index 0000000..a532cd5 --- /dev/null +++ b/testdata/issue33/target/target.go @@ -0,0 +1,10 @@ +package target + +type T struct{} + +func (t *T) Add(a, b int) int { + if a > 0 { + return a + b + } + return b +} diff --git a/testdata/issue33/target/target_test.go b/testdata/issue33/target/target_test.go new file mode 100644 index 0000000..89535ee --- /dev/null +++ b/testdata/issue33/target/target_test.go @@ -0,0 +1,13 @@ +package target_test + +import ( + "testing" + + "github.com/rillig/gobco/testdata/issue33/helper" +) + +func TestAdd(t *testing.T) { + if helper.AddViaTarget(1, 2) != 3 { + t.Fail() + } +}