tests: add integration tests for batch statements #1226

dimakr · 2025-02-11T14:19:22Z

Test cases/ideas are inspired by Java driver batch statements tests.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

github-actions · 2025-02-11T14:26:05Z

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: fc9def4

wprzytula · 2025-02-11T16:14:30Z

I intended to open a PR instead of pushing to your branch directly, sorry @dimakr...

scylla/tests/integration/main.rs

scylla/tests/integration/batch_statements.rs

dimakr · 2025-02-11T22:14:56Z

@wprzytula I've made the changes as per the comments above

scylla/tests/integration/batch.rs

dimakr · 2025-02-12T22:56:46Z

Turned out that we have a few batch tests in scylla/tests/integration/session.rs test suite.
Moved them to the dedicated batch.rs test suite, for which we are creating new test cases in this PR.

scylla/tests/common/utils.rs

scylla/tests/integration/batch_statements.rs

Lorak-mmk · 2025-02-13T09:46:51Z

scylla/tests/integration/batch_statements.rs

+/// TODO: Remove #[ignore] once LWTs are supported with tablets.
+#[tokio::test]
+#[ignore]
+async fn test_cas_batch() {
+    setup_tracing();
+    let test_name = String::from("test_cas_batch");


I guess you got the inspiration from test_lwt_optimization_works_with_tablets. The difference is that this test does not depend on tablets, while test_lwt_optimization_works_with_tablets does.
Please disable tablets explicitly during keyspace creation. This will allow the test to actually run.

scylla/tests/integration/batch_statements.rs

scylla/tests/common/utils.rs

scylla/tests/integration/batch.rs

wprzytula · 2025-02-13T13:43:28Z

scylla/tests/integration/batch.rs

+    // Scylla returns 4 columns, but Cassandra returns only 1
+    let is_scylla: bool = batch_deserializer.column_specs().len() == 4;
+
+    if is_scylla {
+        test_batch_lwts_for_scylla(&session, &batch, &batch_deserializer, &test_name).await;
+    } else {
+        test_batch_lwts_for_cassandra(&session, &batch, &batch_deserializer, &test_name).await;
+    }


🔧 Please provide links to documentation explaining this difference.

dimakr · 2025-02-13T15:09:57Z

@wprzytula @Lorak-mmk addressed the last comments.

Lorak-mmk · 2025-02-13T22:32:40Z

@wprzytula @Lorak-mmk addressed the last comments.

In commit "tests: add integration tests for batch statements" some tests are still marked with #[ignore] instead of disabling tablets. Maybe you forgot to fix that?

Lorak-mmk · 2025-02-13T22:19:38Z

scylla/tests/common/utils.rs

        SystemTime::now()
            .duration_since(UNIX_EPOCH)
            .unwrap()
-            .as_secs(),
+            .as_micros(),


I don't see why this change is necessary.
Notice that the keyspace name also utilizes the global counter.
The only way I see to acutally get a collision is to have 2 copies of the repository, run the same test in both, and be extremely unlucky for them to clash both on the counter and the timestamp. This seems extremely unlikely.

This is how the batch tests fail. if as_sec() is used to generate suffix for KS name:

──── STDERR: scylla::integration batch::test_batch_lwts thread 'batch::test_batch_lwts' panicked at scylla/tests/integration/batch.rs:506:76: called `Result::unwrap()` on an `Err` value: LastAttemptError(DbError(Invalid, "Cannot use LightWeight Transactions for table test_rust_1739527888_0.test_counter_batch: LWT is not yet supported with tablets")) ... ──── STDERR: scylla::integration batch::test_fail_counter_batch_with_non_counter_increment thread 'batch::test_fail_counter_batch_with_non_counter_increment' panicked at scylla/tests/integration/batch.rs:54:14: called `Result::unwrap()` on an `Err` value: LastAttemptError(DbError(Invalid, "Cannot use the 'counter' type for table test_rust_1739527888_0.counter1: Counters are not yet supported with tablets")) ...

The batch test suite uses the common helper to create a KS and table. Then when each test is executed and calls the helper it would try to get its own, unique, KS name. But tests in this test suite are executed within 1 second so the unique_keyspace_name cannot guarantee unique value. And in this particular test suite part of tests require KS with tablets disabled, so the existing KS cannot be reused.

Probably for the affected tests here some other approach could be used, i.e. do not useunique_keyspace_name, but some dedicated KS name generation logic. But I thought that the symptom might pop up in other tests as well, and this change would help.

But tests in this test suite are executed within 1 second so the unique_keyspace_name cannot guarantee unique value.

Please the the source code of unique_keyspace_name. It uses an atomic counter to guarantee uniqueness.

──── STDERR: scylla::integration batch::test_batch_lwts thread 'batch::test_batch_lwts' panicked at scylla/tests/integration/batch.rs:506:76: called `Result::unwrap()` on an `Err` value: LastAttemptError(DbError(Invalid, "Cannot use LightWeight Transactions for table test_rust_1739527888_0.test_counter_batch: LWT is not yet supported with tablets")) ... ──── STDERR: scylla::integration batch::test_fail_counter_batch_with_non_counter_increment thread 'batch::test_fail_counter_batch_with_non_counter_increment' panicked at scylla/tests/integration/batch.rs:54:14: called `Result::unwrap()` on an `Err` value: LastAttemptError(DbError(Invalid, "Cannot use the 'counter' type for table test_rust_1739527888_0.counter1: Counters are not yet supported with tablets")) ...

Weird, the counter is 0 for both of them. How are you running the tests?

It fails when specific module is executed, e.g.:

> cargo nextest r -E 'test(/^batch::/)'

But it is ok when all tests are executed at once (make test, etc.). So I'll drop that change.

cargo nextest is not something we ever tested, nor recommended in CONTRIBUTING.md.
It uses process-per-test architecture, making a counter that guarantees uniqueness useless - because each process, and thus test, will have its own copy of it, thus always using 0.

dimakr · 2025-02-14T10:46:14Z

In commit "tests: add integration tests for batch statements" some tests are still marked with #[ignore] instead of disabling tablets. Maybe you forgot to fix that?

@Lorak-mmk the tablets disabling was done as part of "tests: move batch statements tests to batch.rs module" commit. I've adjusted that commit message to clearly mention this.

Lorak-mmk · 2025-02-14T11:10:01Z

@Lorak-mmk the tablets disabling was done as part of "tests: move batch statements tests to batch.rs module" commit. I've adjusted that commit message to clearly mention this.

Is there a reason for doing it in the commit that moves tests, instead of doing it that way from the beginning?

dimakr · 2025-02-14T12:00:34Z

Is there a reason for doing it in the commit that moves tests, instead of doing it that way from the beginning?

There was a bit of mess in the series of commits in this PR from the beginning after a commit from another person was accidentally pushed in (that one is squashed eventually). But to avoid further issues I've made 'disable tablets'-change in the subsequent commit.

Lorak-mmk · 2025-02-14T12:50:36Z

Two things:

I don't think you fixed all tests in regards to tablets. In the "Files changed view" I see that test_cas_batch, test_counter_batch, test_fail_logged_batch_with_counter_increment, test_fail_counter_batch_with_non_counter_increment in batch_statement.rs are still ignored.
I see that after this PR we have 2 files: batch.rs and batch_statement.rs. Why? What future tests should go into the latter, and what into the former?

dimakr · 2025-02-14T13:29:32Z

Two things:

It was OK, but due to this number of nit related changes one of recent rebases went wrong. Will correct it

New batch statement test cases are moved to exising batch.rs test suite. Additionally, the test cases for LWT and counter tables are unskipped and adjusted to use keyspace with tablets disabled. This allows executing the underlying test scenarios even though LWT and counters are not yet supported with tablets.

dimakr · 2025-02-14T16:13:41Z

Two things:

It was OK, but due to this number of nit related changes one of recent rebases went wrong. Will correct it

@Lorak-mmk This time should be ok - got rid of batch_statement.rs as per comment #1226 (comment)

dimakr force-pushed the session_batch_tests branch from e8fb95f to b5b6185 Compare February 11, 2025 14:27

dimakr requested review from Lorak-mmk and wprzytula February 11, 2025 14:43

dimakr marked this pull request as ready for review February 11, 2025 15:17

wprzytula reviewed Feb 11, 2025

View reviewed changes

scylla/tests/integration/main.rs Outdated Show resolved Hide resolved

scylla/tests/integration/batch_statements.rs Outdated Show resolved Hide resolved

scylla/tests/integration/batch_statements.rs Outdated Show resolved Hide resolved

dimakr force-pushed the session_batch_tests branch from ca37183 to 947c483 Compare February 11, 2025 21:42

dimakr requested a review from wprzytula February 12, 2025 09:47

wprzytula reviewed Feb 12, 2025

View reviewed changes

scylla/tests/integration/batch.rs Outdated Show resolved Hide resolved

dimakr force-pushed the session_batch_tests branch from 947c483 to bd00e4b Compare February 12, 2025 17:12

dimakr requested a review from wprzytula February 12, 2025 22:56

dimakr self-assigned this Feb 12, 2025

wprzytula approved these changes Feb 13, 2025

View reviewed changes

scylla/tests/common/utils.rs Outdated Show resolved Hide resolved

Lorak-mmk requested changes Feb 13, 2025

View reviewed changes

tests: add integration tests for batch statements

0493ac5

dimakr force-pushed the session_batch_tests branch from 6ec6b38 to c53c53b Compare February 13, 2025 12:08

wprzytula requested changes Feb 13, 2025

View reviewed changes

dimakr force-pushed the session_batch_tests branch from c53c53b to 2c7d920 Compare February 13, 2025 14:52

dimakr requested review from wprzytula and Lorak-mmk February 13, 2025 15:10

Lorak-mmk reviewed Feb 13, 2025

View reviewed changes

dimakr force-pushed the session_batch_tests branch from 2c7d920 to 6974ed0 Compare February 14, 2025 10:42

dimakr requested a review from Lorak-mmk February 14, 2025 10:46

dimakr force-pushed the session_batch_tests branch from 6974ed0 to 650ec58 Compare February 14, 2025 11:48

dimakr added 2 commits February 14, 2025 16:40

tests: move batch tests from session.rs to batch.rs module

fc9def4

dimakr force-pushed the session_batch_tests branch from 650ec58 to fc9def4 Compare February 14, 2025 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add integration tests for batch statements #1226

tests: add integration tests for batch statements #1226

dimakr commented Feb 11, 2025

github-actions bot commented Feb 11, 2025 •

edited

Loading

wprzytula commented Feb 11, 2025

dimakr commented Feb 11, 2025

dimakr commented Feb 12, 2025

Lorak-mmk Feb 13, 2025

wprzytula Feb 13, 2025

dimakr commented Feb 13, 2025 •

edited

Loading

Lorak-mmk commented Feb 13, 2025

Lorak-mmk Feb 13, 2025

dimakr Feb 14, 2025

dimakr Feb 14, 2025

Lorak-mmk Feb 14, 2025

Lorak-mmk Feb 14, 2025

dimakr Feb 14, 2025

Lorak-mmk Feb 14, 2025 •

edited

Loading

dimakr commented Feb 14, 2025

Lorak-mmk commented Feb 14, 2025 •

edited

Loading

dimakr commented Feb 14, 2025 •

edited

Loading

Lorak-mmk commented Feb 14, 2025 •

edited

Loading

dimakr commented Feb 14, 2025

dimakr commented Feb 14, 2025

tests: add integration tests for batch statements #1226

Are you sure you want to change the base?

tests: add integration tests for batch statements #1226

Conversation

dimakr commented Feb 11, 2025

Pre-review checklist

github-actions bot commented Feb 11, 2025 • edited Loading

wprzytula commented Feb 11, 2025

dimakr commented Feb 11, 2025

dimakr commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimakr commented Feb 13, 2025 • edited Loading

Lorak-mmk commented Feb 13, 2025

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

Lorak-mmk Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

dimakr commented Feb 14, 2025

Lorak-mmk commented Feb 14, 2025 • edited Loading

dimakr commented Feb 14, 2025 • edited Loading

Lorak-mmk commented Feb 14, 2025 • edited Loading

dimakr commented Feb 14, 2025

dimakr commented Feb 14, 2025

github-actions bot commented Feb 11, 2025 •

edited

Loading

dimakr commented Feb 13, 2025 •

edited

Loading

Lorak-mmk Feb 14, 2025 •

edited

Loading

Lorak-mmk commented Feb 14, 2025 •

edited

Loading

dimakr commented Feb 14, 2025 •

edited

Loading

Lorak-mmk commented Feb 14, 2025 •

edited

Loading