addrman: tidy up unit tests #23477

pull jnewbery wants to merge 5 commits into bitcoin:master from jnewbery:2021-11-addrman-tidyups changing 2 files +58 −83

jnewbery commented at 5:38 PM on November 9, 2021: member

Various tidy-ups to the addrman tests.

[addrman] Remove AddrMan friends

AddrMan's friends both inherit from AddrMan, so just make the private
member protected and remove the friends.

a749fa539a

[addrman] [tests] Remove deterministic argument and member from AddrManTest
It's always set to true.
7784a9a374
[addrman] [tests] Tidy up unused arguments in addrman test functions d02098d1f0

[addrman] [tests] Remove AddrManCorrupted subclass

It's only used to create a corrupted peers.dat file. We can do that directly
in a pure function.

dfbd3a6d71

[addrman] [tests] Remove AddrManUncorrupted subclass
It doesn't do anything different from the base AddrMan class.
36d3510303
jnewbery renamed this:
~~addrman: tidy-ups to unit tests~~
addrman: tidy up unit tests
on Nov 9, 2021
DrahtBot added the label P2P on Nov 9, 2021
DrahtBot commented at 2:58 AM on November 10, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23373 (Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change by vasild)
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.
fanquake commented at 4:17 AM on November 10, 2021: member

Concept ACK
theStack commented at 12:26 PM on November 10, 2021: member

Concept ACK
shaavan approved
shaavan commented at 1:04 PM on November 10, 2021: contributor

crACK 36d3510303875c9f98eb00b28763c7c043d4dcee

The cleanup looks good. Though I am not aware of how to test these changes, but the changes done in this PR seem logically sound to me.
promag commented at 6:24 PM on November 11, 2021: member

Code review ACK 36d3510303875c9f98eb00b28763c7c043d4dcee.
in src/test/addrman_tests.cpp:84 in 7784a9a374 outdated
```
  87 | -        : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100)
  88 | -    {
  89 | -        deterministic = makeDeterministic;
  90 | -    }
  91 | +    explicit AddrManTest(std::vector<bool> asmap = std::vector<bool>())
  92 | +        : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100)
```
theStack commented at 6:31 PM on November 11, 2021:

nit, feel free to ignore:

: AddrMan(asmap, /*deterministic=*/ true, /*consistency_check_ratio=*/ 100)
jnewbery commented at 9:11 AM on November 12, 2021:

oops. I'll be more consistent with these comments in future.

MarcoFalke commented at 10:40 AM on November 12, 2021:

I think /*deterministic=*/true is the "clang-format-correct" formatting.

jnewbery commented at 12:40 PM on November 12, 2021:

Thanks Marco. That was my understanding too. I should have also formatted consistency_check_ratio in that way.
in src/test/addrman_tests.cpp:1024 in dfbd3a6d71 outdated
```
1016 | @@ -1043,13 +1017,39 @@ BOOST_AUTO_TEST_CASE(load_addrman)
1017 |      BOOST_CHECK(addrman2.size() == 3);
1018 |  }
1019 |  
1020 | +// Produce a corrupt peers.dat that claims 20 addrs when it only has one addr.
1021 | +static CDataStream MakeCorruptPeersDat()
1022 | +{
1023 | +    CDataStream s(SER_DISK, CLIENT_VERSION);
1024 | +    s << ::Params().MessageStart();
```
theStack commented at 6:34 PM on November 11, 2021:

nit, since we are already in global namespace, the scope-resolution operator could be dropped (like in AddrmanToStream above):

s << Params().MessageStart();
jnewbery commented at 9:13 AM on November 12, 2021:

I'm not sure, but I think there's a preference to be more explicit when we're calling global functions like this.

theStack commented at 12:36 PM on November 12, 2021:

Okay, my assumption was that the scope-resolution operator is only preferred if there is the possibility of namespace ambiguity (e.g. in class methods). I guess it should then also be added to Params() in AddrmanToStream to be consistent. Again, not a big deal, but maybe something to include in potential follow-up PRs.
theStack approved
theStack commented at 6:38 PM on November 11, 2021: member

Code-review ACK 36d3510303875c9f98eb00b28763c7c043d4dcee

Left two small non-critical suggestions below.
MarcoFalke merged this on Nov 12, 2021
MarcoFalke closed this on Nov 12, 2021
jnewbery deleted the branch on Nov 12, 2021
MarcoFalke removed the label P2P on Nov 12, 2021
MarcoFalke added the label Tests on Nov 12, 2021
sidhujag referenced this in commit 82c0a63027 on Nov 12, 2021
DrahtBot locked this on Nov 12, 2022

Contributors

Labels

addrman: tidy up unit tests #23477

Conflicts