logictest: add TestLogic_tmp for testing untracked logic test files

[mgartner]

This commit improves the developer experience when working with logic
tests. The logic test generator no longer generates a dedicated test for
logic tests files prefixed with "_". The hard-coded TestLogic_tmp test
has been added which runs all such logic test files. This allows
developers to create and run temporary test files that are not checked
into the repository, without repeatedly regenerating and reverting
changes to the generated_test.go files.

For example, if you add a git-ignored file logic test file,
pkg/sql/logictest/testdata/logic_test/__test, you can run it with the
local config with the command:

./dev test pkg/sql/logictest/tests/local -f TestLogic_tmp

Previously, to facilitate running these types of tests, ./dev test
would automatically regenerate logic tests every time tests were run.
This was primarily motivated by the desire to make testing temporary
logic test files easier - a developer would not have to run
./dev gen logictest every time they wanted to test a temporary file.

However, this was a poor experience because it made running tests
significantly slower, and logic tests generated for temporary logic test
files would have to be reverted repeatedly. TestLogic_tmp eliminates
the need for this automatic regeneration, so it has been removed. The
--no-gen flag, which disabled automatic regeneration, has also been
removed. When committing a new logic test file to the repository,
developers must now run ./dev gen testlogic to generate logic tests
for it.

Release note: None

[cockroach-teamcity]

This change is 

[rickystewart]

Not 100% sure about deleting this flag. I think this functionality is still useful.

One option if you find the --no-gen thing annoying is to add support for an environment variable (like DEV_NO_GENERATE_LOGICTEST) so you can set it in your profile. We already support environment variables like DEV_NO_REMOTE_CACHE for turning off functionality globally.

[mgartner]

Do you think it's still useful for dev test too, or just dev logictest?

[rickystewart]

I can go either way on dev test, definitely I think keep it for testlogic though.

[mgartner]

SGTM. I added it back to just testlogic for now.

[yuzefovich]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @mgartner)

---

-- commits line 6 at r1:
Thanks for doing this! Should we add some linter to prevent from checking in any of the logic test files that start with the underscore?

[mgartner]

-- commits line 6 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Thanks for doing this! Should we add some linter to prevent from checking in any of the logic test files that start with the underscore?

We can do that, but I wouldn't say its strictly necessary. If a file with an underscore prefix was added - it would be tested in CI, not ignored. So we wouldn't unknowingly add bad tests. The only risk would be adding so many underscore-prefixed test files that we start to see timeouts again.

I created an issue to track this: #85457

[yuzefovich]

Reviewed 1 of 44 files at r1, 44 of 44 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @rickystewart)

---

-- commits line 28 at r2:
nit: I think this part of the commit message needs an update.

---

pkg/cmd/dev/test.go line 227 at r2 (raw file):

	// We need to re-generate logictest files if we're testing a
	// logictest target.
	if !noGen {

This removal might require a bump to the version of cockroachdb/dev.

[rickystewart]

Awesome, thanks for this work.

This removal might require a bump to the version of cockroachdb/dev.

+1

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @yuzefovich)

---

-- commits line 6 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Thanks for doing this! Should we add some linter to prevent from checking in any of the logic test files that start with the underscore?

We can do that, but I wouldn't say its strictly necessary. If a file with an underscore prefix was added - it would be tested in CI, not ignored. So we wouldn't unknowingly add bad tests. The only risk would be adding so many underscore-prefixed test files that we start to see timeouts again.

I created an issue to track this: #85457

---

-- commits line 28 at r2:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think this part of the commit message needs an update.

Done.

---

pkg/cmd/dev/test.go line 227 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This removal might require a bump to the version of cockroachdb/dev.

Done.

[mgartner]

TFTRs!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)