test: Make AddrMan unit tests use public interface, extend coverage #23826

mzumsande · 2021-12-20T14:19:35Z

This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public AddrMan interface:
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:

Adding a test-only function 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).
Removal of the AddrManTest subclass which would reach into AddrMan's internals, using AddrMan instead
Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of FindAddressEntry().

All in all, this PR increases the unit test coverage of AddrMan by a bit.

jamesob · 2021-12-20T14:36:28Z

Concept ACK based on the description

jnewbery · 2021-12-20T14:55:32Z

Strong concept ACK!

glozow · 2021-12-20T14:58:02Z

Concept ACK

DrahtBot · 2021-12-21T03:12:39Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23807 (p2p: Remove GetAdjustedTime() from AddrMan by w0xlt)
#23373 (test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change by vasild)
#22910 ([RFC] Encapsulate asmap in NetGroupManager by jnewbery)

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

Looks great. Just minor comments inline.

src/addrman.cpp

src/addrman.h

src/addrman.cpp

src/addrman.h

src/addrman.cpp

src/test/addrman_tests.cpp

w0xlt

tACK 250479a

src/addrman.h

mzumsande · 2021-12-23T16:57:43Z

Thanks for the reviews!
@jnewbery I believe I have addressed all your points in my push.

josibake

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

src/test/addrman_tests.cpp

jnewbery · 2021-12-28T12:22:35Z

utACK 5ecdaaf

I'd be happy to rereview if you wanted to address the three remaining review comments:

doxygen comment for FindAddressEntry() incorrectly refers to nullptr instead of nullopt (test: Make AddrMan unit tests use public interface, extend coverage #23826 (comment))
magic value 400 can be commented (test: Make AddrMan unit tests use public interface, extend coverage #23826 (comment))
return value of Good() can be checked (test: Make AddrMan unit tests use public interface, extend coverage #23826 (comment))

Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>

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 <amiti@uttarwar.org>

Switches to AddrMan for tests that use no features of AddrManTest. Also removes unusued AddrManTest variables Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>

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 · 2021-12-28T21:00:24Z

I pushed an update that addresses the outstanding comments.

josibake · 2021-12-29T12:38:52Z

reACK 26046a1

verified updates with git range-diff 5ecdaaf...26046a1

jnewbery

utACK 26046a1

Verified range-diff. Only changes are the suggestions in #23826 (comment).

src/test/addrman_tests.cpp

mzumsande · 2022-01-03T21:36:39Z

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.

jnewbery · 2022-01-04T10:30:54Z

ACK ea4c9fd

josibake · 2022-01-04T10:44:34Z

reACK ea4c9fd

verified with git range-diff

…ce, ext…

fanquake added the Tests label Dec 20, 2021

This was referenced Dec 21, 2021

p2p: Remove GetAdjustedTime() from AddrMan #23807

Closed

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

Merged

jnewbery reviewed Dec 21, 2021

View reviewed changes

This was referenced Dec 22, 2021

p2p, rpc, test: expose tried and refcount in getnodeaddresses, update/improve tests #23035

Closed

net: Encapsulate asmap in NetGroupManager #22910

Merged

w0xlt approved these changes Dec 23, 2021

View reviewed changes

src/addrman.h Show resolved Hide resolved

mzumsande force-pushed the 202112_addrman_unit_tests_1 branch from 250479a to ce8196d Compare December 23, 2021 16:13

mzumsande force-pushed the 202112_addrman_unit_tests_1 branch from ce8196d to 5ecdaaf Compare December 23, 2021 17:00

josibake approved these changes Dec 27, 2021

View reviewed changes

src/test/addrman_tests.cpp Outdated Show resolved Hide resolved

src/test/addrman_tests.cpp Outdated Show resolved Hide resolved

amitiuttarwar and others added 9 commits December 28, 2021 17:26

addrman: Introduce a test-only function to lookup addresses

dad5f76

Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>

test: Update addrman_serialization unit test to use AddrMan's interface

2ba1e74

By updating the test to use FindEntry, it no longer needs to reach into AddrMan's internals (via GetBucketAndEntry) to assert expected behaviors.

test: delete unused GetBucketAndEntry function

5b7aac3

test: Inline SimConnFail function

1c65d42

No need for a function, since it is only used once. Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>

test: use AddrMan instead of AddrManTest where possible

0538520

Switches to AddrMan for tests that use no features of AddrManTest. Also removes unusued AddrManTest variables Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>

test: Remove tests for internal helper functions

b696d78

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.

test: Remove unused AddrManTest class

f0e5efb

test: introduce utility function to retrieve an addrman

f02eee8

test: Add test for updating addrman entries

e880bb7

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 December 28, 2021 20:57

jnewbery reviewed Dec 29, 2021

View reviewed changes

src/test/addrman_tests.cpp Outdated Show resolved Hide resolved

test: Add test for multiplicity in addrman new tables

4f1bb46

test: Cover eviction by timeout in addrman_evictionworks

ea4c9fd

mzumsande force-pushed the 202112_addrman_unit_tests_1 branch from 26046a1 to ea4c9fd Compare January 3, 2022 21:27

fanquake merged commit 4ee7845 into bitcoin:master Jan 4, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 4, 2022

Merge bitcoin#23826: test: Make AddrMan unit tests use public interfa…

ae95672

…ce, ext…

bitcoin locked and limited conversation to collaborators Jan 4, 2023

mzumsande deleted the 202112_addrman_unit_tests_1 branch January 16, 2023 18:47

test: Make AddrMan unit tests use public interface, extend coverage #23826

test: Make AddrMan unit tests use public interface, extend coverage #23826

Uh oh!

Conversation

mzumsande commented Dec 20, 2021

Uh oh!

jamesob commented Dec 20, 2021

Uh oh!

jnewbery commented Dec 20, 2021

Uh oh!

glozow commented Dec 20, 2021

Uh oh!

DrahtBot commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Dec 23, 2021

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 28, 2021

Uh oh!

mzumsande commented Dec 28, 2021

Uh oh!

josibake commented Dec 29, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Jan 3, 2022

Uh oh!

jnewbery commented Jan 4, 2022

Uh oh!

josibake commented Jan 4, 2022

Uh oh!

Uh oh!

DrahtBot commented Dec 21, 2021 •

edited

Loading