dev: avoid re-generating logictest files if unnecessary #110735

liamgillies · 2023-09-15T17:25:19Z

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

blathers-crl · 2023-09-15T17:25:22Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-09-15T17:25:29Z

This change is

rickystewart

Let's tighten up the commit title a bit. avoid rerunning unchanged tests is not the point of this PR. avoid re-generating logictest files if unnecessary is closer, for example.

rickystewart · 2023-09-15T17:33:32Z

pkg/cmd/dev/testlogic.go

@@ -97,6 +97,8 @@ func (d *dev) testlogic(cmd *cobra.Command, commandLine []string) error {
 		ignoreCache = true
 	}

+	changes, _ := d.exec.CommandContextSilent(ctx, "git", "status")


git status reports the changed files on top of the currently-checked-out commit. This is not sufficient to accomplish what we want. For example, consider that I might make a change to the logic test files, then git commit the result -- this wouldn't trigger since git status won't report any changed files, but that's incorrect.

What we want to do is diff the changed files between the merge-base and HEAD. The function determineAffectedTargets() demonstrates one way to do this. You will probably have to refactor that function into a different function to do the remote parsing, the merge-base calculation, and the diff.

We want to skip code generation if there are no updated relevant files. It has no bearing on which tests are run.

rickystewart · 2023-09-15T17:34:23Z

Beforehand, running dev testlogic would always run all tests regarldess if they were changed or not, which wastes time.

Actually, we are going to run the same set of tests either way, so this part of the commit message is not correct either. What we specifically want to do is skip code generation in this case.

rickystewart · 2023-09-15T20:51:00Z

pkg/cmd/dev/testlogic.go

@@ -58,6 +60,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, "skip generating logic test files before running logic tests")


Incorrect comment on the flag -- looks like a bad copy-paste.

rickystewart · 2023-09-15T20:52:03Z

pkg/cmd/dev/testlogic.go

+	if err != nil {
+		return err
+	}
+	if (shouldGen && !noGen) || forceGen {


If someone passes --no-gen or --force-gen, it's a big waste of time to perform the checks with git. Instead, short-circuit if one of those flags is passed.

rickystewart · 2023-09-15T20:52:14Z

pkg/cmd/dev/testlogic.go

@@ -297,6 +305,39 @@ 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) shouldGen(ctx context.Context) (bool, error) {


Let's call this shouldGenerateLogicTests or something -- shouldGen is not descriptive enough.

rickystewart · 2023-09-15T20:52:37Z

pkg/cmd/dev/testlogic.go

+// modified in the current branch, and if so, determines if we should re-generate logic tests.
+func (d *dev) shouldGen(ctx context.Context) (bool, error) {
+	// List files changed against `master`.
+	remotes, err := d.exec.CommandContextSilent(ctx, "git", "remote", "-v")


There is lots of duplicated code with pkg/cmd/dev/test.go. Please refactor into a helper function.

rickystewart · 2023-09-15T20:53:34Z

pkg/cmd/dev/testlogic.go

+		}
+	}
+	if upstream == "" {
+		return true, fmt.Errorf("could not find git upstream, run `git remote add upstream [email protected]:cockroachdb/cockroach.git`")


As discussed in DM: failures should probably be ignored rather than causing everything to fail. The tests don't literally need this optimization to run.

rickystewart · 2023-09-18T16:24:45Z

pkg/cmd/dev/testlogic.go

@@ -127,11 +131,18 @@ func (d *dev) testlogic(cmd *cobra.Command, commandLine []string) error {
 		return err
 	}

-	if !noGen {
+	if forceGen {


if !noGen { if forceGen || d.shouldGenerateLogicTests(ctx) { ... } }

Just reduces some duplication to write the conditional this way. I suppose this is equivalent:

if !noGen && (forceGen || d.shouldGenerateLogicTests(ctx)) { .... }

rickystewart · 2023-09-18T16:26:06Z

pkg/cmd/dev/testlogic.go

+}
+
+// This function retrieves the merge-base hash between the current branch and master
+func (d *dev) getMergeBaseHash(ctx context.Context) (string, error) {


Let's put this in util.go as it is not specific to logic tests or any other specific subcommand.

rickystewart · 2023-09-18T19:06:40Z

pkg/cmd/dev/testlogic.go

+	if base == "" {
+		return true
+	}
+	changedFiles, _ := d.exec.CommandContextSilent(ctx, "git", "diff", "--no-ext-diff", "--name-only", base, "--", "**/logic_test/**", "**/execbuilder/testdata/**")


As mentioned in DM: use the list of files from build/bazelutil/bazel-generate.sh so we're consistent with it.

if files_unchanged_from_upstream WORKSPACE $(find_relevant ./pkg/sql/logictest/logictestbase -name BUILD.bazel -or -name '*.go') $(find_relevant ./pkg/sql/logictest/testdata -name '*') $(find_relevant ./pkg/sql/sqlitelogictest -name BUILD.bazel -or -name '*.go') $(find_relevant ./pkg/ccl/logictestccl/testdata -name '*') $(find_relevant pkg/sql/opt/exec/execbuilder/testdata -name '*'); then

I translate that to the following git diff command:

git diff ... -- 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/**

Oh sorry I was a little confused reading the DM -- thought you meant those files referred to the second option in avoiding failing unit tests. Fixed now.

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: cockroachdb#94845 Release note: None

liamgillies · 2023-09-19T14:58:42Z

bors r=rickystewart

craig · 2023-09-19T17:15:39Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-09-19T19:25:00Z

Build succeeded:

Bazel Essential CI (Cockroach)

Beforehand, running a cross build inside a git worktree would always fail. This PR adds code to also mount the main git repo into docker when in a git worktree, so that running cross builds will succeed. Fixes: cockroachdb#110735 Release note: None

110919: kvcoord: Replace rangefeed catchup semaphore with rate limiter r=miretskiy a=miretskiy The catchup scan limit was added in #77725 in order to attempt to restrict a single rangefeed from consuming all catchup scan slots on the KV side. This client side limit has been largely ineffective. More recently, this semaphore has been coopted in #109346 in order to pace goroutine creation rate on the client. This functionality is important, but the implementation is not very good. Namely, we are interested in controling the rate of new catchup scans being started by the client (and thus, control the rate of goroutine creation). This implementation replaces the old implementation with rate limit based approach. The rate limits are configured using `kv.rangefeed.client.stream_startup_rate` setting which sets the rate on the number of newly established rangefeed streams (default 100). Closes #110439 Epic: CRDB-26372 Release note: None 110929: dev: fix cross builds running in git worktrees r=rickystewart a=liamgillies Beforehand, running a cross build inside a git worktree would always fail. This PR adds code to also mount the main git repo into docker when in a git worktree, so that running cross builds will succeed. Fixes: #110735 Release note: None Co-authored-by: Yevgeniy Miretskiy <[email protected]> Co-authored-by: Liam Gillies <[email protected]>

liamgillies requested a review from a team as a code owner September 15, 2023 17:25

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch from 5bb7c3e to 2ee5472 Compare September 15, 2023 17:26

rickystewart requested changes Sep 15, 2023

View reviewed changes

liamgillies changed the title ~~dev: avoid rerunning unchanged tests~~ dev: avoid re-generating logictest files if unnecessary Sep 15, 2023

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch from 2ee5472 to 973eabb Compare September 15, 2023 20:34

liamgillies requested a review from rickystewart September 15, 2023 20:34

rickystewart requested changes Sep 15, 2023

View reviewed changes

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch from 973eabb to 8d750af Compare September 18, 2023 15:58

rickystewart requested changes Sep 18, 2023

View reviewed changes

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch 5 times, most recently from 2979aaa to 6ce4ad8 Compare September 18, 2023 18:15

liamgillies requested a review from rickystewart September 18, 2023 19:03

rickystewart requested changes Sep 18, 2023

View reviewed changes

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch from 6ce4ad8 to 37fc9ef Compare September 18, 2023 19:29

liamgillies requested a review from rickystewart September 18, 2023 19:29

rickystewart approved these changes Sep 18, 2023

View reviewed changes

liamgillies force-pushed the avoid-rerunning-unchanged-tests branch from 37fc9ef to 9e653ae Compare September 18, 2023 20:02

craig bot merged commit 3071cac into cockroachdb:master Sep 19, 2023

liamgillies mentioned this pull request Sep 19, 2023

dev: fix cross builds running in git worktrees #110929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev: avoid re-generating logictest files if unnecessary #110735

dev: avoid re-generating logictest files if unnecessary #110735

liamgillies commented Sep 15, 2023 •

edited

Loading

blathers-crl bot commented Sep 15, 2023

cockroach-teamcity commented Sep 15, 2023

rickystewart left a comment

rickystewart Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart commented Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart Sep 15, 2023

rickystewart Sep 18, 2023

rickystewart Sep 18, 2023

rickystewart Sep 18, 2023

liamgillies Sep 18, 2023

liamgillies commented Sep 19, 2023

craig bot commented Sep 19, 2023

craig bot commented Sep 19, 2023

dev: avoid re-generating logictest files if unnecessary #110735

dev: avoid re-generating logictest files if unnecessary #110735

Conversation

liamgillies commented Sep 15, 2023 • edited Loading

blathers-crl bot commented Sep 15, 2023

cockroach-teamcity commented Sep 15, 2023

rickystewart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickystewart commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamgillies commented Sep 19, 2023

craig bot commented Sep 19, 2023

craig bot commented Sep 19, 2023

liamgillies commented Sep 15, 2023 •

edited

Loading