Enable integration tests + GitHub Actions Service for Tendermint node #183
Conversation
greg-szabo
commented
Mar 16, 2020
greg-szabo
commented
- Referenced an issue explaining the need for the change. Issue: Run a tendermint node in CI to test the RPC & light client #120
- Updated all relevant documentation in docs
- Updated all code comments where relevant
- Wrote tests
- Updated CHANGES.md
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
=========================================
- Coverage 42.05% 38.4% -3.65%
=========================================
Files 88 97 +9
Lines 3051 3497 +446
Branches 470 552 +82
=========================================
+ Hits 1283 1343 +60
- Misses 1431 1801 +370
- Partials 337 353 +16
Continue to review full report at Codecov.
|
| where | ||
| S: Serializer, | ||
| { | ||
| String::from_utf8(base64::encode(hash.as_bytes())) | ||
| String::from_utf8(base64::encode(hash.unwrap().as_bytes())) |
There was a problem hiding this comment.
Won't this panic if hash is None?
There was a problem hiding this comment.
That's an interesting point. The normal execution in serde is that if the parameter is missing or None and you have the Default trait implemented, then this serializer function is not run. The Default implementation takes over and assumes whatever value is defined there.
There was a problem hiding this comment.
There is definitely something fishy about this 🤔 I think the method should err if hash is None which is possible. At least the type system allows that even if used as intended this won't happen.
There was a problem hiding this comment.
That's why we have the skip_serializing_if = "Option::is_none" added. If hash is None, serde will not call the function.
| @@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize}; | |||
| #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] | |||
| pub struct Request { | |||
| /// Path to the data | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
| path: Option<Path>, | |||
There was a problem hiding this comment.
Surprising that this can be empty. But yeah, in JSON everything seems possible :-D
There was a problem hiding this comment.
It's the Tendermint Go code that's surprising in most of these cases. Some of their definitions don't seem to make sense to me, but if we want to be compatible with a full node, we have to follow them.
| )] | ||
| pub last_block_app_hash: Hash, | ||
| pub last_block_app_hash: Option<Hash>, |
There was a problem hiding this comment.
Similar to above: This can only be empty on genesis, right? Maybe add a comment.
There was a problem hiding this comment.
Nope, abci_info doesn't print this over RPC.
There was a problem hiding this comment.
I think it makes sense to merge this as is and capture relevant comments in an issue.
Thanks a lot @greg-szabo !
|
ref: #71 |
