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

spanconfigsplitterccl: TestDataDriven is brittle (perhaps due to weakness in spanconfigtestcluster? unsure) #106818

Closed
knz opened this issue Jul 14, 2023 · 2 comments · Fixed by #107760
Assignees
Labels
A-multitenancy Related to multi-tenancy A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@knz
Copy link
Contributor

knz commented Jul 14, 2023

Informs #76378

Describe the problem

When TestDataDriven is run with a random test tenant server assigned, the test crashes with a nil pointer exception.

It's unclear what is happening.

To Reproduce

Replace the use of TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet by TestTenantAlwaysEnabled and run the test.

Expected behavior

From a test quality perspective, if the test code lands in an unsupported configuration it should fail with t.Fatal not with a crash.

Jira issue: CRDB-29728
Epic: CRDB-18499

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-testing Testing tools and infrastructure A-multitenancy Related to multi-tenancy labels Jul 14, 2023
@knz
Copy link
Contributor Author

knz commented Jul 14, 2023

Ditto in spanconfigsqltranslatorccl.

knz added a commit to knz/cockroach that referenced this issue Jul 14, 2023
There's a mix of tests that control their tenants directly,
and tests that should really work with virtualization
enabled but don't.

Followup issues: cockroachdb#106821 and cockroachdb#106818.

Release note: None
knz added a commit to knz/cockroach that referenced this issue Jul 14, 2023
There's a mix of tests that control their tenants directly,
and tests that should really work with virtualization
enabled but don't.

Followup issues: cockroachdb#106821 and cockroachdb#106818.

Release note: None
@knz knz added A-kv Anything in KV that doesn't belong in a more specific category. T-kv KV Team labels Jul 14, 2023
craig bot pushed a commit that referenced this issue Jul 14, 2023
106738: logic: skip_on_retry works when errors are expected r=Xiang-Gu a=Xiang-Gu

Previously, we have `skip_on_retry` directive for logic test which, when set, it skips the rest of test if a statement fails with TransactionRetryError. However, it won't skip if the statement is expected to fail with certain error message. This PR ensures that whenever we have a TransactionRetryError and `skip_on_retry` is set, we always skip the rest of the test, even if the stmt is expected to fail.

fixes #104464

Release note: None

106759: streamingccl: unskip TestStreamDeleteRange r=msbutler a=stevendanna

This test had previously timed out. The timeout we saw was the result of a couple of issues.

When waiting for all delete ranges, our loop exit condition was very strict. We would only stop looking for rows if the number of delete ranges was exactly 3. If, however, we got 4 delete ranges, with 2 coming in a single batch, we would never hit this condition.

How would that happen though? One possibility are rangefeed duplicates. Another, and what appears to have been happening in this test, is that the representation of the range deletes observed by the rangefeed consumer is slightly different depending on whether the range delete is delivered as part of a catch-up scan or as part of the rangefeeds steady state. I believe this is because the range deletes overlap but are issued at different time points.  When we get them as part of the steady state, we get a trimmed version of the original event. When we get them as part of the catch-up scan, we get them broke up at the point of overlap.

Fixes #93568

Epic: none

Release note: None

106814: testutils: add helper to target transactions for retries r=lidorcarmel a=stevendanna

This helper makes it a little quicker to write a test that tests whether a particular transaction is retry safe.

Informs #106417

Epic: none

Release note: none

106822: spanconfigccl: remove uses of `TODOTestTenantDisabled` r=stevendanna a=knz

Informs #76378 .
Epic: CRDB-18499

There's a mix of tests that control their tenants directly, and tests that should really work with virtualization enabled but don't.

Followup issues: #106821 and #106818.

Release note: None

106832: server: bark loudly if the test tenant cannot be created r=herkolategan a=knz

Informs #76378 
Informs #103772. 
Epic: CRDB-18499

For context, the automatic test tenant machinery is currently dependent on a CCL enterprise license check.
(This is fundamentally not necessary - see #103772 - but sadly this is the way it is for now)

Prior to this patch, if the user or a test selected the creation of a test tenant, but the test code forgot to import the required CCL go package, the framework would announce that "a test tenant was created" but it was actually silently failing to do so.

This led to confusing investigations where a test tenant was expected, a test was appearing to succeed, but with a release build the same condition would fail.

This commit enhances the situation by ensuring we have clear logging output when the test tenant cannot be created due to the missing CCL import.

Release note: None

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
msbutler pushed a commit to msbutler/cockroach that referenced this issue Jul 17, 2023
There's a mix of tests that control their tenants directly,
and tests that should really work with virtualization
enabled but don't.

Followup issues: cockroachdb#106821 and cockroachdb#106818.

Release note: None
@andrewbaptist andrewbaptist added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed A-kv Anything in KV that doesn't belong in a more specific category. T-kv KV Team labels Jul 20, 2023
@rafiss
Copy link
Collaborator

rafiss commented Jul 21, 2023

@andrewbaptist I noticed you moved this to the SQL Foundations team. Could you provide a few more details? I don't think our team has worked on this test before, but if the error is pointing towards something in the SQL Foundations area we can take a look.

@rafiss rafiss added C-test-failure Broken test (automatically or manually discovered). db-cy-23 labels Jul 25, 2023
@rafiss rafiss self-assigned this Jul 25, 2023
@rafiss rafiss removed the C-test-failure Broken test (automatically or manually discovered). label Jul 25, 2023
@craig craig bot closed this as completed in 167da65 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. db-cy-23 T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants