Merge pull request #158710 from rickystewart/blathers/backport-release-25.2-158548

release-25.2: codeowners: extract GetTestOwner function into new public helper method

Author: rickystewart

pkg/cmd/bazci/githubpost/BUILD.bazel

@@ -26,6 +26,7 @@ go_test(
     deps = [
         "//pkg/build/util",
         "//pkg/cmd/bazci/githubpost/issues",
+        "//pkg/internal/codeowners",
         "//pkg/testutils/datapathutils",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",

pkg/cmd/bazci/githubpost/githubpost.go

@@ -20,12 +20,11 @@ import (
 	"io"
 	"log"
 	"os"
-	"os/exec"
-	"path/filepath"
 	"regexp"
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	buildutil "github.com/cockroachdb/cockroach/pkg/build/util"
@@ -40,23 +39,30 @@ const (
 	unknown = "(unknown)"
 )
 
-type packageAndTest struct {
-	packageName, testName string
-}
+var (
+	co             *codeowners.CodeOwners
+	codeownersOnce sync.Once
+)
 
-type fileAndLine struct {
-	filename, linenum string
+func initCo() {
+	codeownersOnce.Do(func() {
+		co_, err := codeowners.DefaultLoadCodeOwners()
+		if err != nil {
+			panic(err)
+		}
+		co = co_
+	})
 }
 
-// This should be set to some non-nil map in test scenarios. This is used to
-// mock out the results of the getFileLine function.
-var fileLineForTesting map[packageAndTest]fileAndLine
-
 type Formatter func(context.Context, Failure) (issues.IssueFormatter, issues.PostRequest)
 type FailurePoster func(context.Context, Failure) error
 
 func DefaultFormatter(ctx context.Context, f Failure) (issues.IssueFormatter, issues.PostRequest) {
-	teams := getOwner(ctx, f.packageName, f.testName)
+	initCo()
+	teams, logs := co.GetTestOwner(f.packageName, f.testName)
+	for _, line := range logs {
+		log.Println(line)
+	}
 	repro := fmt.Sprintf("./dev test ./pkg/%s --race --stress -f %s",
 		trimPkg(f.packageName), f.testName)
 
@@ -448,21 +454,22 @@ func listFailuresFromJSON(
 	// 2) Otherwise, we don't blame anybody in particular. We file a generic issue
 	// listing the package name containing the report of long-running tests.
 	if timedOutCulprit.name != "" {
+		initCo()
 		slowest := slowFailEvents[0]
 		if len(slowPassEvents) > 0 && slowPassEvents[0].Elapsed > slowest.Elapsed {
 			slowest = slowPassEvents[0]
 		}
 
-		culpritOwner := fmt.Sprintf("%v", getOwner(ctx, timedOutCulprit.pkg, timedOutCulprit.name))
+		culpritOwner, _ := co.GetTestOwner(timedOutCulprit.pkg, timedOutCulprit.name)
 		slowEvents := append(slowFailEvents, slowPassEvents...)
 		// The predicate determines if the union of the slow events is owned by the _same_ team(s) as timedOutCulprit.
 		hasSameOwner := func() bool {
 			for _, slowEvent := range slowEvents {
 				scopedEvent := scoped(slowEvent)
-				owner := fmt.Sprintf("%v", getOwner(ctx, scopedEvent.pkg, scopedEvent.name))
+				owner, _ := co.GetTestOwner(scopedEvent.pkg, scopedEvent.name)
 
-				if culpritOwner != owner {
-					log.Printf("%v has a different owner: %s;bailing out...\n", scopedEvent, owner)
+				if len(culpritOwner) < 1 || len(owner) < 1 || culpritOwner[0].TeamName != owner[0].TeamName {
+					log.Printf("%v has a different owner: %v;bailing out...\n", scopedEvent, owner)
 					return false
 				}
 			}
@@ -623,95 +630,6 @@ func writeSlowTestsReport(report string) error {
 	return os.WriteFile("artifacts/slow-tests-report.txt", []byte(report), 0644)
 }
 
-// getFileLine returns the file (relative to repo root) and line for the given test.
-// The package name is assumed relative to the repo root as well, i.e. pkg/foo/bar.
-func getFileLine(
-	ctx context.Context, packageName, testName string,
-) (_filename string, _linenum string, _ error) {
-	if fileLineForTesting != nil {
-		res, ok := fileLineForTesting[packageAndTest{packageName: packageName, testName: testName}]
-		if ok {
-			return res.filename, res.linenum, nil
-		}
-		return "", "", errors.Newf("could not find testing line:num for %s.%s", packageName, testName)
-	}
-	// Search the source code for the email address of the last committer to touch
-	// the first line of the source code that contains testName. Then, ask GitHub
-	// for the GitHub username of the user with that email address by searching
-	// commits in cockroachdb/cockroach for commits authored by the address.
-	subtests := strings.Split(testName, "/")
-	testName = subtests[0]
-	packageName = strings.TrimPrefix(packageName, "github.com/cockroachdb/cockroach/")
-	for {
-		if !strings.Contains(packageName, "pkg") {
-			return "", "", errors.Newf("could not find test %s", testName)
-		}
-		cmd := exec.Command(`/bin/bash`, `-c`,
-			fmt.Sprintf(`cd "$(git rev-parse --show-toplevel)" && git grep -n 'func %s(' '%s/*_test.go'`,
-				testName, packageName))
-		// This command returns output such as:
-		// ../ccl/storageccl/export_test.go:31:func TestExportCmd(t *testing.T) {
-		out, err := cmd.CombinedOutput()
-		if err != nil {
-			fmt.Printf("couldn't find test %s in %s: %s %+v\n",
-				testName, packageName, string(out), err)
-			packageName = filepath.Dir(packageName)
-			continue
-		}
-		re := regexp.MustCompile(`(.*):(.*):`)
-		// The first 2 :-delimited fields are the filename and line number.
-		matches := re.FindSubmatch(out)
-		if matches == nil {
-			fmt.Printf("couldn't find filename/line number for test %s in %s: %s %+v\n",
-				testName, packageName, string(out), err)
-			packageName = filepath.Dir(packageName)
-			continue
-		}
-		return string(matches[1]), string(matches[2]), nil
-	}
-}
-
-// getOwner looks up the file containing the given test and returns
-// the owning teams. It does not return
-// errors, but instead simply returns what it can.
-// In case no owning team is found, "test-eng" team is returned.
-func getOwner(ctx context.Context, packageName, testName string) (_teams []team.Team) {
-	filename, _, err := getFileLine(ctx, packageName, testName)
-	if err != nil {
-		log.Printf("error getting file:line for %s.%s: %s", packageName, testName, err)
-		// Let's continue so that we can assign the "catch-all" owner.
-	}
-	co, err := codeowners.DefaultLoadCodeOwners()
-	if err != nil {
-		log.Printf("loading codeowners: %s", err)
-		return nil
-	}
-	match := co.Match(filename)
-
-	if match == nil {
-		// N.B. if no owning team is found, we default to 'test-eng'. This should be a rare exception rather than the rule.
-		testEng := co.GetTeamForAlias("cockroachdb/test-eng")
-		if testEng.Name() == "" {
-			log.Fatalf("test-eng team could not be found in TEAMS.yaml")
-		}
-
-		// Workaround for #107885.
-		if strings.Contains(packageName, "backup") {
-			dr := co.GetTeamForAlias("cockroachdb/disaster-recovery")
-			if dr.Name() == "" {
-				log.Fatalf("disaster-recovery team could not be found in TEAMS.yaml")
-			}
-
-			log.Printf("assigning %s.%s to 'disaster-recovery' due to #107885", packageName, testName)
-			match = []team.Team{dr}
-		} else {
-			log.Printf("assigning %s.%s to 'test-eng' as catch-all", packageName, testName)
-			match = []team.Team{testEng}
-		}
-	}
-	return match
-}
-
 func formatPebbleMetamorphicIssue(
 	ctx context.Context, f Failure,
 ) (issues.IssueFormatter, issues.PostRequest) {

pkg/cmd/bazci/githubpost/githubpost_test.go

@@ -14,6 +14,7 @@ import (
 
 	bazelutil "github.com/cockroachdb/cockroach/pkg/build/util"
 	"github.com/cockroachdb/cockroach/pkg/cmd/bazci/githubpost/issues"
+	"github.com/cockroachdb/cockroach/pkg/internal/codeowners"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -30,56 +31,35 @@ type issue struct {
 }
 
 func init() {
-	fileLineForTesting = map[packageAndTest]fileAndLine{
+	codeowners.FileMapForTesting = map[codeowners.PackageAndTest]string{
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/util/stop",
-			testName:    "TestStopperWithCancelConcurrent",
-		}: {
-			filename: "pkg/util/stop/stopper_test.go",
-			linenum:  "375",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/util/stop",
+			TestName:    "TestStopperWithCancelConcurrent",
+		}: "pkg/util/stop/stopper_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/kv/kvserver",
-			testName:    "TestReplicateQueueRebalance",
-		}: {
-			filename: "pkg/kv/kvserver/replicate_queue_test.go",
-			linenum:  "56",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/kv/kvserver",
+			TestName:    "TestReplicateQueueRebalance",
+		}: "pkg/kv/kvserver/replicate_queue_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/kv/kvserver",
-			testName:    "TestGossipHandlesReplacedNode",
-		}: {
-			filename: "pkg/kv/kvserver/gossip_test.go",
-			linenum:  "142",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/kv/kvserver",
+			TestName:    "TestGossipHandlesReplacedNode",
+		}: "pkg/kv/kvserver/gossip_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/util/json",
-			testName:    "TestPretty",
-		}: {
-			filename: "pkg/util/json/json_test.go",
-			linenum:  "2234",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/util/json",
+			TestName:    "TestPretty",
+		}: "pkg/util/json/json_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/util/json",
-			testName:    "TestJSONErrors",
-		}: {
-			filename: "pkg/util/json/json_test.go",
-			linenum:  "249",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/util/json",
+			TestName:    "TestJSONErrors",
+		}: "pkg/util/json/json_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord",
-			testName:    "TestTxnCoordSenderPipelining",
-		}: {
-			filename: "pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go",
-			linenum:  "2429",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord",
+			TestName:    "TestTxnCoordSenderPipelining",
+		}: "pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go",
 		{
-			packageName: "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord",
-			testName:    "TestAbortReadOnlyTransaction",
-		}: {
-			filename: "pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go",
-			linenum:  "1990",
-		},
+			PackageName: "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord",
+			TestName:    "TestAbortReadOnlyTransaction",
+		}: "pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go",
 	}
 }
 

pkg/cmd/roachprod-microbench/BUILD.bazel

@@ -55,7 +55,10 @@ go_test(
         "executor_test.go",
         "export_test.go",
     ],
-    data = glob(["testdata/**"]),
+    data = glob(["testdata/**"]) + [
+        "//:TEAMS.yaml",
+        "//.github:CODEOWNERS",
+    ],
     embed = [":roachprod-microbench_lib"],
     deps = [
         "//pkg/cmd/roachprod-microbench/model",