add mock testing for lite client #72
Conversation
tendermint/src/lite.rs
Outdated
| validator::Set::new(validators), | ||
| ) | ||
| .expect("verify_trusting failed"); | ||
| } |
There was a problem hiding this comment.
Very cool! Thanks. I'm wondering if we either should have simpler rust-only tests first. Or, simplify this (extract all json strings to vars/constants and have only the verification logic in the test). Also, there should be a comment explaining how the JSON was generated.
Furthermore, @Shivani912 is working on generating similar tests: https://github.com/Shivani912/tendermint/tree/shivani/language-agnostic-tests/types/lite-client/tests
There was a problem hiding this comment.
I simply copy-pasted my local test node's json rpc response, only to test the basics work.
It's great to have more sophisticated test cases.
There was a problem hiding this comment.
I think, these kind of tests currently tend to live in https://github.com/interchainio/tendermint-rs/tree/master/tendermint/tests as JSON files: https://github.com/interchainio/tendermint-rs/tree/master/tendermint/tests/support/rpc
I think it would increase the readability here if we'd extract the JSON into vars in the test (e.g. static VALID_HEADER: &str = r#"{ ...}"#
The test seems to fail on my machine btw.
tendermint/src/lite.rs
Outdated
| "parts": { | ||
| "total": "0", | ||
| "hash": "" | ||
| } |
There was a problem hiding this comment.
This will panic:
https://github.com/interchainio/tendermint-rs/blob/ef7410c184c736f032f24296c24cf921175582be/tendermint/src/amino_types/block_id.rs#L36-L37
called e.g. by:
https://github.com/interchainio/tendermint-rs/blob/ef7410c184c736f032f24296c24cf921175582be/tendermint/src/block/header.rs#L106
There was a problem hiding this comment.
This is a bug fixed in another PR, #66. block_id could be empty.
and correctly compute header hash when block id is empty. Represent empty Hash with Option
yihuang
force-pushed
the
mock_testing_lite_client
branch
from
a1a4d97
to
9e00e99
Compare
|
There was a problem hiding this comment.
It's a bit confusing that the changes from 8b1d3dc show up as changes here although #66 was already merged into lite_impl_simple_merkle_merged.
Thanks again @yihuang! LGTM!
| @@ -0,0 +1,78 @@ | |||
| { | |||
| "signed_header": { | |||
There was a problem hiding this comment.
Thanks for moving the test JSON here 👌
Add some mocking data to test lite client.