Add tests based on mutagen outputs #399
Conversation
| @@ -131,7 +131,7 @@ pub fn from(data: &str) -> Result<Vec<u8>, Error> { | |||
| // Build in base 256 | |||
| for d58 in data.bytes() { | |||
| // Compute "X = X * 58 + next_digit" in base 256 | |||
| if d58 as usize > BASE58_DIGITS.len() { | |||
| if d58 as usize >= BASE58_DIGITS.len() { | |||
There was a problem hiding this comment.
This is the right logic because we later do BASE58_DIGITS[d58] which would panic if d58==BASE58_DIGITS.len()
But this can't really happen in practice, because BASE58_DIGITS.len() == 128 and data is &str meaning it only contains valid UTF-8. and UTF-8 don't have any valid byte starting with 128.
(it's either a single byte < 128 or multiple bytes with the first one > 128)
| d.read_slice(&mut ret)?; | ||
| Ok(ret) | ||
| } | ||
| } | ||
|
|
||
| impl Encodable for Box<[u8]> { |
There was a problem hiding this comment.
Is this one even used anywhere?
There was a problem hiding this comment.
yes. pub struct Script(Box<[u8]>); so all operations on script.0 are via the Box impl
There was a problem hiding this comment.
In fairness we might want to just do this impl directly on Script. I think I did it on Box<[u8]> so that all this mostly-boilerplate code would be in encode.rs (which is full of boilerplate code) rather than in script.rs (which is mostly actual functionality). But this isn't a strong argument.
There was a problem hiding this comment.
Already reverted this change back
removing this and implementing on Script can be done in a separate PR.
| test_varint_len(VarInt(u64::max_value()), 9); | ||
| } | ||
|
|
||
| fn test_varint_len(varint: VarInt, expected: usize) { |
There was a problem hiding this comment.
nit: declare methods before they're used
| test_len_is_max_vec::<u64>(); | ||
| } | ||
|
|
||
| fn test_len_is_max_vec<T>() where Vec<T>: Decodable, T: fmt::Debug { |
There was a problem hiding this comment.
nit: declare methods before they're used
| @@ -668,7 +668,11 @@ impl SignedAmount { | |||
| /// | |||
| /// Does not include the denomination. | |||
| pub fn fmt_value_in(&self, f: &mut fmt::Write, denom: Denomination) -> fmt::Result { | |||
| fmt_satoshi_in(self.as_sat().abs() as u64, self.is_negative(), f, denom) | |||
| let sats = self.as_sat().checked_abs().map(|a: i64| a as u64).unwrap_or_else(|| { | |||
| // We could also hard code this into `9223372036854775808` | |||
There was a problem hiding this comment.
Given that we only expect this to happen for a single value of self.as_sat(), should we not better assert it, return the fixed value and have the calculation commented as documentation?
92e5b11 to
13346b7
Compare
Codecov Report
@@ Coverage Diff @@
## master #399 +/- ##
==========================================
- Coverage 85.98% 85.81% -0.17%
==========================================
Files 40 40
Lines 8083 7461 -622
==========================================
- Hits 6950 6403 -547
+ Misses 1133 1058 -75
Continue to review full report at Codecov.
|
|
Needs rebase. |
83caf7d to
2b24d7a
Compare
2b24d7a to
07b30c7
Compare
Hi,
I used mutagen(https://github.com/llogiq/mutagen) to find places where our test don't quite check (even if they "cover" as in code coverage).
and wrote more tests to try and cover the edge cases it found.
I didn't managed to run it on all of our code because it's a lot of work so I decided to do it gradually.
For actual CI inclusion I'll wait and see how Matt will do it in lightningdevkit/rust-lightning#453
And also: llogiq/mutagen#156 llogiq/mutagen#154
I also found some bugs while writing the tests and fixed them.