From 9e653ae7484dc41f56b13f2bd81a32b9d782c7dd Mon Sep 17 00:00:00 2001 From: Liam Gillies Date: Fri, 15 Sep 2023 13:19:28 -0400 Subject: [PATCH] dev: avoid re-generating logictest files if unnecessary Beforehand, running `dev testlogic` would always run all tests regardless if they were modified, which wastes time. This PR will check to see if logic test folders were modified and only re-generates them if they are modified. It also introduces a new flag, `--force-gen`, which will force generate the logic tests. Fixes: #94845 Release note: None --- pkg/cmd/dev/test.go | 20 +------------------- pkg/cmd/dev/testlogic.go | 22 +++++++++++++++++++++- pkg/cmd/dev/util.go | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/dev/test.go b/pkg/cmd/dev/test.go index 3db859a4dd27..d157cf7bfbfc 100644 --- a/pkg/cmd/dev/test.go +++ b/pkg/cmd/dev/test.go @@ -477,28 +477,10 @@ func getDirectoryFromTarget(target string) string { } func (d *dev) determineAffectedTargets(ctx context.Context) ([]string, error) { - // List files changed against `master`. - remotes, err := d.exec.CommandContextSilent(ctx, "git", "remote", "-v") + base, err := d.getMergeBaseHash(ctx) if err != nil { return nil, err } - var upstream string - for _, remote := range strings.Split(strings.TrimSpace(string(remotes)), "\n") { - if (strings.Contains(remote, "github.com/cockroachdb/cockroach") || strings.Contains(remote, "github.com:cockroachdb/cockroach")) && strings.HasSuffix(remote, "(fetch)") { - upstream = strings.Fields(remote)[0] - break - } - } - if upstream == "" { - return nil, fmt.Errorf("could not find git upstream") - } - - baseBytes, err := d.exec.CommandContextSilent(ctx, "git", "merge-base", fmt.Sprintf("%s/master", upstream), "HEAD") - if err != nil { - return nil, err - } - base := strings.TrimSpace(string(baseBytes)) - changedFiles, err := d.exec.CommandContextSilent(ctx, "git", "diff", "--no-ext-diff", "--name-only", base, "--", "*.go", "**/testdata/**") if err != nil { return nil, err diff --git a/pkg/cmd/dev/testlogic.go b/pkg/cmd/dev/testlogic.go index fc6d3aebd5de..92d139887c23 100644 --- a/pkg/cmd/dev/testlogic.go +++ b/pkg/cmd/dev/testlogic.go @@ -11,6 +11,7 @@ package main import ( + "context" "errors" "fmt" "log" @@ -19,6 +20,7 @@ import ( "strings" "time" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/spf13/cobra" ) @@ -29,6 +31,7 @@ const ( configsFlag = "config" showSQLFlag = "show-sql" noGenFlag = "no-gen" + forceGenFlag = "force-gen" flexTypesFlag = "flex-types" workmemFlag = "default-workmem" ) @@ -58,6 +61,7 @@ func makeTestLogicCmd(runE func(cmd *cobra.Command, args []string) error) *cobra testLogicCmd.Flags().Bool(showSQLFlag, false, "show SQL statements/queries immediately before they are tested") testLogicCmd.Flags().Bool(rewriteFlag, false, "rewrite test files using results from test run") testLogicCmd.Flags().Bool(noGenFlag, false, "skip generating logic test files before running logic tests") + testLogicCmd.Flags().Bool(forceGenFlag, false, "force generating logic test files before running logic tests") testLogicCmd.Flags().Bool(streamOutputFlag, false, "stream test output during run") testLogicCmd.Flags().Bool(stressFlag, false, "run tests under stress") testLogicCmd.Flags().String(testArgsFlag, "", "additional arguments to pass to go test binary") @@ -85,6 +89,7 @@ func (d *dev) testlogic(cmd *cobra.Command, commandLine []string) error { timeout = mustGetFlagDuration(cmd, timeoutFlag) verbose = mustGetFlagBool(cmd, vFlag) noGen = mustGetFlagBool(cmd, noGenFlag) + forceGen = mustGetFlagBool(cmd, forceGenFlag) showSQL = mustGetFlagBool(cmd, showSQLFlag) count = mustGetFlagInt(cmd, countFlag) stress = mustGetFlagBool(cmd, stressFlag) @@ -127,7 +132,7 @@ func (d *dev) testlogic(cmd *cobra.Command, commandLine []string) error { return err } - if !noGen { + if !noGen && (forceGen || d.shouldGenerateLogicTests(ctx)) { err := d.generateLogicTest(cmd) if err != nil { return err @@ -297,6 +302,21 @@ func (d *dev) testlogic(cmd *cobra.Command, commandLine []string) error { return nil } +// This function determines if any test_logic or execbuilder/testdata files were +// modified in the current branch, and if so, determines if we should re-generate logic tests. +func (d *dev) shouldGenerateLogicTests(ctx context.Context) bool { + if buildutil.CrdbTestBuild { + return true + } + base, _ := d.getMergeBaseHash(ctx) + // Generate logic tests if the merge base hash isn't found + if base == "" { + return true + } + changedFiles, _ := d.exec.CommandContextSilent(ctx, "git", "diff", "--no-ext-diff", "--name-only", base, "--", "pkg/sql/logictest/logictestbase/** ", "pkg/sql/logictest/testdata/**", "pkg/sql/sqlitelogictest/BUILD.bazel", "pkg/sql/sqlitelogictest/sqlitelogictest.go", "pkg/ccl/logictestccl/testdata/**", "pkg/sql/opt/exec/execbuilder/testdata/**") + return strings.TrimSpace(string(changedFiles)) != "" +} + // We know that the regular expressions for files should not contain whitespace // because the file names do not contain whitespace. We support users passing // whitespace separated regexps for multiple files. diff --git a/pkg/cmd/dev/util.go b/pkg/cmd/dev/util.go index 9d76e73c6232..541422820b28 100644 --- a/pkg/cmd/dev/util.go +++ b/pkg/cmd/dev/util.go @@ -249,3 +249,27 @@ func (d *dev) warnAboutChangeInStressBehavior(timeout time.Duration) { log.Printf("Set DEV_I_UNDERSTAND_ABOUT_STRESS=1 to squelch this message") } } + +// This function retrieves the merge-base hash between the current branch and master +func (d *dev) getMergeBaseHash(ctx context.Context) (string, error) { + // List files changed against `master` + remotes, err := d.exec.CommandContextSilent(ctx, "git", "remote", "-v") + if err != nil { + return "", err + } + var upstream string + for _, remote := range strings.Split(strings.TrimSpace(string(remotes)), "\n") { + if (strings.Contains(remote, "github.com/cockroachdb/cockroach") || strings.Contains(remote, "github.com:cockroachdb/cockroach")) && strings.HasSuffix(remote, "(fetch)") { + upstream = strings.Fields(remote)[0] + break + } + } + if upstream == "" { + return "", fmt.Errorf("could not find git upstream, run `git remote add upstream git@github.com:cockroachdb/cockroach.git`") + } + baseBytes, err := d.exec.CommandContextSilent(ctx, "git", "merge-base", fmt.Sprintf("%s/master", upstream), "HEAD") + if err != nil { + return "", err + } + return strings.TrimSpace(string(baseBytes)), nil +}