test(query_result): cluster state tests #1229

soyacz · 2025-02-11T16:27:45Z

Added tests around query response metadata:

initial test for cluster state (node addresses)
improved trace_info test with more detailed verification

based on:
com.datastax.oss.driver.core.metadata.MetadataIT
com.datastax.oss.driver.core.cql.QueryTraceIT

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-11T16:38:33Z

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

Lorak-mmk · 2025-02-12T12:35:13Z

scylla/tests/integration/query_result.rs

+#[tokio::test]
+#[ntest::timeout(60000)]
+#[cfg(not(scylla_cloud_tests))]
+async fn test_session_should_have_metadata() {
+    setup_tracing();
+    let session = create_new_session_builder().build().await.unwrap();
+    let state = session.get_cluster_state();
+    let keys = ["SCYLLA_URI", "SCYLLA_URI2", "SCYLLA_URI3"];
+    let expected_addresses: HashSet<String> = keys
+        .iter()
+        .map(|key| env::var(key).unwrap_or_else(|_| panic!("{} not set", key)))
+        .collect();
+
+    let got_addresses: HashSet<String> = state
+        .get_nodes_info()
+        .iter()
+        .map(|node| node.address.to_string())
+        .collect();
+
+    assert_eq!(
+        got_addresses, expected_addresses,
+        "Cluster node addresses do not match environment variables"
+    );
+}


I think this test doesn't fit in the query_result tests. Maybe create a new file for it, like cluster_state_tests?

If you do, you could also move some tests from scylla/tests/integration/session.rs into this new file, because they test cluster state too. I'm talking about tests like test_table_partitioner_in_metadata, test_primary_key_ordering_in_metadata etc. This could be done in another PR.

Lorak-mmk · 2025-02-12T12:39:01Z

scylla/tests/integration/query_result.rs

+#[tokio::test]
+async fn test_should_not_have_tracing_id_when_tracing_disabled() {
+    setup_tracing();
+    let session = create_new_session_builder().build().await.unwrap();
+    let query: Query = Query::new("SELECT release_version FROM system.local");
+
+    let result = session.query_unpaged(query, &[]).await.unwrap();
+    let tracing_id: Option<Uuid> = result.tracing_id();
+    assert!(tracing_id.is_none());
+}
+
+#[tokio::test]
+async fn test_should_fetch_trace_when_tracing_enabled() {
+    let session = create_new_session_builder().build().await.unwrap();
+
+    let mut query = Query::from("SELECT release_version FROM system.local");
+    query.set_tracing(true);
+
+    let result = session.query_unpaged(query, &[]).await.unwrap();
+    let tracing_id: Option<Uuid> = result.tracing_id();
+    assert!(tracing_id.is_some());
+    let tracing_info: TracingInfo = session
+        .get_tracing_info(&tracing_id.unwrap())
+        .await
+        .unwrap();
+
+    // Verify trace information is present and has expected format
+    assert!(!tracing_info.events.is_empty());
+
+    // Check if the request type matches
+    assert_eq!(tracing_info.request.unwrap(), "Execute CQL3 query");
+
+    // Verify duration is positive
+    assert!(tracing_info.duration.unwrap() > 0);
+
+    // Verify started_at timestamp is present
+    assert!(tracing_info.started_at.unwrap().0 > 0);
+
+    // Check parameters
+    assert!(tracing_info
+        .parameters
+        .clone()
+        .unwrap()
+        .contains_key("consistency_level"));
+    assert!(tracing_info.parameters.unwrap().contains_key("query"));
+
+    // Check events
+    for event in tracing_info.events {
+        assert!(!event.activity.clone().unwrap().is_empty());
+        assert!(event.source.is_some());
+        assert!(event.source_elapsed.unwrap() >= 0);
+        assert!(!event.activity.unwrap().is_empty());
+    }
+}


Could you look into test_tracing_ tests in integration/session.rs? It looks to me like those tests mostly duplicate the ones already existing - maybe instead of adding new ones you could improve existing ones with the checks that you wrote?

Lorak-mmk · 2025-02-12T12:40:12Z

scylla/tests/integration/query_result.rs

+#[tokio::test]
+#[ntest::timeout(60000)]
+#[cfg(not(scylla_cloud_tests))]
+async fn should_expose_execution_info_on_exceptions() {
+    setup_tracing();
+    let res = test_with_3_node_cluster(
+        ShardAwareness::Unaware,
+        |proxy_uris, translation_map, mut running_proxy| async move {
+            let session: Session = SessionBuilder::new()
+                .known_node(proxy_uris[0].as_str())
+                .address_translator(Arc::new(translation_map))
+                .build()
+                .await
+                .unwrap();
+
+            let forge_error_rule = RequestRule(Condition::True, RequestReaction::forge().invalid());
+            running_proxy.running_nodes[0]
+                .change_request_rules(Some(vec![forge_error_rule.clone()]));
+            running_proxy.running_nodes[1]
+                .change_request_rules(Some(vec![forge_error_rule.clone()]));
+            running_proxy.running_nodes[2]
+                .change_request_rules(Some(vec![forge_error_rule.clone()]));
+
+            let mut query = Query::from("select * from foo");
+            let history_listener = Arc::new(HistoryCollector::new());
+            query.set_history_listener(history_listener.clone());
+            let err = session
+                .query_unpaged(query, &[])
+                .await
+                .expect_err("expecting error");
+            let structured_history: StructuredHistory = history_listener.clone_structured_history();
+
+            assert_matches!(
+                err,
+                ExecutionError::LastAttemptError(RequestAttemptError::DbError(_, _))
+            );
+            assert_eq!(structured_history.requests.len(), 1);
+            running_proxy
+        },
+    )
+    .await;
+
+    match res {
+        Ok(()) => (),
+        Err(err) => panic!("{}", err),
+    }
+}


Isn't that mostly a history tests? What does it verify that tests in integration/history.rs don't?

basically I wanted to test more details in errors, like Java driver does provide - but Rust driver don't expose them.
Somehow I have problems with finding similar tests - like also tracing tests I didn't notice already exist. Still I'm adapting to new IDE and ctrl+f mislead me.

The structure of our tests (which test resides in which file) is unfortunately not the best :( We'd like to improve it - which is why I made my comments.
Feel free to ask if you can't find something that you suspect may already exist.
Nice way is to check for references to the function / method / field etc that you want to use in the test.
For Session::get_tracing_info() it does correctly find a reference in integration/session.rs:

specifically, in test_get_tracing_info. This function is in turn called by test_tracing in the same file, and there you can see our existing test cases for tracing.

mostly my confusion is taken (again) out of python: test files usually start with test_ so when I search for something and I see it in test_xxx file then I know it's in testing. Here when searching it is shown in session.rs which mislead me (I missed looking at the directories).

Yes, Rust and Python have vastly different approaches to many topics. In general, in Rust things are usually more explicit, instead of relying on properties like name.

soyacz · 2025-02-12T14:13:42Z

@Lorak-mmk I adjusted to your's comments. Other state tests will be moved in different PR.

Added tests around query response metadata: 1. initial test for cluster state (node addresses) 2. improved trace_info test with more detailed verification based on: com.datastax.oss.driver.core.metadata.MetadataIT com.datastax.oss.driver.core.cql.QueryTraceIT

Lorak-mmk · 2025-02-13T07:52:41Z

scylla/tests/integration/session.rs

+    // Verify duration is positive when Scylla is used
+    if session
+        .get_cluster_state()
+        .get_nodes_info()
+        .first()
+        .unwrap()
+        .sharder()
+        .is_some()
+    {
+        assert!(tracing_info.duration.unwrap() > 0);
+    }


Is there some Scylla issue about the incompatibility? Or maybe a documentation if the incompatibility is intentional? It should be linked here.

wprzytula · 2025-02-13T08:48:13Z

scylla/tests/integration/cluster_state_tests.rs

+async fn test_session_should_have_metadata() {
+    setup_tracing();
+    let session = create_new_session_builder().build().await.unwrap();
+    let state = session.get_cluster_state();
+    let keys = ["SCYLLA_URI", "SCYLLA_URI2", "SCYLLA_URI3"];
+    let expected_addresses: HashSet<String> = keys
+        .iter()
+        .map(|key| env::var(key).unwrap_or_else(|_| panic!("{} not set", key)))
+        .collect();
+
+    let got_addresses: HashSet<String> = state
+        .get_nodes_info()
+        .iter()
+        .map(|node| node.address.to_string())
+        .collect();
+
+    assert_eq!(
+        got_addresses, expected_addresses,
+        "Cluster node addresses do not match environment variables"
+    );
+}


🤔 Maybe: test_session_should_have_metadata -> test_session_should_have_topology_metadata? Schema metadata is not checked here at all.

wprzytula · 2025-02-13T08:50:50Z

scylla/tests/integration/session.rs

+    // Check parameters
+    assert!(tracing_info
+        .parameters
+        .clone()
+        .unwrap()
+        .contains_key("consistency_level"));
+    assert!(tracing_info.parameters.unwrap().contains_key("query"));


🔧 Instead of clone()ing an Option<T> in order to unwrap() it, it's better to use as_ref() to unwrap() on a borrowed version of the Option. See:

Suggested change

// Check parameters

assert!(tracing_info

.parameters

.clone()

.unwrap()

.contains_key("consistency_level"));

assert!(tracing_info.parameters.unwrap().contains_key("query"));

// Check parameters

assert!(tracing_info

.parameters

.as_ref()

.unwrap()

.contains_key("consistency_level"));

assert!(tracing_info.parameters.as_ref().unwrap().contains_key("query"));

wprzytula · 2025-02-13T08:51:34Z

scylla/tests/integration/session.rs

+        assert!(!event.activity.clone().unwrap().is_empty());
+        assert!(event.source.is_some());
+        assert!(event.source_elapsed.unwrap() >= 0);
+        assert!(!event.activity.unwrap().is_empty());


Suggested change

assert!(!event.activity.clone().unwrap().is_empty());

assert!(event.source.is_some());

assert!(event.source_elapsed.unwrap() >= 0);

assert!(!event.activity.unwrap().is_empty());

assert!(!event.activity.as_ref().unwrap().is_empty());

assert!(event.source.is_some());

assert!(event.source_elapsed.unwrap() >= 0);

assert!(!event.activity.as_ref().unwrap().is_empty());

soyacz requested review from wprzytula and Lorak-mmk February 11, 2025 16:27

soyacz force-pushed the hackathon-query-metadata-test branch 2 times, most recently from ae622d4 to 3b444d3 Compare February 11, 2025 16:56

Lorak-mmk requested changes Feb 12, 2025

View reviewed changes

soyacz force-pushed the hackathon-query-metadata-test branch from 3b444d3 to 9e1ed5a Compare February 12, 2025 14:12

soyacz changed the title ~~test(query_result): added metadata tests~~ test(query_result): cluster state tests Feb 12, 2025

soyacz force-pushed the hackathon-query-metadata-test branch from 9e1ed5a to 4981fae Compare February 12, 2025 15:23

soyacz force-pushed the hackathon-query-metadata-test branch from 4981fae to e1254b6 Compare February 12, 2025 15:30

soyacz requested a review from Lorak-mmk February 12, 2025 15:55

Lorak-mmk reviewed Feb 13, 2025

View reviewed changes

wprzytula requested changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(query_result): cluster state tests #1229

test(query_result): cluster state tests #1229

soyacz commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

Lorak-mmk Feb 12, 2025

Lorak-mmk Feb 12, 2025

Lorak-mmk Feb 12, 2025

soyacz Feb 12, 2025

Lorak-mmk Feb 12, 2025

soyacz Feb 12, 2025

Lorak-mmk Feb 12, 2025

soyacz commented Feb 12, 2025

Lorak-mmk Feb 13, 2025

wprzytula Feb 13, 2025

wprzytula Feb 13, 2025

wprzytula Feb 13, 2025

test(query_result): cluster state tests #1229

Are you sure you want to change the base?

test(query_result): cluster state tests #1229

Conversation

soyacz commented Feb 11, 2025 • edited Loading

Pre-review checklist

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

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

soyacz commented Feb 12, 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

soyacz commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading