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

feat: create json-rpc mock builder #1378

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

morph-dev
Copy link
Collaborator

What was wrong?

There is no easy way to mock our json-rpc request/responses which makes unit test of certain parts of the codebase very challenging.

How was it fixed?

Created json-rpc mock builder, that is very customization.

See example of the real PR use case here

That case can be simplified even further (since it only cares about one response) in the following way:

let history_jsonrpc_tx = MockJsonRpcBuilder::new_builder().or_else(content_info);

For more complex logic, one can do something like this:

let history_jsonrpc_tx = MockJsonRpcBuilder::new_builder()
    // respond to RecursiveFindContent with one respond
    .with_response(
        HistoryEndpoint::RecursiveFindContent(content_key_1),
        content_info_1
    )
    // to another content key with another
    .with_response(
        HistoryEndpoint::RecursiveFindContent(content_key_2),
        content_info_2
    )
    // to gossip with completely different type
    .with_response(
        HistoryEndpoint::Gossip(content_key, content_value),
        42 // number of nodes that received gossip
    )
    // even custom trigger that handles all Ping requests
    .with_custom_trigger(
        |request| matches!(request, HistoryEndpoint::Ping(_)), // respond the same for every ping request
        pong_info,
    )
    // fail otherwise
    .of_fail();

To-Do

@morph-dev morph-dev self-assigned this Aug 13, 2024
@morph-dev
Copy link
Collaborator Author

I'm not fully set on this approach as final version. I'm looking for the early feedback and thoughts.
If there are no major objections, I'm also fine with merging it like this and we can iterate as we see use cases.

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

I don't think we should or try to get it right the first time as that is idealism. If this will help us write good tests and works I think that is good for now. If it doesn't fit our needs in the future we can fix that.

Since this isn't an architecture change which determines the fate of the whole project.

I would be a fan of merging on it and iterating as required.

:shipit:

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

Looks great to me 💯 !

}

impl<T: PartialEq + Send + 'static> MockJsonRpcBuilder<T> {
pub fn new_builder() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would rename this method to new to match better the Rust naming conventions for builders.

@morph-dev morph-dev merged commit f81b98a into ethereum:master Aug 14, 2024
8 checks passed
@morph-dev morph-dev deleted the json_rpc_mocking branch August 14, 2024 10:13
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.

3 participants