-
Notifications
You must be signed in to change notification settings - Fork 265
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: improved mock server for unit and provider testing #1952
base: main
Are you sure you want to change the base?
feat: improved mock server for unit and provider testing #1952
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supportive
some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smol pedantic nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last suggestion to remove channel because none of this does IO
// Spawn a new task to handle this request | ||
request_handlers.push(tokio::spawn(async move { | ||
// Get the next queued response or generate an error | ||
let response = inner.replies.lock().pop_front().map_or_else(|| { | ||
warn!("No queued response available for request"); | ||
serde_json::to_vec(&json!({ | ||
"jsonrpc": "2.0", | ||
"id": request.get("id"), | ||
"error": { | ||
"code": -32603, | ||
"message": "No response queued" | ||
} | ||
})).expect("JSON serialization cannot fail") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo we can get rid of this additional roundtrip entirely.
we don't need this and can write the response right away.
// Spawn a new task to handle this request | ||
request_handlers.push(tokio::spawn(async move { | ||
// Get the next queued response or generate an error | ||
let response = inner.replies.lock().pop_front().map_or_else(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also problematic because there's no guarantee that this task will pop the item that was at first position when the task was spawned
ah one more problem here is that this uses unix socket which is incompatible with windows, we have |
Motivation
As a user of the ecosystem, I need a robust way to unit test code without having to connect to a node or fork alloy, for example when I need the presence of a provider I control the responses of. The existing mock provider was limited in that it could not handle concurrent requests, nor did it have a proper handle for management.
Solution
This pull request implements a much improved mock provider that can be used, and is usable, for flexible unit tests across the ecosystem where a provider is involved.
Additionally, there are tests, which could be included here at will.
However, I didn't want to presume to add dependencies to this crate.
PR Checklist