Skip to content
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.

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

add tests for unstable_backend #1765

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

pkhry
Copy link
Contributor

@pkhry pkhry commented Sep 9, 2024

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

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

impl Data {
async fn call_meth<'a>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async fn call_meth<'a>(
async fn call<'a>(

(*method)(&mut self.data_table, &mut self.subscription_channel, params)
}

async fn call_subscription<'a>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async fn call_subscription<'a>(
async fn subscribe<'a>(


type RpcResult<T> = Result<T, RpcError>;
type Item = RpcResult<String>;
pub type SubscriptionHandler = Box<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


struct MockDataTable {
items: HashMap<Vec<u8>, VecDeque<Item>>,
pub enum Message<T> {
Copy link
Member

@niklasad1 niklasad1 Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

request: MockDataTable,
subscription: Subscription,
data_table: MockDataTable,
subscription_channel: Option<Subscription>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move the subscription_channel to the subscription handler? or is this needed to keep the channel alive?

}
Message::Single(item) => sender.send(item).await.unwrap(),
};
});
}
}

struct Data {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit of strange call this type Data which has functionality to perform rpc calls and subscription + the mock data table.

I would rather remove this struct and move all this functionality to the MockedRpcClient or rename this to InnerMockedRpcClient or something like that

Self {
data: Data {
data_table: MockDataTable::new(),
subscription_channel: None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this always None? can we remove it?

}
}

pub fn add_method(mut self, method_name: &str, meth: MethodHandler) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lots of meth ^^ :)

.add_mock_data_from_iter(response_data)
.build();

let (backend, mut driver): (UnstableBackend<Conf>, _) =
Copy link
Member

@niklasad1 niklasad1 Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this unstable backend stuff to a helper function such as fn run_backend? It's used for several tests

Ok(resp1 @ StorageResponse { .. }),
Ok(resp2 @ StorageResponse { .. }),
Err(Error::Other(s))
] if s == "errro"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Suggested change
] if s == "errro"
] if s == "error"


assert!(matches!(
response,
Err(Error::Other(reason)) if reason == "errro"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Suggested change
Err(Error::Other(reason)) if reason == "errro"
Err(Error::Other(reason)) if reason == "error"

self
}

pub fn add_mock_data_from_iter<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is the only way to add mock_data, why not removing the from_iter?

Suggested change
pub fn add_mock_data_from_iter<
pub fn add_mock_data<

@@ -839,6 +874,22 @@ pub(crate) mod hashmap_as_tuple_list {
deserializer.deserialize_any(HashMapVisitor(PhantomData))
}

/// Serialize a number as string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder rather move these helpers to the test module

@@ -778,6 +799,9 @@ pub(crate) mod unsigned_number_as_string {
use serde::de::{Deserializer, Visitor};
use std::fmt;

#[cfg(test)]
use serde::ser::Serializer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to test module?!

@@ -786,6 +810,14 @@ pub(crate) mod unsigned_number_as_string {
deserializer.deserialize_any(NumberVisitor(std::marker::PhantomData))
}

/// Serialize a number as string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to test module?!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants