Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/cmd/bazci/githubpost/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
128 changes: 23 additions & 105 deletions pkg/cmd/bazci/githubpost/githubpost.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)

Expand Down Expand Up @@ -455,21 +461,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
}
}
Expand Down Expand Up @@ -630,95 +637,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) {
Expand Down
66 changes: 23 additions & 43 deletions pkg/cmd/bazci/githubpost/githubpost_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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",
}
}

Expand Down
5 changes: 4 additions & 1 deletion pkg/cmd/roachprod-microbench/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ go_test(
"export_test.go",
"github_test.go",
],
data = glob(["testdata/**"]),
data = glob(["testdata/**"]) + [
"//:TEAMS.yaml",
"//.github:CODEOWNERS",
],
embed = [":roachprod-microbench_lib"],
deps = [
"//pkg/cmd/bazci/githubpost/issues",
Expand Down
Loading