Test try from remote network config source #1713
Conversation
WalkthroughA new test module has been added to the codebase, specifically targeting the asynchronous behavior of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant RemoteNetworks as RemoteNetworks::try_from_remote_network_config_source
participant HTTP as HTTP Endpoint
participant Error as Error Handling
Test->>RemoteNetworks: Call with config source and format
alt Successful "chainid" fetch
RemoteNetworks->>HTTP: Fetch config source
HTTP-->>RemoteNetworks: Return valid response
RemoteNetworks-->>Test: Return parsed RemoteNetworks
else HTTP decode error
RemoteNetworks->>HTTP: Fetch config source
HTTP-->>RemoteNetworks: Return undecodable response
RemoteNetworks->>Error: Wrap as ReqwestError
RemoteNetworks-->>Test: Return error
else Connection error
RemoteNetworks->>HTTP: Fetch config source (invalid URL)
HTTP-->>RemoteNetworks: Connection error
RemoteNetworks->>Error: Wrap as ReqwestError
RemoteNetworks-->>Test: Return error
else Unsupported/empty format
RemoteNetworks->>Error: UnknownFormat error
RemoteNetworks-->>Test: Return error
end
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
0xgleb
force-pushed
the
test-try-from-remote-network-config-source
branch
from
b049b99 to
47f9809
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/settings/src/remote/chains/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: Deploy-Preview
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
🔇 Additional comments (1)
crates/settings/src/remote/chains/mod.rs (1)
97-124: Good test structure for multiple cases.This test effectively covers multiple format string cases without making network calls, making it fast and reliable. The use of a vector to test multiple cases is a good pattern for similar test scenarios.
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_ok_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://chainid.network/chains.json".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let RemoteNetworks::ChainId(chain_ids) = result; | ||
|
|
||
| assert_ne!(chain_ids.len(), 0); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using mocks for external API calls in tests.
This test makes a real network call to an external API, which is good for end-to-end testing but can make tests brittle and slow. Consider using a mock server like mockito or wiremock to simulate the API response, making the test more reliable and faster.
// Example with mockito:
let mock_server = mockito::server_url();
let _m = mockito::mock("GET", "/chains.json")
.with_status(200)
.with_header("content-type", "application/json")
.with_body(r#"[{"name":"Ethereum Mainnet","chain":"ETH","icon":"ethereum","rpc":["https://eth.llamarpc.com"]}]"#)
.create();
let remote_networks_config_source = RemoteNetworksConfigSource {
format: "chainid".to_string(),
url: format!("{}/chains.json", mock_server),
};There was a problem hiding this comment.
Yeah, I was thinking about this. Should I make all requests mocked? @hardyjosh @findolor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_ok_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://chainid.network/chains.json".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let RemoteNetworks::ChainId(chain_ids) = result; | ||
|
|
||
| assert_ne!(chain_ids.len(), 0); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_invalid_response_err_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://example.com/".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await; | ||
|
|
||
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | ||
| if err.is_decode() { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| panic!("Expected RemoteNetworkError::ReqwestError decoding error"); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_invalid_url_err_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://invalid.url/unreal".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await; | ||
|
|
||
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | ||
| if err.is_connect() { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| panic!("Expected RemoteNetworkError::ReqwestError connection error"); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_format_parsing_err_case() { | ||
| let remote_networks_config_sources = vec![ | ||
| RemoteNetworksConfigSource { | ||
| format: "".to_string(), | ||
| url: "https://example.com".to_string(), | ||
| }, | ||
| RemoteNetworksConfigSource { | ||
| format: "unknown".to_string(), | ||
| url: "https://polygon-rpc.com/".to_string(), | ||
| }, | ||
| ]; | ||
|
|
||
| for remote_networks_config_source in remote_networks_config_sources { | ||
| let unexpected_format = remote_networks_config_source.format.clone(); | ||
|
|
||
| let result = RemoteNetworks::try_from_remote_network_config_source( | ||
| remote_networks_config_source, | ||
| ) | ||
| .await; | ||
|
|
||
| if let Err(RemoteNetworkError::UnknownFormat(format)) = result { | ||
| assert_eq!(format, unexpected_format); | ||
| } else { | ||
| panic!("Expected RemoteNetworkError::UnknownFormat error"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add tests for potential timeouts and slow responses.
The current tests cover success, decoding errors, connection errors, and format errors, but don't test timeout scenarios. Consider adding tests that verify how the system handles slow responses or timeouts, which are common in network interactions.
#[tokio::test]
async fn test_try_from_remote_network_config_source_timeout() {
// This requires a mock server that can be configured to delay responses
// or a real endpoint that's known to be slow/timeout
let remote_networks_config_source = RemoteNetworksConfigSource {
format: "chainid".to_string(),
url: "https://blackhole.webpagetest.org/".to_string(), // Known to time out
};
let client = reqwest::Client::builder()
.timeout(std::time::Duration::from_millis(100))
.build()
.unwrap();
// This would require modifying the original function to accept a custom client
let result = RemoteNetworks::try_from_remote_network_config_source_with_client(
remote_networks_config_source,
client,
).await;
assert!(matches!(
result,
Err(RemoteNetworkError::ReqwestError(err)) if err.is_timeout()
));
}| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_invalid_url_err_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://invalid.url/unreal".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await; | ||
|
|
||
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | ||
| if err.is_connect() { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| panic!("Expected RemoteNetworkError::ReqwestError connection error"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test might be flaky due to network changes.
This test relies on a non-existent domain name which could potentially be registered in the future, causing the test to fail. Consider using a more controlled approach like a mock server or a well-known local address that will reliably fail (like localhost with an unused port).
- url: "https://invalid.url/unreal".to_string(),
+ url: "http://localhost:1".to_string(), // Guaranteed to fail with connection refused📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn test_try_from_remote_network_config_source_invalid_url_err_case() { | |
| let remote_networks_config_source = RemoteNetworksConfigSource { | |
| format: "chainid".to_string(), | |
| url: "https://invalid.url/unreal".to_string(), | |
| }; | |
| let result = | |
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | |
| .await; | |
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | |
| if err.is_connect() { | |
| return; | |
| } | |
| } | |
| panic!("Expected RemoteNetworkError::ReqwestError connection error"); | |
| } | |
| #[tokio::test] | |
| async fn test_try_from_remote_network_config_source_invalid_url_err_case() { | |
| let remote_networks_config_source = RemoteNetworksConfigSource { | |
| format: "chainid".to_string(), | |
| url: "http://localhost:1".to_string(), // Guaranteed to fail with connection refused | |
| }; | |
| let result = | |
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | |
| .await; | |
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | |
| if err.is_connect() { | |
| return; | |
| } | |
| } | |
| panic!("Expected RemoteNetworkError::ReqwestError connection error"); | |
| } |
| #[tokio::test] | ||
| async fn test_try_from_remote_network_config_source_invalid_response_err_case() { | ||
| let remote_networks_config_source = RemoteNetworksConfigSource { | ||
| format: "chainid".to_string(), | ||
| url: "https://example.com/".to_string(), | ||
| }; | ||
|
|
||
| let result = | ||
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | ||
| .await; | ||
|
|
||
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | ||
| if err.is_decode() { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| panic!("Expected RemoteNetworkError::ReqwestError decoding error"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve test assertions with more explicit error checks.
The current pattern of using if let ... return followed by a panic works but makes the test's intent less clear. Consider using a more explicit assertion pattern and testing specific error details.
- if let Err(RemoteNetworkError::ReqwestError(err)) = &result {
- if err.is_decode() {
- return;
- }
- }
-
- panic!("Expected RemoteNetworkError::ReqwestError decoding error");
+ assert!(matches!(
+ result,
+ Err(RemoteNetworkError::ReqwestError(err)) if err.is_decode()
+ ), "Expected a decoding error, got {:?}", result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn test_try_from_remote_network_config_source_invalid_response_err_case() { | |
| let remote_networks_config_source = RemoteNetworksConfigSource { | |
| format: "chainid".to_string(), | |
| url: "https://example.com/".to_string(), | |
| }; | |
| let result = | |
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | |
| .await; | |
| if let Err(RemoteNetworkError::ReqwestError(err)) = &result { | |
| if err.is_decode() { | |
| return; | |
| } | |
| } | |
| panic!("Expected RemoteNetworkError::ReqwestError decoding error"); | |
| } | |
| #[tokio::test] | |
| async fn test_try_from_remote_network_config_source_invalid_response_err_case() { | |
| let remote_networks_config_source = RemoteNetworksConfigSource { | |
| format: "chainid".to_string(), | |
| url: "https://example.com/".to_string(), | |
| }; | |
| let result = | |
| RemoteNetworks::try_from_remote_network_config_source(remote_networks_config_source) | |
| .await; | |
| assert!(matches!( | |
| result, | |
| Err(RemoteNetworkError::ReqwestError(err)) if err.is_decode() | |
| ), "Expected a decoding error, got {:?}", result); | |
| } |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit