Add http/ipc jsonrpc testing to harness #148
Conversation
njgheorghita
requested review from
ogenev,
jacobkaufmann and
carver
and removed request for
ogenev and
jacobkaufmann
ethportal-peertest/src/jsonrpc.rs
Outdated
| let result = body_json.get("result").unwrap(); | ||
| validate_endpoint_response(endpoint.method, result); |
There was a problem hiding this comment.
Can we add some check for an error member on the JSON-RPC response here? If there's an error, result may be empty, and the validation failure would not give us visibility into some possible error. Maybe we ought to change the signature of validate_endpoint_response to take the body of the response and check for an error, logging it and failing if one is present.
| #[derive(Copy, Clone)] | ||
| pub struct JsonRpcEndpoint { | ||
| pub method: &'static str, | ||
| pub id: &'static u8, |
There was a problem hiding this comment.
Is the id just a convenience field so that we do not need to generate a unique id for each JSON-RPC request that we test?
There was a problem hiding this comment.
Yup, exactly. Not strictly necessary to do here, but since params will also soon be added to the struct, seemed harmless to stick these in there as well
ethportal-peertest/src/jsonrpc.rs
Outdated
| let json_string = format!( | ||
| r#"{{"jsonrpc":"2.0","id":0,"method":"{}","params":[]}}"#, | ||
| endpoint.method | ||
| ); |
There was a problem hiding this comment.
Why not use endpoint.to_jsonrpc() here?
|
Good start! I am concerned that this may get cumbersome as we continue to add endpoints. Is it possible to leverage the endpoint types that we have defined in |
@jacobkaufmann For sure, that's how this pr started out. But I punted it down the road to avoid dealing with wrapped types. One alternative option is to put all the testing properties on the original object definition, but I wanted to avoid bloating |
|
@njgheorghita Gotcha. Maybe we can do something similar to the |
njgheorghita
force-pushed
the
testharness
branch
3 times, most recently
from
ef67a82 to
b733133
Compare
CONTRIBUTING.md
Outdated
| @@ -22,8 +22,10 @@ through github via pull requests. | |||
| * Mark unfinished pull requests with the "Work in Progress" label. | |||
| * Before submitting a pr for review, you should run the following commands | |||
| locally and make sure they are passing, otherwise CI will raise an error. | |||
| * `cargo fmt` and `cargo clippy` for linting checks | |||
| * `cargo test --workspace` to run all tests | |||
| * `cargo fmt -- --check` and `cargo clippy -- --deny warnings` for linting checks | |||
There was a problem hiding this comment.
| * `cargo fmt -- --check` and `cargo clippy -- --deny warnings` for linting checks | |
| * `cargo fmt --all` and `cargo clippy --all -- --deny warnings` for linting checks |
| Ok(val) => val, | ||
| Err(msg) => { | ||
| warn!("Error parsing http request: {:?}", msg); | ||
| break; |
There was a problem hiding this comment.
Shall we write an error message to the stream instead of a break?
ethportal-peertest/src/jsonrpc.rs
Outdated
| pub async fn test_jsonrpc_endpoints_over_ipc() { | ||
| for endpoint in JsonRpcEndpoint::all_endpoints() { | ||
| info!("Testing over IPC: {:?}", endpoint.method); | ||
| let mut stream = UnixStream::connect("/tmp/trin-jsonrpc.ipc").unwrap(); |
There was a problem hiding this comment.
If we want to keep the direction to support cross-client testing in this crate, probably it is worth including the socket path as a parameter to CLI. I guess we can keep the default value as /tmp/trin-jsonrpc.ipc.
njgheorghita
force-pushed
the
testharness
branch
from
b733133 to
a2a8fb0
Compare
First steps towards a more robust test harness. For some reason, the overlay network pings seem to be broken. I dug into it a bit, but couldn't quite figure out the solution. It also seems like either #141 might help the situation, or at least help with debugging the situation. Tracking that in this issue #147
Fixes #4