sql: increase tenant testing coverage #142391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
rafiss
requested changes
Mar 13, 2025
There was a problem hiding this comment.
thank you! i had one nit.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/mvcc_backfiller_test.go line 597 at r4 (raw file):
defer s.Stopper().Stop(context.Background()) codec := s.ApplicationLayer().Codec() scratch = append(s.Codec().TenantPrefix(), roachpb.Key("scratch")...)
i don't think roachpb.Key("scratch") is correct here. does this work:
var err error
scratch, err = s.ScratchRange()
require.NoError(t, err)
scratch = append(s.Codec().TenantPrefix(), scratch...)
shubhamdhama
force-pushed
the
sql-tenant-testing-returns
branch
from
c370cdd to
bf124c9
Compare
|
Previously, rafiss (Rafi Shamim) wrote…
Thanks for pointing this out. The suggested approach seems much better. |
There was a problem hiding this comment.
Pull Request Overview
This PR increases tenant testing coverage for SQL components by switching test server parameter creation to allow tenants in several tests. Key changes include:
- Replacing createTestServerParams with createTestServerParamsAllowTenants in multiple test files, ensuring tenant support.
- Adjusting test expectations based on the server’s deployment mode (external vs. shared-process).
- Introducing new helper functions in test_server_args.go to manage external process mode behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sql/jobs_profiler_execution_details_test.go | Updates tests for job profiler execution details to use tenant-enabled server parameters and conditionally check outputs based on deployment mode. |
| pkg/base/test_server_args.go | Adds functions to flag tests as skipped or non-functional in external process mode for performance reasons. |
| pkg/sql/txn_restart_test.go | Updates various txn restart tests to use tenant-enabled parameters and modernizes PG URL generation approach. |
| pkg/sql/unsplit_range_test.go | Adjusts unsplit range test configuration to run with tenant-enabled defaults and skips external mode tests where applicable. |
| pkg/sql/mvcc_backfiller_test.go | Enables tenant support in MVCC backfiller tests and prefixes the scratch range with a tenant prefix to simulate multi-tenant behavior. |
rafiss
approved these changes
Mar 13, 2025
There was a problem hiding this comment.
lgtm! i did not review pkg/sql/jobs_profiler_execution_details_test.go
Reviewable status:
I opened issue cockroachdb#142388 to track the work for enabling external-process mode testing, as it turned out to be a non-trivial effort.
shubhamdhama
force-pushed
the
sql-tenant-testing-returns
branch
from
bf124c9 to
7c40dd2
Compare
msbutler
reviewed
Mar 18, 2025
msbutler
approved these changes
Mar 18, 2025
|
TFTRs! bors r=rafiss,msbutler |
shubhamdhama
added a commit
to shubhamdhama/cockroach
that referenced
this pull request
Mar 19, 2025
In cockroachdb#142391, a bug was introduced in the test where `testutils.SucceedSoon`'s closure used `require.Len` instead of returning an error, causing the test to fail if slice length doesn't match without retrying. This assertion has been reverted to the `if` statement that returns an error instead. Fixes: cockroachdb#143082 Release note: none Epic: none
craig bot
pushed a commit
that referenced
this pull request
Mar 19, 2025
143113: sql: fix TestListProfilerExecutionDetails r=yuzefovich a=shubhamdhama In #142391, a bug was introduced in the test where `testutils.SucceedSoon`'s closure used `require.Len` instead of returning an error, causing the test to fail if slice length doesn't match without retrying. This assertion has been reverted to the `if` statement that returns an error instead. Fixes: #143082 Release note: none Epic: none Co-authored-by: Shubham Dhama <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sql: increase tenant testing coverage for `jobs_profiler_execution_details_test`sql: increase tenant testing coverage for `mvcc_backfiller_test`sql: increase tenant testing coverage for `txn_restart_test`sql: enable shared-process mode tenant testing for `TestUnsplitRanges`I opened issue #142388 to track the work for enabling external-process mode
testing, as it turned out to be a non-trivial effort.
Informs: #140446
Epic: CRDB-48564
Release note: None