Insert test #328
Insert test #328
Conversation
| } | ||
|
|
||
| #[test] | ||
| fn insert_and_retrieve_multiple() { |
There was a problem hiding this comment.
I guess I should explain why this is necessary, what do you think?
There was a problem hiding this comment.
Yes. I would also argue that the prior test should either be marked smoke or removed as it is fully tested by this test case.
There was a problem hiding this comment.
🤔, you're absolutely right, however, by the same argument, one could say that many unit tests are actually covered by integration tests and are therefore unnecessary.
There are three separate outcomes when looking at these tests together:
- both tests pass (everything works, hurray)
- the
_multipletest fails and the other tests pass (something is wrong with multiple insertion specifically, it's probably something to do with splitting nodes) - both tests fail (nothing is working)
And with that said, I've realized that this test needs to be altered as it would not cause a split, but I am going to do that in the next PR!
| s.rehash(); | ||
| s | ||
| } | ||
| } |
There was a problem hiding this comment.
Constructors leaf and branch should be used instead of Node::new (deleted below), but when decoding an encoded node, we're left with the arbitrary NodeType in some places. This is an easy way to convert between types.
In general, there's a really bad pattern used here that doesn't work that well with Rust. Whatever needs to do the locking should wrap the types they need locked instead of having wrapper types with locks. So eventually, NodeType will be completely refactored into Node and only the Cache should manage locks. But that's all for another day.
There was a problem hiding this comment.
Let's get this comment into the code with a TODO
| } | ||
|
|
||
| #[test] | ||
| fn insert_and_retrieve_multiple() { |
There was a problem hiding this comment.
Yes. I would also argue that the prior test should either be marked smoke or removed as it is fully tested by this test case.
| s.rehash(); | ||
| s | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's get this comment into the code with a TODO
firewood/src/merkle/node.rs
Outdated
| } | ||
|
|
||
| pub fn branch( | ||
| repeated_disk_adderss: usize, |
There was a problem hiding this comment.
No, haven't you ever heard of an adderss? 😅
| }; | ||
|
|
||
| pub const TRIE_HASH_LEN: usize = 32; | ||
| const U64_TRIE_HASH_LEN: u64 = TRIE_HASH_LEN as u64; |
There was a problem hiding this comment.
Not sure I love this. This is just because in some places we are using the wrong type, I think. I'd prefer usize over u64 and use that everywhere.
There was a problem hiding this comment.
You're not sure? I hate it haha... but I think it's better than what was there before. This is all due to some shale madness though. When shale finally only deals with usize, the compiler will help guide us through where changes need to be made and this code will disappear. At the very least, this const isn't pub, so the bad code is isolated.
Co-authored-by: Ron Kuris <ron.kuris@avalabs.org> Signed-off-by: Richard Pringle <rpring9@gmail.com>
richardpringle
force-pushed
the
insert-test
branch
from
382ea97
to
588dfd5
Compare
I got a little carried away cleaning things up, but now it's a lot easier to add new tests