addrman: tidy up unit tests #23477
addrman: tidy up unit tests #23477
Conversation
AddrMan's friends both inherit from AddrMan, so just make the private member protected and remove the friends.
…anTest It's always set to true.
It's only used to create a corrupted peers.dat file. We can do that directly in a pure function.
It doesn't do anything different from the base AddrMan class.
|
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. |
|
Concept ACK |
|
Concept ACK |
| deterministic = makeDeterministic; | ||
| } | ||
| explicit AddrManTest(std::vector<bool> asmap = std::vector<bool>()) | ||
| : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100) |
There was a problem hiding this comment.
nit, feel free to ignore:
| : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100) | |
| : AddrMan(asmap, /*deterministic=*/ true, /*consistency_check_ratio=*/ 100) |
There was a problem hiding this comment.
oops. I'll be more consistent with these comments in future.
There was a problem hiding this comment.
I think /*deterministic=*/true is the "clang-format-correct" formatting.
There was a problem hiding this comment.
Thanks Marco. That was my understanding too. I should have also formatted consistency_check_ratio in that way.
| static CDataStream MakeCorruptPeersDat() | ||
| { | ||
| CDataStream s(SER_DISK, CLIENT_VERSION); | ||
| s << ::Params().MessageStart(); |
There was a problem hiding this comment.
nit, since we are already in global namespace, the scope-resolution operator could be dropped (like in AddrmanToStream above):
| s << ::Params().MessageStart(); | |
| s << Params().MessageStart(); |
There was a problem hiding this comment.
I'm not sure, but I think there's a preference to be more explicit when we're calling global functions like this.
There was a problem hiding this comment.
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.
7 participants
Various tidy-ups to the addrman tests.