refactor, test: update `addrman_tests.cpp` to use output from `AddrMan::Good()` #23780

pull josibake wants to merge 4 commits into bitcoin:master from josibake:josibake-addrman-test-refactors changing 1 files +34 −39

josibake commented at 12:03 PM on December 15, 2021: member

As a follow-up to #23713 , this PR refactors the remaining tests in src/tests/addrman_tests.cpp to use the output from AddrMan::Good() where appropriate.
fanquake added the label Needs rebase on Dec 15, 2021
josibake force-pushed on Dec 15, 2021

refactor: addrman_selecttriedcollisions test

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.

8bdd9240d4

refactor: addrman_noevict test

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.

e281fccd8a

refactor: addrman_evictionworks test

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.

5a64dc018c

refactor: addrman_select test

Check that `Good()` is successful whenever it is called.

bf4f817135

josibake force-pushed on Dec 15, 2021
DrahtBot removed the label Needs rebase on Dec 15, 2021
DrahtBot added the label Tests on Dec 15, 2021
in src/test/addrman_tests.cpp:821 in 8bdd9240d4 outdated
```
 815 | @@ -816,19 +816,21 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
 816 |      for (unsigned int i = 1; i < 23; i++) {
 817 |          CService addr = ResolveService("250.1.1." + ToString(i));
 818 |          BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source));
 819 | -        addrman.Good(addr);
 820 |  
 821 | -        // No collisions yet.
 822 | -        BOOST_CHECK(addrman.size() == i);
 823 | +        // No collisions in tried.
 824 | +        BOOST_CHECK(addrman.Good(addr));
```
naumenkogs commented at 9:56 AM on December 16, 2021:

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?
josibake commented at 3:37 PM on December 16, 2021:

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
naumenkogs commented at 8:52 AM on December 17, 2021:

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 :)

mzumsande commented at 8:10 PM on December 19, 2021:

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.

naumenkogs commented at 7:20 AM on December 20, 2021:

@mzumsande yeah i agree, it's more of a naming problem
naumenkogs commented at 8:58 AM on December 17, 2021: member

ACK bf4f8171352e9b384b42c91da61dfc9c3ca89ed8
mzumsande commented at 8:17 PM on December 19, 2021: contributor

Code Review ACK bf4f8171352e9b384b42c91da61dfc9c3ca89ed8

I like the improved comments - before, it was a bit hard to understand what the tried collision tests were doing.
maflcko merged this on Dec 20, 2021
maflcko closed this on Dec 20, 2021
sidhujag referenced this in commit cd51051fe2 on Dec 20, 2021
bitcoin locked this on Dec 20, 2022
josibake deleted the branch on Jan 26, 2024

Contributors

Labels

Linked (view graph)

#23713 refactor, test: refactor addrman_tried_collisions test to directly check for collisions