Page MenuHomePhabricator

Add unit tests for CSeederNode::ProcessMessage()

Authored by nakihito on Nov 13 2019, 01:13.



Adds some unit tests for the CSeederNode class by using a test class wrapper
for CSeederNode.

Depends on D4449

Test Plan
make check
cmake -GNinja ..
ninja check-bitcoin-seeder

../configure CXX=clang++ CC=clang --with-gui=no
make check
cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -GNinja ..
ninja check-bitcoin-seeder

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox requested changes to this revision.Feb 10 2020, 22:23
jasonbcox added inline comments.
17 ↗(On Diff #16231)


23 ↗(On Diff #16231)


83 ↗(On Diff #16231)

These above 5 lines are reused throughout the tests unmodified. All of the tests would be much more concise if these were delegated to a static function.

139 ↗(On Diff #16231)

Nit: When the constant is defined away from the implementation or test, it's best to refer to the constant name instead of the value, since the value is subject to change and this comment would become stale.

147 ↗(On Diff #16231)

Setup is identical to the above test, so try rolling this test into the previous one similar to what you did in D4418

This revision now requires changes to proceed.Feb 10 2020, 22:23
23 ↗(On Diff #16231)

This is necessary to successfully compile due to some translation functions. You can see it on line 24 in seeder/main.cpp, for example. Its in multiple other files.

Squashed some tests together, extracted some setup code, and fixed some comments.

jasonbcox requested changes to this revision.Feb 12 2020, 01:43

Looking much better. Please review the includes, because I don't want to dig into all of them.

8 ↗(On Diff #16241)

This doesn't appear to be used.

9 ↗(On Diff #16241)


36 ↗(On Diff #16241)

Nit: CreateDummyService is sufficient. Sure, it has side effects, but the AndAddress part doesn't really add much IMO.

This revision now requires changes to proceed.Feb 12 2020, 01:43

Removed excess includes and changed function name.

Fabien requested changes to this revision.Feb 13 2020, 08:21

Looking at the test cases exhibits a common and repetitive initialization pattern.
You might want to create a test fixture to avoid this boilerplate.

57 ↗(On Diff #16320)

User agent is a string, not an integer

This revision now requires changes to proceed.Feb 13 2020, 08:21

Changed BOOST_AUTO_TEST_SUITE to BOOST_FIXTURE_TEST_SUITE and BOOST_AUTO_TEST_CASE to BOOST_FIXTURE_TEST_CASE. Added SeederTestingSetup struct for the fixture test suite. Changed user_agent type to std::string.

Fabien requested changes to this revision.Feb 17 2020, 11:03

Since I completely failed to understand what the ProcessMessage() is supposed to do/return by looking at the test, I looked at the code and my understanding is that false is not a status output here (it doesn't indicate a failure).
If I'm right, then you really need to leverage your wrapper TestProcessMessage() to give some sense to the test.
If false is a success, then add some static enum to the test node class and pass it as the expected result to the TestProcessMessage(), moving the check to the wrapper as well.

30 ↗(On Diff #16369)

The explicit keyword is superfluous.

32 ↗(On Diff #16369)

Nit: might pick a more suitable name (bitcoin.test or whatever).

36 ↗(On Diff #16369)

These local variables are useless since they are only used once:

service = {ip, SERVICE_PORT};
vAddr.emplace_back(service, ServiceFlags());
88 ↗(On Diff #16369)

Since you already attached the fixture to the test suite you don't need to specify the fixture for each test case.

89 ↗(On Diff #16369)

Any reason to keep the test node out of the fixture ? It's constructed the same for every test case.

98 ↗(On Diff #16369)

What are you testing ? I don't get what is wrong with this message. Furthermore it's only testing the failure case but not success, making it impossible to guess.

107 ↗(On Diff #16369)


117 ↗(On Diff #16369)

There is some logic issue here. How can the message cause a failure and the address get added anyway ? If this is a bug then you need to fix it.

This revision now requires changes to proceed.Feb 17 2020, 11:03

Changed FIXTURE test cases to AUTO, renamed some variables and constants, added testNode to the fixture setup, increased the functionality of the wrapper class.

Add enum to abstract return value. Removed branching logic in wrapper test function.

Fabien requested changes to this revision.Feb 22 2020, 12:37

There are 2 remaining things to fix:

  • Your comments about VADDR_SOFT_CAP are inconsistent (or I misunderstood and then they are unclear). What is the limit, VADDR_SOFT_CAP or VADDR_SOFT_CAP + 1 ?
  • Your address test looks good, but the others are simply not testing anything. It is easy to demonstrate: you can change any value in the frame and the test will always pass.
This revision now requires changes to proceed.Feb 22 2020, 12:37

Adjusted comments related to ADDR_SOFT_CAP, removed verack message test, and added additional check for the version test.

Fabien requested changes to this revision.Feb 26 2020, 15:02
Fabien added inline comments.
66 ↗(On Diff #16508)

I have a build failure with clang here, and solved it with BOOST_CHECK_EQUAL(ret, bool(addrCap));

This revision now requires changes to proceed.Feb 26 2020, 15:02

Adjusted ADDR_SOFT_CAP comments, refined addr message test with advice from Fabien, removed smurf naming of AddrCapacity enum values.

Fabien added inline comments.
131 ↗(On Diff #16531)

I still think that Full is inaccurate here, as you can still add new addresses, only the addition rate is limited to 1. Same goes for ADDR_SOFT_CAP, I understand cap as a boundary but maybe I'm wrong with this one.

This revision is now accepted and ready to land.Feb 27 2020, 07:22

AddrCapacity -> AddrAdditionRate, NotFull -> Unlimited, Full -> Limited.

Discussed with Jason offline, changed enum to better reflect the return value's purpose.