refactor, test: update addrman_tests.cpp to use output from AddrMan::Good()
#23780
Conversation
josibake
force-pushed
the
josibake-addrman-test-refactors
branch
from
0f1806a to
93c58db
Compare
Check `Good()` directly when adding addresses. Previously, test would check `size()`, which is incorrect. Check that duplicates are also handled by checking the output from `SelectTriedCollision()` when `Good()` returns false.
Check the response from `Good()` wherever it is called. Previously, the test was using `size()` (incorrect for checking tried) and `SelectTriedCollision()` to determine if a collision happened.
Test for collisions and duplicates directly with `Good()`. If an entry to tried is a duplicate, `Good()` will return false but `SelectTriedCollision()` will be empty (assuming there were no prior collisions). If there is a collision, `Good()` will retun false and `SelectTriedCollision()` will return a value.
Check that `Good()` is successful whenever it is called.
josibake
force-pushed
the
josibake-addrman-test-refactors
branch
from
93c58db to
bf4f817
Compare
| // No collisions yet. | ||
| BOOST_CHECK(addrman.size() == i); | ||
| // No collisions in tried. | ||
| BOOST_CHECK(addrman.Good(addr)); |
There was a problem hiding this comment.
This test seems very limited, because the output is always "[::]:0".
But since reviewing this change requires revisiting this logic, maybe might as well improve it to test broader behavior?
There was a problem hiding this comment.
I agree, this test is very limited. Ideally, it would:
- create a collision and test that
SelectTriedCollision()returns a value - resolve collisions
- try to enter a duplicate and test that
SelectTriedCollision()does not return a value
I didn't make those changes because I was trying to keep the scope of the PR limited to just refactoring the usage of Good(), but perhaps you're right and it's better to improve the test while refactoring? I can update the PR description to be a bit more general
There was a problem hiding this comment.
Yeah, your intention would usually make sense, but here I found myself digging into the addrman logic while reviewing.
I could be easier on review i guess :)
There was a problem hiding this comment.
The more complicated scenarios are covered in the subsequent tests addrman_noevict and addrman_evictionworks. I think that's fine - maybe just the naming of addrman_selecttriedcollision is not ideal because it suggests it covers all about selecttriedcollision that there is to test.
There was a problem hiding this comment.
@mzumsande yeah i agree, it's more of a naming problem
|
ACK bf4f817 |
|
Code Review ACK bf4f817 I like the improved comments - before, it was a bit hard to understand what the tried collision tests were doing. |
6 participants
As a follow-up to #23713 , this PR refactors the remaining tests in
src/tests/addrman_tests.cppto use the output fromAddrMan::Good()where appropriate.