-
Notifications
You must be signed in to change notification settings - Fork 159
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(tool): forest-tool api generate-test-snapshot #5074
base: main
Are you sure you want to change the base?
Conversation
7696c1a
to
f496aed
Compare
f496aed
to
91e4716
Compare
…m/forest-tool-api-generate-test-snapshot
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.
The format of the memory-db is not stable. If someone changes the memory-db internals, this code will fail. We'll either have to deal with it right now or in the future. I vote to deal with it right now.
Deploying forest-docs with Cloudflare Pages
|
@lemmih Switched to Forest CAR DB as discussed. |
827e63a
to
18089eb
Compare
…nerate-test-snapshot
…nerate-test-snapshot
pub async fn export_forest_car<W: tokio::io::AsyncWrite + Unpin>( | ||
&self, | ||
writer: &mut W, | ||
) -> anyhow::Result<()> { | ||
let roots = | ||
SettingsStoreExt::read_obj::<TipsetKey>(self, crate::db::setting_keys::HEAD_KEY)? | ||
.context("chain head is not tracked and cannot be exported")? | ||
.into_cids(); | ||
let blocks = { | ||
let blockchain_db = self.blockchain_db.read(); | ||
let blockchain_persistent_db = self.blockchain_persistent_db.read(); | ||
blockchain_db | ||
.iter() | ||
.chain(blockchain_persistent_db.iter()) | ||
.map(|(&cid, data)| { | ||
anyhow::Ok(CarBlock { | ||
cid, | ||
data: data.clone(), | ||
}) | ||
}) | ||
.collect_vec() | ||
}; | ||
let frames = | ||
crate::db::car::forest::Encoder::compress_stream_default(futures::stream::iter(blocks)); | ||
crate::db::car::forest::Encoder::write(writer, roots, frames).await | ||
} | ||
} |
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.
Can we contrive a unit test for this functionality?
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.
Sure, unit test added.
#[arg(num_args = 1.., required = true)] | ||
test_dump_files: Vec<PathBuf>, |
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.
Let's have docs for this as well.
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.
Fixed.
#[arg(num_args = 1.., required = true)] | ||
files: Vec<PathBuf>, |
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.
Let's have docs for this as well.
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.
Fixed.
CurrentNetwork::set_global(Network::Testnet); | ||
} | ||
let mut run = false; | ||
let chain_config = Arc::new(ChainConfig::calibnet()); |
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.
The chain config should depend on the chain
parameter, no?
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.
Good catch, fixed!
} | ||
|
||
fn setting_keys(&self) -> anyhow::Result<Vec<String>> { | ||
// HACKHACK: may need some care |
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.
It would be great to elaborate on why the care might be needed as a comment.
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.
Resolved or removed all HACKHACKs, methods that read settings are no longer supported after migrating DB snapshot to CAR format.
#[tokio::test] | ||
async fn rpc_regression_tests() { | ||
let urls = include_str!("test_snapshots.txt") | ||
.trim() | ||
.split("\n") | ||
.filter_map(|n| { | ||
Url::parse( | ||
format!("https://forest-snapshots.fra1.digitaloceanspaces.com/rpc_test/{n}") | ||
.as_str(), | ||
) | ||
.ok() | ||
}) | ||
.collect_vec(); | ||
for url in urls { | ||
print!("Testing {url} ..."); | ||
let tmp_dir = tempfile::tempdir().unwrap(); | ||
let tmp = tempfile::NamedTempFile::new_in(&tmp_dir) | ||
.unwrap() | ||
.into_temp_path(); | ||
println!("start downloading at {}", tmp.display()); | ||
download_to(&url, &tmp).await.unwrap(); | ||
println!("done downloading {}", tmp.display()); | ||
run_test_from_snapshot(&tmp).await.unwrap(); | ||
println!(" succeeded."); | ||
} |
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.
If I understand correctly, each test invocation will temporarily download the snapshots. The total snapshot size currently is around 4 MB, but I can easily see it going higher once we add more snapshots. This will be a pain for someone on a worse connection just trying to run tests.
Perhaps we could add something like /.local/share/forest-dev/rpc-regression-snapshots
persistent path (it would vary from system to system, we could re-use the logic we have for the DB).
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.
Cache implemented.
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.
Great stuff
75caaf6
to
c469b88
Compare
Summary of changes
This PR includes changes in #5034
Changes introduced in this pull request:
forest-tool api generate-test-snapshot
forest-tool api test
(Will use a separate PR to add below steps to dev docs)
Step 1: generating RPC test dumps
forest-tool api compare --dump-dir /var/tmp/rpc-dumps ...
Step 2: generate RPC test snapshots
forest-tool api generate-test-snapshot --db ~/.local/share/forest/calibnet/0.23.0 --chain calibnet --out-dir /var/tmp/rpc-snapshots /var/tmp/test-dumps/filecoin_stategetallallocations*.json
Step 2(b): zstd compress test snapshots
Step 3: verify test snapshots
forest-tool api test /var/tmp/rpc-snapshots/*.json
orforest-tool api test /var/tmp/rpc-snapshots/*.zst
Step 3(b): update unit test
test_snapshots.txt
cargo test --lib -- --test rpc_regression_tests --nocapture
Reference issue to close (if applicable)
Closes #5034
Other information and links
Change checklist