Ensure we only ever import the test version of the packag efor covera…

…ge (#13) * Ensure we only ever import the test version of the packag efor coverage * fix test * Fix tests
please-build · Apr 6, 2022 · 4c1f27b · 4c1f27b
1 parent 9952601
commit 4c1f27b
Show file tree

Hide file tree

Showing 12 changed files with 37 additions and 21 deletions.
diff --git a/.github/workflows/plugin.yaml b/.github/workflows/plugin.yaml
@@ -25,7 +25,7 @@ jobs:
       - name: Checkout code
         uses: actions/checkout@v2
       - name: Build
-        run: ./pleasew build -p -v notice //package:release_files
+        run: ./pleasew build --profile release -p -v notice //package:release_files
       - name: Release
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

diff --git a/.plzconfig b/.plzconfig
@@ -60,4 +60,7 @@ Repeatable = true
 Optional = true
 
 [FeatureFlags]
-ExcludeGoRules = true
+ExcludeGoRules = true
+
+[BuildConfig]
+Release = False
diff --git a/.plzconfig.release b/.plzconfig.release
@@ -0,0 +1,2 @@
+[BuildConfig]
+Release = true
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,8 @@
+Version 0.2.3
+-------------
+    * Import the test version of internal packages when calculating
+      coverage
+
 Version 0.2.2
 -------------
     * Handle hyphens in source file names when generating cover vars

diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-0.2.2
+0.2.3
diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs
@@ -909,8 +909,8 @@ def go_test_main(name:str, srcs:list, test_package:str="", test_only:bool=False,
     """
     cover = cover and (CONFIG.BUILD_CONFIG == "cover")
     test_package = test_package or _get_import_path()
-
-    cmd = f'"$TOOLS_PLZ" testmain --test_package "{test_package}" -o $OUT'
+    external_flag = "--external" if external else ""
+    cmd = f'"$TOOLS_PLZ" testmain --test_package "{test_package}" {external_flag} -o $OUT'
     if benchmark:
         cmd = f'{cmd} --benchmark'
     cmds = {

diff --git a/package/BUILD b/package/BUILD
@@ -6,6 +6,7 @@ def cross_compile(arch):
         srcs = [f"///{arch}//tools/please_go"],
         outs = [f"please_go-{VERSION}-{arch}"],
         cmd = "mv $SRC $OUT",
+        labels = ["manual"],
     )
 
 filegroup(
@@ -17,5 +18,5 @@ filegroup(
         "linux_amd64",
         "linux_arm64",
     ]],
-    labels = ["hlink:plz-out/package"],
+    labels = ["hlink:plz-out/package", "manual"],
 )
diff --git a/third_party/go/BUILD b/third_party/go/BUILD
@@ -1,6 +1,5 @@
 subinclude("//build_defs:go")
 
-
 go_toolchain(
     name = "toolchain",
     architectures = [
@@ -9,7 +8,9 @@ go_toolchain(
         "freebsd_amd64",
         "linux_amd64",
         "linux_arm64",
-    ],
+    # Installing takes a while so only do it when we need it for release
+    # This extra check can be removed once we fix booleans in build config
+    ] if CONFIG.RELEASE and CONFIG.RELEASE != "False" else [],
     version = "1.18",
     tags = [
         "osusergo",

diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go
@@ -41,6 +41,7 @@ var opts = struct {
 		Output      string   `short:"o" long:"output" description:"Output filename" required:"true"`
 		TestPackage string   `short:"t" long:"test_package" description:"The import path of the test package"`
 		Benchmark   bool     `short:"b" long:"benchmark" description:"Whether to run benchmarks instead of tests"`
+		External    bool     `long:"external" description:"Whether the test is external or not"`
 		Args        struct {
 			Sources []string `positional-arg-name:"sources" description:"Test source files" required:"true"`
 		} `positional-args:"true" required:"true"`
@@ -92,7 +93,7 @@ var subCommands = map[string]func() int{
 		return 0
 	},
 	"testmain": func() int {
-		test.PleaseGoTest(opts.Test.GoTool, opts.Test.Dir, opts.Test.TestPackage, opts.Test.Output, opts.Test.Args.Sources, opts.Test.Exclude, opts.Test.Benchmark)
+		test.PleaseGoTest(opts.Test.GoTool, opts.Test.Dir, opts.Test.TestPackage, opts.Test.Output, opts.Test.Args.Sources, opts.Test.Exclude, opts.Test.Benchmark, opts.Test.External)
 		return 0
 	},
 	"covervars": func() int {

diff --git a/tools/please_go/test/find_cover_vars.go b/tools/please_go/test/find_cover_vars.go
@@ -16,7 +16,7 @@ type CoverVar struct {
 }
 
 // FindCoverVars searches the given directory recursively to find all Go files with coverage variables.
-func FindCoverVars(dir string, excludedDirs []string) ([]CoverVar, error) {
+func FindCoverVars(dir, testPackage string, external bool, excludedDirs []string) ([]CoverVar, error) {
 	if dir == "" {
 		return nil, nil
 	}
@@ -35,7 +35,7 @@ func FindCoverVars(dir string, excludedDirs []string) ([]CoverVar, error) {
 				return filepath.SkipDir
 			}
 		} else if strings.HasSuffix(name, ".cover_vars") {
-			vars, err := parseCoverVars(name)
+			vars, err := parseCoverVars(name, testPackage, external)
 			if err != nil {
 				return err
 			}
@@ -47,7 +47,7 @@ func FindCoverVars(dir string, excludedDirs []string) ([]CoverVar, error) {
 }
 
 // parseCoverVars parses the coverage variables file for all cover vars
-func parseCoverVars(filepath string) ([]CoverVar, error) {
+func parseCoverVars(filepath, testPackage string, external bool) ([]CoverVar, error) {
 	dir := strings.TrimRight(path.Dir(filepath), "/")
 	if dir == "" {
 		dir = "."
@@ -68,8 +68,12 @@ func parseCoverVars(filepath string) ([]CoverVar, error) {
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("malformed cover var line in %v: %v", filepath, line)
 		}
-
-		ret = append(ret, coverVar(dir, parts[0], parts[1]))
+		if external || dir != os.Getenv("PKG_DIR") {
+			ret = append(ret, coverVar(dir, parts[0], parts[1]))
+		} else {
+			// We recompile the test package, so override the import path here to get the right version
+			ret = append(ret, coverVar(dir, testPackage, parts[1]))
+		}
 	}
 
 	return ret, nil
@@ -88,4 +92,3 @@ func coverVar(dir, importPath, v string) CoverVar {
 		File:       f,
 	}
 }
-
diff --git a/tools/please_go/test/find_cover_vars_test.go b/tools/please_go/test/find_cover_vars_test.go
@@ -15,18 +15,18 @@ var coverageVars = []CoverVar{{
 }}
 
 func TestFindCoverVars(t *testing.T) {
-	vars, err := FindCoverVars("tools/please_go/test/test_data", []string{"tools/please_go/test/test_data/x", "tools/please_go/test/test_data/binary"})
+	vars, err := FindCoverVars("tools/please_go/test/test_data", "", false, []string{"tools/please_go/test/test_data/x", "tools/please_go/test/test_data/binary"})
 	assert.NoError(t, err)
 	assert.Equal(t, coverageVars, vars)
 }
 
 func TestFindCoverVarsFailsGracefully(t *testing.T) {
-	_, err := FindCoverVars("wibble", nil)
+	_, err := FindCoverVars("wibble", "", false, nil)
 	assert.Error(t, err)
 }
 
 func TestFindCoverVarsReturnsNothingForEmptyPath(t *testing.T) {
-	vars, err := FindCoverVars("", nil)
+	vars, err := FindCoverVars("", "", false, nil)
 	assert.NoError(t, err)
 	assert.Equal(t, 0, len(vars))
 }
@@ -39,7 +39,7 @@ func TestFindBinaryCoverVars(t *testing.T) {
 		Var:        "GoCover_lock_go",
 		File:       "tools/please_go/test/test_data/binary/lock.go",
 	}}
-	vars, err := FindCoverVars("tools/please_go/test/test_data/binary", nil)
+	vars, err := FindCoverVars("tools/please_go/test/test_data/binary", "", false, nil)
 	assert.NoError(t, err)
 	assert.Equal(t, expected, vars)
 }
diff --git a/tools/please_go/test/gotest.go b/tools/please_go/test/gotest.go
@@ -7,8 +7,8 @@ import (
 )
 
 // PleaseGoTest will generate the test main for the provided sources
-func PleaseGoTest(goTool, dir, testPackage, output string, sources, exclude []string, isBenchmark bool) {
-	coverVars, err := FindCoverVars(dir, exclude)
+func PleaseGoTest(goTool, dir, testPackage, output string, sources, exclude []string, isBenchmark, external bool) {
+	coverVars, err := FindCoverVars(dir, testPackage, external, exclude)
 	if err != nil {
 		log.Fatalf("Error scanning for coverage: %s", err)
 	}