From 5614596c92badc89a0cff22e9033106b73fdca1d Mon Sep 17 00:00:00 2001 From: hanabi1224 Date: Tue, 10 Dec 2024 18:52:35 +0800 Subject: [PATCH] feat(tool): dump RPC tests to a directory (#5054) --- CHANGELOG.md | 3 + src/rpc/client.rs | 2 +- src/rpc/reflect/mod.rs | 7 +- src/rpc/request.rs | 8 +- src/tool/subcommands/api_cmd.rs | 127 +++++++++++++++++++------------- 5 files changed, 90 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d741c3e65e5b..4ce46c0f6248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,9 @@ methods, fixes (notably to the garbage collection), and other improvements. - [#5028](https://github.com/ChainSafe/forest/pull/5028) Added `forest-cli f3 powertable get-proportion` CLI command. +- [#5054](https://github.com/ChainSafe/forest/pull/5054) Added `--dump-dir` + option to `forest-tool api compare` CLI command. + - [#4704](https://github.com/ChainSafe/forest/issues/4704) Add support for the `Filecoin.EthGetTransactionReceiptLimited` RPC method. diff --git a/src/rpc/client.rs b/src/rpc/client.rs index d638d8527f10..390b33cf0119 100644 --- a/src/rpc/client.rs +++ b/src/rpc/client.rs @@ -79,7 +79,7 @@ impl Client { timeout, .. } = req; - + let method_name = method_name.as_ref(); let client = self.get_or_init_client(api_paths).await?; let span = tracing::debug_span!("request", method = %method_name, url = %client.url); let work = async { diff --git a/src/rpc/reflect/mod.rs b/src/rpc/reflect/mod.rs index 1185a135428a..5d4f789a2220 100644 --- a/src/rpc/reflect/mod.rs +++ b/src/rpc/reflect/mod.rs @@ -96,11 +96,12 @@ pub enum Permission { /// Which paths should this method be exposed on? /// /// This information is important when using [`crate::rpc::client`]. -#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)] +#[derive(Debug, Default, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)] pub enum ApiPaths { /// Only expose this method on `/rpc/v0` V0, /// Only expose this method on `/rpc/v1` + #[default] V1, /// Expose this method on both `/rpc/v0` and `/rpc/v1` #[allow(dead_code)] @@ -233,7 +234,7 @@ pub trait RpcMethodExt: RpcMethod { // hardcode calling convention because lotus is by-position only let params = Self::request_params(params)?; Ok(crate::rpc::Request { - method_name: Self::NAME, + method_name: Self::NAME.into(), params, result_type: std::marker::PhantomData, api_paths: Self::API_PATHS, @@ -275,7 +276,7 @@ pub trait RpcMethodExt: RpcMethod { }; Ok(crate::rpc::Request { - method_name: name, + method_name: name.into(), params, result_type: std::marker::PhantomData, api_paths: Self::API_PATHS, diff --git a/src/rpc/request.rs b/src/rpc/request.rs index 248196c38651..c7b462b53e54 100644 --- a/src/rpc/request.rs +++ b/src/rpc/request.rs @@ -3,16 +3,20 @@ use super::ApiPaths; use jsonrpsee::core::traits::ToRpcParams; +use serde::{Deserialize, Serialize}; use std::{marker::PhantomData, time::Duration}; /// An at-rest description of a remote procedure call, created using /// [`rpc::RpcMethodExt`](crate::rpc::RpcMethodExt::request), and called using [`rpc::Client::call`](crate::rpc::Client::call). -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct Request { - pub method_name: &'static str, + pub method_name: std::borrow::Cow<'static, str>, pub params: serde_json::Value, + #[serde(skip)] pub result_type: PhantomData, + #[serde(skip)] pub api_paths: ApiPaths, + #[serde(skip)] pub timeout: Duration, } diff --git a/src/tool/subcommands/api_cmd.rs b/src/tool/subcommands/api_cmd.rs index f6cce62df45f..5bcc3cae821f 100644 --- a/src/tool/subcommands/api_cmd.rs +++ b/src/tool/subcommands/api_cmd.rs @@ -7,17 +7,16 @@ use crate::eth::{EthChainId as EthChainIdType, SAFE_EPOCH_DELAY}; use crate::lotus_json::HasLotusJson; use crate::message::{Message as _, SignedMessage}; use crate::networks::NetworkChain; +use crate::rpc; use crate::rpc::auth::AuthNewParams; use crate::rpc::beacon::BeaconGetEntry; -use crate::rpc::eth::types::{EthAddress, EthBytes}; +use crate::rpc::eth::{ + new_eth_tx_from_signed_message, types::*, BlockNumberOrHash, EthInt64, EthUint64, Predefined, +}; use crate::rpc::gas::GasEstimateGasLimit; use crate::rpc::miner::BlockTemplate; use crate::rpc::state::StateGetAllClaims; use crate::rpc::types::{ApiTipsetKey, MessageFilter, MessageLookup}; -use crate::rpc::{ - self, - eth::{types::*, *}, -}; use crate::rpc::{prelude::*, Permission}; use crate::shim::actors::market; use crate::shim::actors::MarketActorStateLoad as _; @@ -47,8 +46,9 @@ use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; use serde_json::Value; use similar::{ChangeTag, TextDiff}; -use std::io; use std::{ + borrow::Cow, + io, path::{Path, PathBuf}, str::FromStr, sync::Arc, @@ -57,7 +57,6 @@ use std::{ use tabled::{builder::Builder, settings::Style}; use tokio::sync::Semaphore; use tracing::debug; -use types::BlockNumberOrPredefined; const COLLECTION_SAMPLE_SIZE: usize = 5; @@ -139,6 +138,10 @@ pub enum ApiCommands { #[command(flatten)] create_tests_args: CreateTestsArgs, + + /// Specify a directory to which the RPC tests are dumped + #[arg(long)] + dump_dir: Option, }, DumpTests { #[command(flatten)] @@ -191,6 +194,7 @@ impl ApiCommands { run_ignored, max_concurrent_requests, create_tests_args, + dump_dir, } => { let forest = Arc::new(rpc::Client::from_url(forest)); let lotus = Arc::new(rpc::Client::from_url(lotus)); @@ -208,6 +212,7 @@ impl ApiCommands { filter.clone(), run_ignored, fail_fast, + dump_dir.clone(), ) .await?; } @@ -344,19 +349,28 @@ impl TestSummary { } /// Data about a failed test. Used for debugging. +#[derive(Debug, Clone, Serialize, Deserialize)] struct TestDump { request: rpc::Request, - forest_response: Option, - lotus_response: Option, + forest_response: Result, + lotus_response: Result, } impl std::fmt::Display for TestDump { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "Request dump: {:?}", self.request)?; writeln!(f, "Request params JSON: {}", self.request.params)?; - if let (Some(forest_response), Some(lotus_response)) = - (&self.forest_response, &self.lotus_response) - { + let (forest_response, lotus_response) = ( + self.forest_response + .as_ref() + .ok() + .and_then(|v| serde_json::to_string_pretty(v).ok()), + self.lotus_response + .as_ref() + .ok() + .and_then(|v| serde_json::to_string_pretty(v).ok()), + ); + if let (Some(forest_response), Some(lotus_response)) = (&forest_response, &lotus_response) { let diff = TextDiff::from_lines(forest_response, lotus_response); let mut print_diff = Vec::new(); for change in diff.iter_all_changes() { @@ -371,10 +385,10 @@ impl std::fmt::Display for TestDump { writeln!(f, "Lotus response: {}", lotus_response)?; writeln!(f, "Diff: {}", print_diff.join("\n"))?; } else { - if let Some(forest_response) = &self.forest_response { + if let Some(forest_response) = &forest_response { writeln!(f, "Forest response: {}", forest_response)?; } - if let Some(lotus_response) = &self.lotus_response { + if let Some(lotus_response) = &lotus_response { writeln!(f, "Lotus response: {}", lotus_response)?; } }; @@ -499,19 +513,9 @@ impl RpcTest { async fn run(&self, forest: &rpc::Client, lotus: &rpc::Client) -> TestResult { let forest_resp = forest.call(self.request.clone()).await; + let forest_response = forest_resp.as_ref().map_err(|e| e.to_string()).cloned(); let lotus_resp = lotus.call(self.request.clone()).await; - - let forest_json_str = if let Ok(forest_resp) = forest_resp.as_ref() { - serde_json::to_string_pretty(forest_resp).ok() - } else { - None - }; - - let lotus_json_str = if let Ok(lotus_resp) = lotus_resp.as_ref() { - serde_json::to_string_pretty(lotus_resp).ok() - } else { - None - }; + let lotus_response = lotus_resp.as_ref().map_err(|e| e.to_string()).cloned(); let (forest_status, lotus_status) = match (forest_resp, lotus_resp) { (Ok(forest), Ok(lotus)) @@ -550,22 +554,14 @@ impl RpcTest { } }; - if forest_status == TestSummary::Valid && lotus_status == TestSummary::Valid { - TestResult { - forest_status, - lotus_status, - test_dump: None, - } - } else { - TestResult { - forest_status, - lotus_status, - test_dump: Some(TestDump { - request: self.request.clone(), - forest_response: forest_json_str, - lotus_response: lotus_json_str, - }), - } + TestResult { + forest_status, + lotus_status, + test_dump: Some(TestDump { + request: self.request.clone(), + forest_response, + lotus_response, + }), } } } @@ -1769,7 +1765,7 @@ fn create_tests( eth_chain_id, )?); } - tests.sort_by_key(|test| test.request.method_name); + tests.sort_by_key(|test| test.request.method_name.clone()); Ok(tests) } @@ -1799,6 +1795,7 @@ async fn run_tests( filter: String, run_ignored: RunIgnored, fail_fast: bool, + dump_dir: Option, ) -> anyhow::Result<()> { let forest = Into::>::into(forest); let lotus = Into::>::into(lotus); @@ -1823,7 +1820,14 @@ async fn run_tests( }, ignore, .. - }| (*method_name, params.clone(), *api_paths, ignore.is_some()), + }| { + ( + method_name.clone(), + params.clone(), + *api_paths, + ignore.is_some(), + ) + }, ) { // By default, do not run ignored tests. if matches!(run_ignored, RunIgnored::Default) && test.ignore.is_some() { @@ -1834,7 +1838,7 @@ async fn run_tests( continue; } - if !filter_list.authorize(test.request.method_name) { + if !filter_list.authorize(&test.request.method_name) { continue; } @@ -1877,6 +1881,25 @@ async fn run_tests( } _ => false, }; + + if let (Some(dump_dir), Some(test_dump)) = (&dump_dir, &test_result.test_dump) { + let dir = dump_dir.join(if success { "valid" } else { "invalid" }); + if !dir.is_dir() { + std::fs::create_dir_all(&dir)?; + } + let filename = format!( + "{}_{}.json", + test_dump + .request + .method_name + .as_ref() + .replace(".", "_") + .to_lowercase(), + chrono::Utc::now().timestamp_micros() + ); + std::fs::write(dir.join(filename), serde_json::to_string_pretty(test_dump)?)?; + } + let result_entry = (method_name, forest_status, lotus_status); if success { success_results @@ -1884,8 +1907,8 @@ async fn run_tests( .and_modify(|v| *v += 1) .or_insert(1u32); } else { - if let Some(test_result) = test_result.test_dump { - fail_details.push(test_result); + if let Some(test_dump) = test_result.test_dump { + fail_details.push(test_dump); } failed_results .entry(result_entry) @@ -1914,8 +1937,8 @@ fn print_error_details(fail_details: &[TestDump]) { } fn print_test_results( - success_results: &HashMap<(&'static str, TestSummary, TestSummary), u32>, - failed_results: &HashMap<(&'static str, TestSummary, TestSummary), u32>, + success_results: &HashMap<(Cow<'static, str>, TestSummary, TestSummary), u32>, + failed_results: &HashMap<(Cow<'static, str>, TestSummary, TestSummary), u32>, ) { // Combine all results let mut combined_results = success_results.clone(); @@ -1929,7 +1952,8 @@ fn print_test_results( println!("{}", format_as_markdown(&results)); } -fn format_as_markdown(results: &[((&'static str, TestSummary, TestSummary), u32)]) -> String { +#[allow(clippy::type_complexity)] +fn format_as_markdown(results: &[((Cow<'static, str>, TestSummary, TestSummary), u32)]) -> String { let mut builder = Builder::default(); builder.push_record(["RPC Method", "Forest", "Lotus"]); @@ -1973,7 +1997,8 @@ impl FilterList { /// Authorize (or not) an RPC method based on its name. /// If the allow list is empty, all methods are authorized, unless they are rejected. - fn authorize(&self, entry: &str) -> bool { + fn authorize(&self, entry: impl AsRef) -> bool { + let entry = entry.as_ref(); (self.allow.is_empty() || self.allow.iter().any(|a| entry.contains(a))) && !self.reject.iter().any(|r| entry.contains(r)) }