add reconnecting tests for unstable_backend #1765

pkhry · 2024-09-09T09:33:37Z

Description

As in the title. This PR adds tests for UnstableBackend implementation in subxt/backend.
The tests are mostly about testing StorageResponses in different cases of reconnections.
Pr uses a mocked RpcClient implementation for providing expected responses.

Additional changes

refactor MockRpcClient to be usable across legacy backend and unstable_backend following suggestion from last PR
add test for stale subscription ID causing error when trying to fetch storage to have a test that should fail if the relevant issue is fixed

Part of the work on #1677

subxt/src/backend/mod.rs

niklasad1 · 2024-09-20T15:28:07Z

subxt/src/backend/mod.rs


-        type RpcResult<T> = Result<T, RpcError>;
-        type Item = RpcResult<String>;
+        pub type SubscriptionHandler = Box<


quite complicated type and a bit hard to read but I'm fine to remove the lifetimes and cloning the data for each call if it simplifies the code...

it just tests anyway

i dont think cloning would help as we need mutable refs to mutate the data inside, but if you have an idea how to simplify it, please suggest

It's fine to keep this, I thought it was possible to just clone the Arc<Mutex<_>> but it's one layer above this...

niklasad1 · 2024-09-20T15:29:47Z

subxt/src/backend/mod.rs


-        struct MockDataTable {
-            items: HashMap<Vec<u8>, VecDeque<Item>>,
+        pub enum Message<T> {


dq: why is many Result<T, Error> and a single just T?

when we load initial mock_data its concrete types like FollowEvent but inside mocked rpc its just Item which is Result<String,RpcError>

subxt/src/backend/mod.rs

subxt/src/backend/unstable/rpc_methods.rs

niklasad1

nice work, I have mostly have comments to simplify the tests to make it a little easier to read.

I'll take a closer look at the test themselves in the next iteration

niklasad1 · 2024-09-23T07:26:14Z

subxt/src/backend/unstable/rpc_methods.rs

+    #[serde(deserialize_with = "unsigned_number_as_string::deserialize")]
+    #[cfg_attr(
+        test,
+        serde(serialize_with = "unsigned_number_as_string::for_test::serialize")


aight, I see makes sense to have these in this module then

Missed the serialize_with

subxt/src/backend/mod.rs

niklasad1 · 2024-09-23T14:59:33Z

subxt/src/backend/unstable/rpc_methods.rs

 pub struct TransactionBlockDetails<Hash> {
    /// The block hash.
    pub hash: Hash,
    /// The index of the transaction in the block.
-    #[serde(with = "unsigned_number_as_string")]
+    #[serde(deserialize_with = "unsigned_number_as_string::deserialize")]


ok, deserialize is only implemented for this type and with is a shortcut for both serialize and deserialize impl

subxt/src/backend/mod.rs

niklasad1 · 2024-09-23T15:57:06Z

subxt/src/backend/mod.rs

+                ("continue_response", Message::Single(Ok(()))),
+                ("continue_response", Message::Single(Ok(()))),
+            ];
+            let mock_data = vec![


It's not really easy for me to follow how the responses is linked the "follow_event/subscription data`,
So I wonder whether we provide something like:

( method_response(id1), Message::Many(vec![Ok(storage_items("Id1", &[storage_result("ID1", "Data1")])), Ok(operation_continue("Id1"))]) )

where the "first item" is the method_response i.e, operationStarted and the "second item" is the subscription data i.e, follow_events on provided on that method response/operation ID.

That way it's simpler to understand the responses for each call but it may not work that well how you wrote the tests i.e, by pop the data unless it inserted with some unique id

hmm, given we know the exact ordering of the requests/responses/subscription messages i can try to refactor it to be one single stack of responses that will be popped one by one. But maybe dong this in a followup PR would be more appropriate?

Yeah it's fine to keep it out of this PR, let's hear what James and Alex thinks about this as well :)

Yep it would be a bit easier to follow the test with Niklas suggestion. If it turns out too complicated to change it we can do a followup for sure :D

Ok, so after a bit of experimenting it looks like it's not possible without adding an external dependency or adding mock data item by item to the mock data set. using dyn Serialize is not possible(not object safe) and adding another dependency just to make tests seem a bit more pretty feels like an overkill.

niklasad1

Great,

Minor thingy, whether we can improve the readability for the tests by linking responses and subscription messages together but it's not a blocker for this PR IMO.

subxt/src/backend/mod.rs

lexnv · 2024-09-24T16:40:00Z

subxt/src/backend/mod.rs

+                "chainHead_v1_storage",
+                Message::Many(Ok(vec![Ok(operation_error("Id1")), Ok(FollowEvent::Stop)])),
+            )];
+            let rpc_client = setup_mock_rpc_client(false)


Nice! The API looks quite elegant!

subxt/src/backend/mod.rs

lexnv · 2024-09-24T16:44:52Z

subxt/src/backend/mod.rs

+        #[tokio::test]
+        /// Tests the error behaviour in streams based on chainHead_v1_continue calls


nit: new line

lexnv · 2024-09-24T16:49:12Z

subxt/src/backend/mod.rs

+                )
+                .await
+                .unwrap();
+            // operation returned FollowEvent::OperationError


dq: Does subxt retry on OperationError?

No, all errors except DisconnectWillReconnect and Rejected are handed back to the user to deal with.

lexnv · 2024-09-24T16:54:08Z

subxt/src/backend/unstable/rpc_methods.rs

@@ -745,11 +769,16 @@ pub enum TransactionStatus<Hash> {

 /// Details of a block that a transaction is seen in.
 #[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
+#[cfg_attr(test, derive(Serialize))]


Should we just expose a derive(Serialize) instead of placing it under a cfg(test)? That might turn out to be useful for some developers, wdyt?

I'm not super opposed to this! If we do choose to do it though then we should make sure to be consistent and apply it on all related types and not just those used in tests (if any were missed!)

lexnv

LGTM! Tiny comments around test styles. Maybe we can expose the derive(Serialize) implementation for some structures and we can think about grouping test data, but those can be a followup 👍

subxt/src/backend/mod.rs

jsdw · 2024-09-25T16:44:43Z

subxt/src/backend/mod.rs

+                    Box::new(|data, _sub, params| {
+                        Box::pin(async move {
+                            let params = params.map(|p| p.get().to_string());
+                            let rpc_params = jsonrpsee::types::Params::new(params.as_deref());
+                            let key: sp_core::Bytes = rpc_params.sequence().next().unwrap();
+                            let value = data.pop(key.0).unwrap_single();
+                            value.map(|v| serde_json::value::RawValue::from_string(v).unwrap())
+                        })
+                    }),


I wonder how easily we could remove the Box's here to make the API cleaner.

Eg if add_method and add_subscription took in some generic like:

fn add_subscription(sub: &str, f: F) where Fn: Send + Fn( &'a mut MockDataTable, &'a mut Option<Subscription>, Option<Box<serde_json::value::RawValue>>, ) -> RawRpcFuture<'a, Box<serde_json::value::RawValue>>,

Then you could do the boxing internally and keep the API cleaner (maybe you also return some future that is then boxed or whatever instead of returning RawRpcFuture<'a, Box<serde_json::value::RawValue>> so that both boxes are removed?)

The downside is you probably end up having to copy these bounds in both the Box and in the where though could maybe avoid duplication with a tiny macro or trait like trait SubscriptionFn: Fn(...).

Not a big deal really because this is just test code but might be a nice little ergonomic win :)

removed boxing form the callsite

jsdw · 2024-09-25T16:57:53Z

subxt/src/backend/mod.rs

+                    )
+                });
+
+                (*sub)(&mut self.data_table, &mut self.subscription_channel, params)


I geuss this means that once you call or subscribe, the returned fut has a mutable borrow on the RPC client and then you can't do another call or subscribe while thefirst one is open, which I geuss is working out ok!

One thing I was wondering was whether we can keep MockDataTable separate from MockRpcClient so that the two were separate and one could eg pick a different way to pass data in to MockRpcClient stuff.

One approach would be to have the closure capture whatever you wanted eg:

let mock_data = MockDataTable::new(); MockRpcBuilder::default() .add_method( "state_getStorage", Box::new(|_sub, params| { Box::pin(async move { let params = params.map(|p| p.get().to_string()); let rpc_params = jsonrpsee::types::Params::new(params.as_deref()); let key: sp_core::Bytes = rpc_params.sequence().next().unwrap(); let value = mock_data.pop(key.0).unwrap_single(); value.map(|v| serde_json::value::RawValue::from_string(v).unwrap()) }) });

But I think that you need to have the mock data embedded in order for the lifetimes to work out nicely (though one could add some easy way to clone it or whatever but that's more annoying).

Another way to keep the two separate might be to have MockRpcClient<Data> so that one can be generic over the structure of data provided etc, allowing for different testing places to use different things. This would lead to the generic being added in a bunch of places though as a tradeoff for decoupling the two things.

Not sure it's a big deal either way, but it's the sort of thing I'd probably consider in this sort of design to make the MockRpcClient more generic in terms of how it can be used :)

Ok, so a few methods/subscriptions do in fact require access to the same data point, so capturing stuff in a closure ends up more cumbersome that the current approach in some cases

add tests for unstable_backend

f129063

pkhry requested a review from a team as a code owner September 9, 2024 09:33

clippy

42af8fd

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/unstable/rpc_methods.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/unstable/rpc_methods.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

subxt/src/backend/unstable/rpc_methods.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 20, 2024

View reviewed changes

review comments

4fd47de

pkhry requested a review from niklasad1 September 23, 2024 06:55

niklasad1 reviewed Sep 23, 2024

View reviewed changes

jsdw reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

review comments 2: the sequel

3396002

pkhry requested review from jsdw and niklasad1 September 23, 2024 11:59

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

subxt/src/backend/mod.rs Show resolved Hide resolved

niklasad1 reviewed Sep 23, 2024

View reviewed changes

niklasad1 approved these changes Sep 24, 2024

View reviewed changes

niklasad1 changed the title ~~add tests for unstable_backend~~ add reconnecting tests for unstable_backend Sep 24, 2024

pkhry and others added 2 commits September 24, 2024 14:11

review comments

b0126ba

Merge branch 'master' into pkhry/unstable_backend_tests

98749ab

lexnv reviewed Sep 24, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

lexnv reviewed Sep 24, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

lexnv reviewed Sep 24, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

lexnv reviewed Sep 24, 2024

View reviewed changes

lexnv approved these changes Sep 24, 2024

View reviewed changes

jsdw reviewed Sep 25, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 25, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 25, 2024

View reviewed changes

subxt/src/backend/mod.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 25, 2024

View reviewed changes

pkhry added 2 commits September 26, 2024 11:58

review comments

217c3e9

redundant await

8444df4

pkhry mentioned this pull request Sep 27, 2024

Deriving Serialize for more types inside subxt/backend defs #1787

Closed

pkhry merged commit 8f2c92f into master Sep 29, 2024
13 checks passed

pkhry deleted the pkhry/unstable_backend_tests branch September 29, 2024 22:03

pkhry mentioned this pull request Oct 1, 2024

derive serialize on more types #1797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add reconnecting tests for unstable_backend #1765

add reconnecting tests for unstable_backend #1765

pkhry commented Sep 9, 2024

niklasad1 Sep 20, 2024

pkhry Sep 23, 2024 •

edited

Loading

niklasad1 Sep 23, 2024

niklasad1 Sep 20, 2024 •

edited

Loading

pkhry Sep 23, 2024

niklasad1 left a comment

niklasad1 Sep 23, 2024

niklasad1 Sep 23, 2024

niklasad1 Sep 23, 2024 •

edited

Loading

pkhry Sep 24, 2024

niklasad1 Sep 24, 2024

lexnv Sep 24, 2024

pkhry Sep 27, 2024

niklasad1 left a comment

lexnv Sep 24, 2024

lexnv Sep 24, 2024

lexnv Sep 24, 2024

niklasad1 Sep 25, 2024 •

edited

Loading

lexnv Sep 24, 2024

jsdw Sep 25, 2024

lexnv left a comment

jsdw Sep 25, 2024

pkhry Sep 26, 2024

jsdw Sep 25, 2024

pkhry Sep 27, 2024

		#[tokio::test]
		/// Tests the error behaviour in streams based on chainHead_v1_continue calls

add reconnecting tests for unstable_backend #1765

add reconnecting tests for unstable_backend #1765

Conversation

pkhry commented Sep 9, 2024

Description

Additional changes

Choose a reason for hiding this comment

pkhry Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Sep 23, 2024 • 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

niklasad1 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

niklasad1 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv 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

pkhry Sep 23, 2024 •

edited

Loading

niklasad1 Sep 20, 2024 •

edited

Loading

niklasad1 Sep 23, 2024 •

edited

Loading

niklasad1 Sep 25, 2024 •

edited

Loading