Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testutils: run roachvet on test files #70194

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Sep 14, 2021

The previously constructed extended regular expression would end up
looking like:

(pkg/*_test.go)|(pkg/workload)|...|(pkg/testutil):.*Use of go keyword not allowed

As a result, roachvet was being skipped on all test files and all but
the last package in this list.

Fixes #70193

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna stevendanna force-pushed the ssd/test-lint-oops branch 4 times, most recently from aae77e4 to 060fef4 Compare September 14, 2021 16:20
@stevendanna stevendanna marked this pull request as ready for review September 14, 2021 16:21
@stevendanna stevendanna requested a review from a team September 14, 2021 16:21
@stevendanna stevendanna requested a review from a team as a code owner September 14, 2021 16:21
@stevendanna stevendanna requested a review from a team September 14, 2021 16:21
@stevendanna stevendanna requested a review from a team as a code owner September 14, 2021 16:21
@stevendanna stevendanna requested a review from a team September 14, 2021 16:21
@stevendanna stevendanna requested review from a team as code owners September 14, 2021 16:21
@stevendanna stevendanna requested review from shermanCRL and removed request for a team September 14, 2021 16:21
@@ -423,7 +423,7 @@ func TestReliableIntentCleanup(t *testing.T) {
// testTxnExecute executes a transaction for testTxn. It is a separate function
// so that any transaction errors can be retried as appropriate.
testTxnExecute := func(t *testing.T, spec testTxnSpec, txn *kv.Txn, txnKey roachpb.Key) error {
ctx, cancel := context.WithCancel(ctx)
tCtx, cancel := context.WithCancel(ctx)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my least favourite change.

@stevendanna
Copy link
Collaborator Author

I'll probably squash these commits before merging, but doing this linter-by-linter was the easiest.

@shermanCRL shermanCRL requested review from adityamaru and removed request for shermanCRL September 14, 2021 16:31
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

I did leave some comments but seeing how big this PR is you may want to just get it merged and then send a follow-up if necessary. Thanks for chewing through this!

Reviewed 8 of 8 files at r1, 4 of 4 files at r2, 7 of 7 files at r3, 2 of 2 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 3 of 3 files at r7, 1 of 1 files at r8, 3 of 3 files at r9, 4 of 4 files at r10, 1 of 1 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, 7 of 7 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @stevendanna)


pkg/ccl/changefeedccl/changefeed_test.go, line 4286 at r3 (raw file):

		// Wait for the high water mark to be non-zero.
		testutils.SucceedsSoon(t, func() error {
			progress = loadProgress()

This is strictly worse though? If this is what the linter wants us to do, I think the solution is to turn off the linter?


pkg/ccl/changefeedccl/helpers_test.go, line 192 at r13 (raw file):

		actualFormatted, err = stripTsFromPayloads(actual)
		if err != nil {
			return err

Classic.


pkg/kv/kvserver/allocator_test.go, line 1491 at r3 (raw file):

	}

	replicas := func(ids ...roachpb.StoreID) (repls []roachpb.ReplicaDescriptor) {

An identical func is already in the global scope?


pkg/kv/kvserver/intent_resolver_integration_test.go, line 426 at r15 (raw file):

Previously, stevendanna (Steven Danna) wrote…

This is my least favourite change.

Agreed, this sucks. Can you make ctx a parameter to testTxnExecute instead?


pkg/sql/sqlstats/persistedsqlstats/scheduled_sql_stats_compaction_test.go, line 206 at r3 (raw file):

			testutils.SucceedsSoon(t, func() error {
				// Reload schedule from DB.
				sj = getSQLStatsCompactionSchedule(t, helper)

sad sad sad, this is not an improvement, similar to the case earlier. In this particular case though, does there have to be a sj in the parent-parent-...-parent scope?


pkg/storage/pebble_file_registry_test.go, line 93 at r3 (raw file):

	require.NoError(t, mem.MkdirAll("/mydb", 0755))
	registry := &PebbleFileRegistry{FS: mem, DBDir: "/mydb"}

You could also declare this below checkEquality, right?

@RaduBerinde
Copy link
Member


pkg/ccl/changefeedccl/changefeed_test.go, line 4286 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This is strictly worse though? If this is what the linter wants us to do, I think the solution is to turn off the linter?

We should just use a different name for the local variable (it's less confusing).

Copy link
Collaborator Author

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbg Thanks for the review. Some of the shadowing fixes are ugly and I think they are worth fixing up before merge (our linters shouldn't make the code worse!). Thanks for not letting them slide through :D.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @RaduBerinde, @stevendanna, and @tbg)


pkg/ccl/changefeedccl/changefeed_test.go, line 4286 at r3 (raw file):

Previously, RaduBerinde wrote…

We should just use a different name for the local variable (it's less confusing).

Uff, thanks for flagging this. Not sure what I was thinking, let's use a different name for the local scope.


pkg/kv/kvserver/allocator_test.go, line 1491 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

An identical func is already in the global scope?

Yup. Technically not exactly identical, but for the purpose of this test I believe the func in global scope accomplishes the same goal.


pkg/sql/sqlstats/persistedsqlstats/scheduled_sql_stats_compaction_test.go, line 206 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

sad sad sad, this is not an improvement, similar to the case earlier. In this particular case though, does there have to be a sj in the parent-parent-...-parent scope?

👍 I'll clean up the other test cases to not require it in global scope.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The previously constructed extended regular expression would end up
looking like:

    (pkg/*_test.go)|(pkg/workload)|...|(pkg/testutil):.*Use of go keyword not allowed

As a result, roachvet was being skipped on all test files and all but
the last package in this list.

This fixes the filter and addresses the violations that had snuck into
the code:

- *: fix unnecessary conversion lint violations
- *: fix composite literal uses unkeyed fields violations
- *: fix 'format argument is not a constant expression' violations
- *: fix 'message argument is not a constant expression' violations
- *: fix 'invalid direct cast on error object' violations
- *: fix 'use errors.Is instead of a direct comparison' violations
- changefeedccl: fix returnerrcheck violations
- *: fix shadowing violations
- *: fix missing calls to context.Cancel
- kv/kvserver: set timer.Read = true
- *: fix various format string mistakes
- *: fix grpcconnclose violations
- kvtenantccl: fix unreachable code violation

Release note: None
@stevendanna
Copy link
Collaborator Author

bors r=tbg

@craig
Copy link
Contributor

craig bot commented Sep 17, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 17, 2021

Build succeeded:

@craig craig bot merged commit 475d707 into cockroachdb:master Sep 17, 2021
@RaduBerinde
Copy link
Member

This will create conflicts in various backports that will require manual fixing. Let's consider backporting this to release-21.2.

@rafiss
Copy link
Collaborator

rafiss commented Sep 21, 2021

Hm, so does the case need to be made to the backport approval email thread?

@RaduBerinde
Copy link
Member

Hm, so does the case need to be made to the backport approval email thread?

It would be a test-only change, if folks agree it's a good idea I don't think it would be a problem.

@stevendanna
Copy link
Collaborator Author

Sorry about any backporting troubles this has caused. I've opened a backport here for discussion: #70561

@andreimatei
Copy link
Contributor

nit: in the future, try to have the merge message be the same as the commit message for single-commit PRs

- *: fix 'invalid direct cast on error object' violations
- *: fix 'use errors.Is instead of a direct comparison' violations

I personally consider these rules to be a bad idea for tests. Have you considered excluding tests from the rules instead?

@stevendanna
Copy link
Collaborator Author

nit: in the future, try to have the merge message be the same as the commit message for single-commit PRs

Uff, thanks for pointing this out, sorry about that.

I personally consider these rules to be a bad idea for tests. Have you considered excluding tests from the rules instead?

I hadn't, but only because there were relatively few violations to fix. I wouldn't be opposed to skipping those in tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testutils: test files inadvertently excluded from roachvet
6 participants