testing: fix construction of the testing artifacts path

The existing implementation had a few problems:
- It constructs a path which starts with a forward slash, which is
then immediately rejected by filepath.Localize() as invalid.
- It did not correctly remove the module path from the import path if
the test was in the root package of the module: it would do the
equivalent of strings.TrimPrefix("example.com/jdoe/loader",
"example.com/jdoe/loader/"), which trims nothing, and leaves the full
module name in the base.
- Tests didn't check for any behaviors related to which artifact path
was selected.

Removed leading slash, handle tests in the root package by omitting the
<pkg> component from the artifact dir path, and add tests.

Fixes #77763

Change-Id: I00fc7381b939e4d8a8a9811e2a95fc2d8c5a6e84
GitHub-Last-Rev: 9c39392a69
GitHub-Pull-Request: golang/go#77778
Reviewed-on: https://go-review.googlesource.com/c/go/+/748581
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>

src/testing/testing.go

@@ -1363,30 +1363,7 @@ func (c *common) makeArtifactDir() (string, error) {
 		return c.makeTempDir()
 	}
 
-	// If the test name is longer than maxNameSize, truncate it and replace the last
-	// hashSize bytes with a hash of the full name.
-	const maxNameSize = 64
-	name := strings.ReplaceAll(c.name, "/", "__")
-	if len(name) > maxNameSize {
-		h := fmt.Sprintf("%0x", hashString(name))
-		name = name[:maxNameSize-len(h)] + h
-	}
-
-	// Remove the module path prefix from the import path.
-	pkg := strings.TrimPrefix(c.importPath, c.modulePath+"/")
-
-	// Join with /, not filepath.Join: the import path is /-separated,
-	// and we don't want removeSymbolsExcept to strip \ separators on Windows.
-	base := "/" + pkg + "/" + name
-	base = removeSymbolsExcept(base, "!#$%&()+,-.=@^_{}~ /")
-	base, err := filepath.Localize(base)
-	if err != nil {
-		// This name can't be safely converted into a local filepath.
-		// Drop it and just use _artifacts/<random>.
-		base = ""
-	}
-
-	artifactBase := filepath.Join(artifactDir, base)
+	artifactBase := filepath.Join(artifactDir, c.relativeArtifactBase())
 	if err := os.MkdirAll(artifactBase, 0o777); err != nil {
 		return "", err
 	}
@@ -1400,6 +1377,39 @@ func (c *common) makeArtifactDir() (string, error) {
 	return dir, nil
 }
 
+func (c *common) relativeArtifactBase() string {
+	// If the test name is longer than maxNameSize, truncate it and replace the last
+	// hashSize bytes with a hash of the full name.
+	const maxNameSize = 64
+	name := strings.ReplaceAll(c.name, "/", "__")
+	if len(name) > maxNameSize {
+		h := fmt.Sprintf("%0x", hashString(name))
+		name = name[:maxNameSize-len(h)] + h
+	}
+
+	// Remove the module path prefix from the import path.
+	// If this is the root package, pkg will be empty.
+	pkg := strings.TrimPrefix(c.importPath, c.modulePath)
+	// Remove the leading slash.
+	pkg = strings.TrimPrefix(pkg, "/")
+
+	base := name
+	if pkg != "" {
+		// Join with /, not filepath.Join: the import path is /-separated,
+		// and we don't want removeSymbolsExcept to strip \ separators on Windows.
+		base = pkg + "/" + name
+	}
+	base = removeSymbolsExcept(base, "!#$%&()+,-.=@^_{}~ /")
+	base, err := filepath.Localize(base)
+	if err != nil {
+		// This name can't be safely converted into a local filepath.
+		// Drop it and just use _artifacts/<random>.
+		base = ""
+	}
+
+	return base
+}
+
 func removeSymbolsExcept(s, allowed string) string {
 	mapper := func(r rune) rune {
 		if unicode.IsLetter(r) ||

src/testing/testing_test.go

@@ -1123,6 +1123,101 @@ func TestArtifactDirConsistent(t *testing.T) {
 	}
 }
 
+func TestArtifactDirectoryPaths(t *testing.T) {
+	testenv.MustHaveExec(t)
+	t.Parallel()
+
+	tempDir := t.TempDir()
+
+	// 1. Setup the temporary module.
+	if err := os.WriteFile(filepath.Join(tempDir, "go.mod"), []byte("module example.com/testmod\n"), 0666); err != nil {
+		t.Fatal(err)
+	}
+
+	writeTmpl := func(pkgDir string, content string) {
+		fullDir := filepath.Join(tempDir, pkgDir)
+		if err := os.MkdirAll(fullDir, 0777); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(fullDir, "a_test.go"), []byte(content), 0666); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Root package test
+	writeTmpl(".", `package root_test
+import "testing"
+func TestRootNiceName(t *testing.T) { t.ArtifactDir() }
+`)
+
+	// Subpackage test
+	writeTmpl("subpkg", `package subpkg_test
+import "testing"
+func TestSubNiceName(t *testing.T) { t.ArtifactDir() }
+`)
+
+	// Deep subpackage test with various scenarios
+	writeTmpl("deep/nested/pkg", `package pkg_test
+import "testing"
+func TestNiceName(t *testing.T) { t.ArtifactDir() }
+func TestParent(t *testing.T) {
+	t.Run("SubTest", func(t *testing.T) { t.ArtifactDir() })
+}
+func TestVeryLongNameThatExceedsSixtyFourCharactersAndThereforeMustBeTruncatedAndHashed(t *testing.T) { t.ArtifactDir() }
+func TestInvalid_Chars_In_Name(t *testing.T) { t.ArtifactDir() }
+`)
+	// We use "TestInvalid_Chars_In_Name" because go test framework parses functions by ^Test.
+	// But let's actually make it have invalid path chars:
+	writeTmpl("deep/nested/pkg2", `package pkg2_test
+import "testing"
+func TestInvalid_Chars_In_Name(t *testing.T) {
+	t.Run("SubTest:with*stars", func(t *testing.T) { t.ArtifactDir() })
+}
+`)
+
+	// 2. Run the tests.
+	cmd := testenv.Command(t, testenv.GoToolPath(t), "test", "-v", "-artifacts", "./...")
+	cmd.Dir = tempDir
+	cmd = testenv.CleanCmdEnv(cmd)
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("%v failed: %v\n%s", cmd, err, out)
+	}
+
+	// 3. Parse and verify output.
+	tests := []struct {
+		name       string
+		wantPrefix string
+	}{
+		// Root package tests don't have a package prefix because modulePath == importPath
+		{"TestRootNiceName", "TestRootNiceName"},
+		{"TestSubNiceName", "subpkg/TestSubNiceName"},
+		{"TestNiceName", "deep/nested/pkg/TestNiceName"},
+		{"TestParent/SubTest", "deep/nested/pkg/TestParent__SubTest"},
+		{"TestVeryLongNameThatExceedsSixtyFourCharactersAndThereforeMustBeTruncatedAndHashed", `deep/nested/pkg/TestVeryLongNameThatExceedsSixtyFourCharactersAn[0-9a-f]+`},
+		{`TestInvalid_Chars_In_Name/SubTest:with\*stars`, `deep/nested/pkg2/TestInvalid_Chars_In_Name__SubTestwithstars`},
+	}
+
+	for _, tt := range tests {
+		re := regexp.MustCompile(`=== ARTIFACTS ` + tt.name + ` ([^\n]+)`)
+		match := re.FindSubmatch(out)
+		if match == nil {
+			t.Errorf("expected output matching %q, got\n%q", re, out)
+			continue
+		}
+		artifactDir := string(match[1])
+
+		slashDir := filepath.ToSlash(artifactDir)
+
+		wantSuffixPattern := `_artifacts/` + tt.wantPrefix + `/[^/]+$`
+		wantRegex := regexp.MustCompile(wantSuffixPattern)
+
+		if !wantRegex.MatchString(slashDir) {
+			t.Errorf("artifact directory for %s: got %s, want suffix matching %s", tt.name, slashDir, wantSuffixPattern)
+		}
+	}
+}
+
 func checkArtifactDir(t *testing.T, out []byte, testName, outputDir string) {
 	t.Helper()