Integration tests for unstable-reconnecting-rpc-client
#1711
Conversation
There was a problem hiding this comment.
Cool,
So for me the MVP is to add one extra test for the unstable and stable backend where we restart the node and see that it's works as intended i.e, running all existing tests with reconnecting rpc client is not needed. I don't know what's the best to test for this but I suggest 1) block subscription, 2) kill/restart the node 3) check that block subscription works after reconnect
AFAIU, that may take 1-3 minutes to do as starting the node can be slow which should be acceptable since some other jobs are already quite slow because they run in parallel.
Bonus stuff as the first step would be to test that unstable backend emit an error when the block subscription misses blocks during "reconnect".
| @@ -121,6 +121,9 @@ async fn transaction_validation() { | |||
|
|
|||
| wait_for_blocks(&api).await; | |||
|
|
|||
| #[cfg(feature = "unstable-reconnecting-rpc-client")] | |||
| let _ctx = ctx.restart().await; | |||
There was a problem hiding this comment.
I would rather create a separate test for unstable-reconnecting stuff than injecting it here.
Also this would remove all feature-gated stuff in this PR and just feature-gate the test itself
There was a problem hiding this comment.
removed all the feature gating stuff and moved this to a separate test
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
| signed_extrinsic | ||
| .validate() | ||
| .await | ||
| .expect("validation failed"); |
There was a problem hiding this comment.
.validate() obtains the latest block ref and then calls some runtime API, but I'm not sure how much that is testing the reconnecting logic, so curious what you think about this? I guess the point is that Subxt reconnects and doesn't start throwing out errors for every call, which is definitely a good start!
A couple of other tests that we might want to add:
-
Subscribe to storage entries and restart the node while streaming them, checking that you don't miss any inbetween or start over or whatever. (might require some setup though to get some storage entries of interest, or maybe subscribe to accountIds and expect the dev ones or something).
-
Subscribe to finalized blocks and restart while this is happening, checking that the block numbers of returned blocks follow eachother nicely regardless (though 6s block times means you'd probably not often screw up)
There was a problem hiding this comment.
Subscribe to finalized blocks and restart while this is happening, checking that the block numbers of returned blocks follow eachother nicely regardless (though 6s block times means you'd probably not often screw up)
This will return a toplevel error instead of retrying in the stream currently.
Subscribe to storage entries and restart the node while streaming them, checking that you don't miss any inbetween or start over or whatever. (might require some setup though to get some storage entries of interest, or maybe subscribe to accountIds and expect the dev ones or something).
I've rewritten the test to check that clients follow the same rules for streaming finalized blocks
There was a problem hiding this comment.
Generally looks good to me, and a good foundation for adding some more integration tests around restarting! Mainly I just have a Q about whether we need the feature flag, and what test(s) are best to have (though happy to start with the test you added and then add some more)!
Description
Issue link
I've plugged the new rpc client into integration tests behind a feature-flag.
Reconnection testing
I've added a few functions to have the ability to restart the
substrate-nodewhen needed inclienttests, but so far it seems like handwriting a server with predetermined responses might be a better idea as node restarts can take upwards of 30-60s inside a test.