tests: serialization-deserialization tests #1232

k0machi · 2025-02-12T12:34:51Z

This commit refactors and adds additional tests related to serialization
inside integration tests. It adds coverage for most scalar types,
additional cases for more complex types such as Uuid and Text, Null
checking inside queries and also splits certain test cases into multiple
tests to separate expected failures from expected successes.

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-12T12:43:00Z

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

Lorak-mmk · 2025-02-13T09:25:21Z

I see there is a conflict, could you rebase on curent branch-hackathon to fix it?

k0machi · 2025-02-13T10:51:30Z

I see there is a conflict, could you rebase on curent branch-hackathon to fix it?

Done, there should be 3 tests that are currently failing, I'm going to push a fix to handle them properly - they fail on bad values, so I will either split them into separate test or just add an error handler callback to the test function.

wprzytula

🔧 It's difficult to follow all the changes, as they are all introduced in one commit. Could you please split them into at least the following commits:

Rename one of {cql_types.rs,cql_value.rs} to serialization.rs.
Move contents of the remaining one of {cql_types.rs,cql_value.rs} to serialization.rs.
Change the existing tests, if needed.
Add your own new tests, if any.

k0machi · 2025-02-13T12:13:03Z

🔧 It's difficult to follow all the changes, as they are all introduced in one commit. Could you please split them into at least the following commits:

Rename one of {cql_types.rs,cql_value.rs} to serialization.rs.

Move contents of the remaining one of {cql_types.rs,cql_value.rs} to serialization.rs.

Change the existing tests, if needed.

Add your own new tests, if any.

Done.

wprzytula

👍 Thanks for the split, commits 1, 2, 4 are now very readable.

🔧 The third commit (refactor and split old serialization tests) is still very large and the displayed diff is complicated. Could you split it further, preferably into separate logical steps of changes, so that it's easier to follow?

wprzytula · 2025-02-13T13:54:11Z

scylla/tests/integration/serialization.rs

+async fn run_literal_and_bound_value_test_with_callback<LitVal, BoundVal, ErrHandler>(
+    table_name: &str,
+    type_name: &str,
+    values: Vec<(LitVal, BoundVal)>,


🔧 A slice would suffice here.

Suggested change

values: Vec<(LitVal, BoundVal)>,

values: &[(LitVal, BoundVal)],

Then, you could pass stack-allocated slices to this function and elide allocations. Not so important in tests, but useful in general.

wprzytula · 2025-02-13T13:55:34Z

scylla/tests/integration/serialization.rs

+async fn run_literal_input_maybe_empty_output_test<X>(
+    table_name: &str,
+    type_name: &str,
+    values: Vec<(Option<&str>, Option<MaybeEmpty<X>>)>,


🔧 ditto: non-owned slice is enough.

wprzytula · 2025-02-13T13:56:51Z

scylla/tests/integration/serialization.rs

+            .filter(|row| match row {
+                Ok((_,)) => true,
+                Err(e) => handle_deserialize_error(e, || false),
+            })
            .map(Result::unwrap)


filter_map could help avoid unwrap here.

wprzytula · 2025-02-13T13:58:44Z

scylla/tests/integration/serialization.rs

+#[cfg(any(
+    feature = "num-bigint-03",
+    feature = "num-bigint-04",
+    feature = "bigdecimal-04"
+))]
+fn bigint_from_str<T>(val: &str) -> T
+where
+    T: FromStr,
+    <T as FromStr>::Err: Debug,
+{
+    T::from_str(val).expect("Failed to parse BigInt")
+}


This function has nothing to do with bigint - it justs calls the <T as FromStr>::from_str() implementation. It's thus not a good generic use IMO.

k0machi · 2025-02-13T15:51:42Z

🔧 The third commit (refactor and split old serialization tests) is still very large and the displayed diff is complicated. Could you split it further, preferably into separate logical steps of changes, so that it's easier to follow?

I don't think further splitting is possible - I started by replacing old test functions with new ones and then went and cleaned bodies of most of the tests, alongside extracting some bits into separate tests - I guess I could separate the extraction part - I commented them out first, but other than that I think I will end up with non-compiling commits.

Lorak-mmk

Just posting a few comments because I can't review the whole thing now, I have to go to sleep

Lorak-mmk · 2025-02-13T21:40:11Z

scylla/tests/integration/serialization.rs

+use core::str;
+use std::{
+    fmt::{Debug, Display},
+    net::{IpAddr, Ipv4Addr, Ipv6Addr},
+    str::FromStr,
+    vec,
+};


Please group imports by module. There is some options in rust-analyzer to do that, or you can do that manually.
By module I mean, in this case:

use std::fmt::{Debug, Display}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::str::FromStr; use std::vec;

Grouping more than by module makes it much more difficult to do certain refactors, and introduces a lot of unnecessary conflicts when rebasing.
Not grouping at all results in a lot of code. Grouping by module is a good compromise.

Here is my config for rust-analyzer:

"rust-analyzer.imports.merge.glob": false, "rust-analyzer.imports.granularity.group": "module", "rust-analyzer.imports.granularity.enforce": true,

I'm not sure if it will fix the existing imports - you probably will need to fix those manually. But all new automatic imports (ctrl + .) will be grouped properly

@muzarski Thanks, added to my config.

Done

Lorak-mmk · 2025-02-13T21:42:55Z

scylla/tests/integration/serialization.rs

+use scylla::{
+    client::session::Session,
+    deserialize::{
+        row::BuiltinDeserializationError,
+        value::{Emptiable, MaybeEmpty},
+    },
+    errors::DeserializationError,
+    serialize::value::SerializeValue,
+    value::{
+        Counter, CqlDate, CqlDuration, CqlTime, CqlTimestamp, CqlTimeuuid, CqlValue, CqlVarint,
+    },
+    DeserializeValue, SerializeValue,
+};


Ditto, don't group by more than module.

Lorak-mmk · 2025-02-13T21:46:48Z

scylla/tests/integration/serialization.rs

+fn handle_deserialize_error<F, T>(error: &DeserializationError, callback: F) -> T
+where
+    F: FnOnce() -> T,
+{
+    match &error.downcast_ref::<BuiltinDeserializationError>().unwrap().kind{
+        scylla::deserialize::row::BuiltinDeserializationErrorKind::ColumnDeserializationFailed { err, .. } => match err.downcast_ref::<scylla::deserialize::value::BuiltinDeserializationError>().unwrap().kind {
+            scylla::deserialize::value::BuiltinDeserializationErrorKind::ExpectedNonNull => {
+                callback()
+            },
+            _ => panic!("Unexpected error: {:?}", error),
+        }
+        _ => panic!("Unexpected error: {:?}", error),
+    }


It looks like you are always calling this function with callback set to || None. In that case I don't see why the callback is useful, please remove it.
After removing it, a much better name for this function would be assert_expected_non_null_error.

Indeed, that callback was useful when I initially wrote .filter().map() chain, so in some cases I needed T as the return value of the callback and in others bool, but I think this can be removed now.

Lorak-mmk · 2025-02-13T21:51:39Z

scylla/tests/integration/serialization.rs

+#[cfg(any(feature = "chrono-04", feature = "time-03",))]
+fn handle_incorrect_dates<T>(error: &DeserializationError) -> Option<T> {
+    match &error.downcast_ref::<BuiltinDeserializationError>().unwrap().kind {
+            scylla::deserialize::row::BuiltinDeserializationErrorKind::ColumnDeserializationFailed { err, .. } => match err.downcast_ref::<scylla::deserialize::value::BuiltinDeserializationError>().unwrap().kind {
+                scylla::deserialize::value::BuiltinDeserializationErrorKind::ValueOverflow => {
+                    None
+                },
+                scylla::deserialize::value::BuiltinDeserializationErrorKind::ExpectedNonNull => {
+                    None
+                },
+                _ => panic!("Unexpected error: {:?}", error),
+            }
+            _ => panic!("Unexpected error: {:?}", error),
+    }
+}


This function either panics or returns None. In that case there is no reason to return anything.
Make it just panic or not and rename it to assert_overflow_or_expected_non_null_error - or another name if you can think of some.

I use this function inside a filter_map - wouldn't this break the functionality of that?

This function always returns None.
So you can change it just as I suggested, and use None literal instead of its return value

Oh right, I see, okay, I'll do that.

Lorak-mmk · 2025-02-13T22:07:48Z

scylla/tests/integration/serialization.rs

 // Creates a new keyspace
 // Drops and creates table {table_name} (id int PRIMARY KEY, val {type_name})
 async fn init_test(table_name: &str, type_name: &str) -> Session {
-    init_test_maybe_without_tablets(table_name, type_name, true).await
+    prepare_test_table(table_name, type_name, true).await
+}


Is there a reason for this function to exist? Its name doesn't say anything about tablets, and the only difference between this one and prepare_test_table is that this one always creates the ks and table with tablet support.
Maybe just remove it and call prepare_test_table directly?

This function was there previously and I initially started without it but then ported it back over when wholesale copying some of the tests - I'll just remove it, it was there to set tablets to true for pretty much every test except counter one.

Generalized most tests and split some of them into distinct tests to see the results easier

This commit adds additional test to test various simple types such as int and float, as well as one more uuid test and some simple string type tests

k0machi requested review from muzarski and Lorak-mmk February 12, 2025 12:35

k0machi force-pushed the serialization-tests branch 2 times, most recently from 2c3e6d6 to 22071b4 Compare February 12, 2025 12:37

k0machi force-pushed the serialization-tests branch from 22071b4 to 93ca668 Compare February 12, 2025 14:45

Lorak-mmk mentioned this pull request Feb 12, 2025

Add integration tests for query parameters #1235

Open

8 tasks

k0machi force-pushed the serialization-tests branch 2 times, most recently from 35ebefe to ab61b5d Compare February 13, 2025 10:47

k0machi force-pushed the serialization-tests branch 2 times, most recently from 6dacbfd to 1d3719d Compare February 13, 2025 11:39

wprzytula requested changes Feb 13, 2025

View reviewed changes

k0machi force-pushed the serialization-tests branch 3 times, most recently from abce619 to 09db999 Compare February 13, 2025 12:10

k0machi requested a review from wprzytula February 13, 2025 12:13

wprzytula requested changes Feb 13, 2025

View reviewed changes

k0machi force-pushed the serialization-tests branch 3 times, most recently from feb9649 to 99817b8 Compare February 13, 2025 15:47

k0machi requested a review from wprzytula February 13, 2025 15:51

Lorak-mmk requested changes Feb 13, 2025

View reviewed changes

k0machi added 2 commits February 14, 2025 17:05

tests: rename cql_types.rs -> serialization.rs

c98d6aa

tests: move cql_value.rs tests into serialization.rs

92b0c7f

k0machi force-pushed the serialization-tests branch from 99817b8 to 24dda73 Compare February 14, 2025 16:27

k0machi requested a review from Lorak-mmk February 14, 2025 16:29

k0machi added 2 commits February 14, 2025 17:30

tests: refactor and split old serialization tests

af38b24

Generalized most tests and split some of them into distinct tests to see the results easier

tests: Add more scalar type tests, normal uuid and string type tests

8b93622

This commit adds additional test to test various simple types such as int and float, as well as one more uuid test and some simple string type tests

k0machi force-pushed the serialization-tests branch from 24dda73 to 8b93622 Compare February 14, 2025 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: serialization-deserialization tests #1232

tests: serialization-deserialization tests #1232

k0machi commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

Lorak-mmk commented Feb 13, 2025

k0machi commented Feb 13, 2025

wprzytula left a comment

k0machi commented Feb 13, 2025

wprzytula left a comment

wprzytula Feb 13, 2025

wprzytula Feb 13, 2025

wprzytula Feb 13, 2025

k0machi Feb 13, 2025

wprzytula Feb 13, 2025

k0machi Feb 13, 2025

wprzytula Feb 13, 2025

k0machi Feb 13, 2025

wprzytula Feb 13, 2025

k0machi Feb 13, 2025

k0machi commented Feb 13, 2025

Lorak-mmk left a comment

Lorak-mmk Feb 13, 2025

muzarski Feb 14, 2025

k0machi Feb 14, 2025

Lorak-mmk Feb 13, 2025

k0machi Feb 14, 2025

Lorak-mmk Feb 13, 2025

k0machi Feb 14, 2025

k0machi Feb 14, 2025

Lorak-mmk Feb 13, 2025

k0machi Feb 14, 2025

Lorak-mmk Feb 14, 2025

k0machi Feb 14, 2025

k0machi Feb 14, 2025

Lorak-mmk Feb 13, 2025

k0machi Feb 14, 2025

k0machi Feb 14, 2025

	values: Vec<(LitVal, BoundVal)>,
	values: &[(LitVal, BoundVal)],

tests: serialization-deserialization tests #1232

Are you sure you want to change the base?

tests: serialization-deserialization tests #1232

Conversation

k0machi commented Feb 12, 2025 • edited Loading

Pre-review checklist

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

Lorak-mmk commented Feb 13, 2025

k0machi commented Feb 13, 2025

wprzytula left a comment

Choose a reason for hiding this comment

k0machi commented Feb 13, 2025

wprzytula left a 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

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

k0machi commented Feb 13, 2025

Lorak-mmk left a 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

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

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

k0machi commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading