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.

Sign up for GitHub

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

Jump to bottom

Add integration tests for query parameters #1235

Open

sylwiaszunejko wants to merge 8 commits into scylladb:branch-hackathon from sylwiaszunejko:hackathon_test_query

Contributor

sylwiaszunejko commented Feb 12, 2025

Moved some tests from different files and added new ones inspired by java-driver 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.


          Remove duplicate test

f1b259d

sylwiaszunejko requested review from muzarski and Lorak-mmk

February 12, 2025 13:47

sylwiaszunejko self-assigned this

github-actions bot commented Feb 12, 2025 •

edited

Loading

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

sylwiaszunejko force-pushed the hackathon_test_query branch from fa6fdeb to cbcdfac Compare

February 12, 2025 14:10

Lorak-mmk closed this

Lorak-mmk reopened this

sylwiaszunejko force-pushed the hackathon_test_query branch from cbcdfac to d487ff5 Compare

February 12, 2025 14:29

muzarski reviewed

View reviewed changes

Contributor

muzarski left a comment

Looks good. I'm thinking if query.rs is a good name for the module (we are trying to rename the Query struct before the 1.0.0 - most probably it will be renamed to Statement). I think it should be maybe called statement.rs? These tests indeed verify correctness binding values to the statement, and some statement config parameters (e.g. timestamps)

scylla/tests/integration/query.rs Outdated

Comment on lines 53 to 80

+                  regular_query.set_timestamp(Some(420));
+                  session
+                      .query_iter(
+                          regular_query.clone(),
+                          ("regular query iter", "higher timestamp"),
+                      )
+                      .await
+                      .unwrap();

Contributor

muzarski Feb 12, 2025

I'm actually shocked that it works. The usual use case for [query/execute]_iter is to use them with SELECT statements (here we use INSERT). I was almost sure that we throw an error in case response is not RESULT:Rows. But then I saw this in pager.rs:

            Ok(NonErrorQueryResponse {
                response: NonErrorResponse::Result(_),
                tracing_id,
                ..
            }) => {
                // We have most probably sent a modification statement (e.g. INSERT or UPDATE),
                // so let's return an empty iterator as suggested in #631.

                // We must attempt to send something because the iterator expects it.
                let (proof, _) = self.sender.send_empty_page(tracing_id).await;
                Ok(ControlFlow::Break(proof))
            }

In such case, it's nice you extended the test cases by query/execute_iter scenarios.

scylla/tests/integration/query.rs Outdated

Comment on lines 444 to 445

		async fn test_deserialize_empty_collections() {
		// Setup session.

Contributor

muzarski Feb 12, 2025

I think this test does not belong here. It should be placed in deserialization test suite (the closest one is integration/cql_types.rs I believe) cc: @Lorak-mmk

Collaborator

Lorak-mmk Feb 12, 2025

I didn't yet read this PR nor #1232 , but please make sure that you are not repeating the same work

scylla/tests/integration/query.rs Outdated

Comment on lines 563 to 590

+                  let result = session
+                      .query_single_page(query.clone(), (KEY, 1), PagingState::start())
+                      .await;
+                  assert_matches!(
+                      result,
+                      Err(ExecutionError::LastAttemptError(
+                          RequestAttemptError::SerializationError(_)
+                      ))
+                  );
+                  let prepared_query = session.prepare(query).await.unwrap();
+                  let result = session.execute_unpaged(&prepared_query, (KEY, 1)).await;
+                  assert_matches!(
+                      result,
+                      Err(ExecutionError::BadQuery(BadQuery::SerializationError(_)))
+                  );
+                  let result = session.execute_iter(prepared_query.clone(), (KEY, 1)).await;
+                  assert_matches!(result, Err(PagerExecutionError::SerializationError(_)));
+                  let result = session
+                      .execute_single_page(&prepared_query, (KEY, 1), PagingState::start())
+                      .await;
+                  assert_matches!(
+                      result,
+                      Err(ExecutionError::BadQuery(BadQuery::SerializationError(_)))
+                  );

Contributor

muzarski Feb 12, 2025

I'm glad we have some test cases which kinda show the difference between ExecutionError::BadQuery::SerializationError and ExecutionError::LastAttemptError::SerializationError. I know that it must be confusing that SerializationError appears twice in errors hierarchy. This is something we will be trying to fix in the future. But the fix for this is non-trivial, and we won't be able to do that before 1.0.0,

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

sylwiaszunejko force-pushed the hackathon_test_query branch from d487ff5 to 965af53 Compare

February 13, 2025 09:05

Contributor Author

sylwiaszunejko commented Feb 13, 2025

I changed the file name as suggested, moved test_deserialize_empty_collections and made error matches more specific.

sylwiaszunejko requested a review from muzarski

February 13, 2025 09:08

Lorak-mmk requested changes

View reviewed changes

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

scylla/tests/integration/query.rs Outdated

Comment on lines 10 to 24

+              #[tokio::test]
+              async fn test_prepared_config() {
+                  setup_tracing();
+                  let session = create_new_session_builder().build().await.unwrap();
+                  let mut query = Query::new("SELECT * FROM system_schema.tables");
+                  query.set_is_idempotent(true);
+                  query.set_page_size(42);
+                  let prepared_statement = session.prepare(query).await.unwrap();
+                  assert!(prepared_statement.get_is_idempotent());
+                  assert_eq!(prepared_statement.get_page_size(), 42);
+              }

Collaborator

Lorak-mmk Feb 12, 2025

Apart from idempotency and page_size, could you also check:

consistency
serial consistency
tracing
request_timeout
And other parameters that should survive preparation (see the metods on PreparedStatement and Query)
?

Contributor Author

sylwiaszunejko Feb 13, 2025

I added additional checks, is there something more I should add?

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

wprzytula requested changes

View reviewed changes

scylla/tests/integration/statement.rs Outdated

Comment on lines 27 to 70

+                  let query_str = format!("INSERT INTO {}.t_timestamp (a, b) VALUES (?, ?)", ks);
+                  // test regular query timestamps
+                  let mut regular_query = Query::new(query_str.to_string());

Collaborator

wprzytula Feb 13, 2025

⛏️ Needless to_string() - format! already returns a String.

Contributor Author

sylwiaszunejko Feb 13, 2025

to_string() is still there, but there is no format! after I started using USE ks

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

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

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

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

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

scylla/tests/integration/statement.rs Show resolved Hide resolved

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

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

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

sylwiaszunejko added 3 commits

February 13, 2025 14:55


          Fix assert_matches! usage in tests/integration/session.rs

e2a08ab


          Move test_deserialize_empty_collections to cql_types.rs

c223d1a


          Move timestamp test to statement.rs and add missing cases

72b59c7

sylwiaszunejko force-pushed the hackathon_test_query branch 2 times, most recently from a6e85e7 to 5193daf Compare

February 13, 2025 14:38

sylwiaszunejko requested review from Lorak-mmk and wprzytula

February 13, 2025 14:44

Lorak-mmk approved these changes

View reviewed changes

muzarski approved these changes

View reviewed changes

Contributor

muzarski left a comment

Some commit titles still mention query.rs (which we decided to rename to statement.rs)

sylwiaszunejko added 4 commits

February 17, 2025 08:49


          Move test_prepared_config to statement.rs and add additional cases to it

d9af125


          Move bind markers and value list tests to statement.rs

d3cbf9d

Extra cases and some small fixes added.


          Add integration tests for named/unnamed bind markers

ba2c83f

Tests are inspired by https://github.com/scylladb/java-driver/blob/48a9ea0496e5fe6c75eb481789490731c8e33050/integration-tests/src/test/java/com/datastax/oss/driver/core/cql/SimpleStatementCcmIT.java


          Fix panic while generatic a timestamp

8c41770

If random::<i64>() produces i64::MIN, calling .abs()
on it causes an overflow.
This commit fixes that by using random::<u64>().

sylwiaszunejko force-pushed the hackathon_test_query branch from 5193daf to 8c41770 Compare

February 17, 2025 07:49

Contributor Author

sylwiaszunejko commented Feb 17, 2025

I changed the commit message to mention correct file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet