test: Make AddrMan unit tests use public interface, extend coverage #23826
Conversation
|
Concept ACK based on the description |
|
Strong concept ACK! |
|
Concept ACK |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Looks great. Just minor comments inline.
mzumsande
force-pushed
the
202112_addrman_unit_tests_1
branch
from
250479a to
ce8196d
Compare
|
Thanks for the reviews! |
mzumsande
force-pushed
the
202112_addrman_unit_tests_1
branch
from
ce8196d to
5ecdaaf
Compare
josibake
left a comment
josibake
left a comment
There was a problem hiding this comment.
tACK 5ecdaaf
This is great stuff! Rebased on master and ran the unit tests for each commit, everything looks good. I had one suggestion regarding Good(). I recompiled with my changes and everything still passes - feel free to ping me for a re-ack if you decide to take the suggestion
|
utACK 5ecdaaf4d024d09fbcaa24add0de7baaa4c425de I'd be happy to rereview if you wanted to address the three remaining review comments:
|
Co-Authored-By: Martin Zumsande <[email protected]>
By updating the test to use FindEntry, it no longer needs to reach into AddrMan's internals (via GetBucketAndEntry) to assert expected behaviors.
No need for a function, since it is only used once. Co-Authored-By: Amiti Uttarwar <[email protected]>
Switches to AddrMan for tests that use no features of AddrManTest. Also removes unusued AddrManTest variables Co-Authored-By: Amiti Uttarwar <[email protected]>
The logic of these functions is already covered by existing unit tests using publicly exposed functions of the interface. Therefore, removing them does not decrease test coverage.
This covers Connected() which updates nTime, and SetServices() which updates nServices
mzumsande
force-pushed
the
202112_addrman_unit_tests_1
branch
from
5ecdaaf to
26046a1
Compare
|
I pushed an update that addresses the outstanding comments. |
|
reACK 26046a1 verified updates with |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
utACK 26046a12473ad0e342ec200b82184bc1336cf6f3
Verified range-diff. Only changes are the suggestions in #23826 (comment).
mzumsande
force-pushed
the
202112_addrman_unit_tests_1
branch
from
26046a1 to
ea4c9fd
Compare
|
Made another push, fixing a typo. Btw, this conflicts with #23373, which has been open a while longer but seems close - happy to rebase if that gets merged first. |
|
ACK ea4c9fd |
|
reACK ea4c9fd verified with git range-diff |
9 participants
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public
AddrManinterface:This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted here for using a multiindex) without having to adjust the tests.
This includes the following steps:
FindAddressEntry()to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).AddrManTestsubclass which would reach into AddrMan's internals, usingAddrManinsteadFindAddressEntry().All in all, this PR increases the unit test coverage of AddrMan by a bit.