Adds some unit tests for the CSeederNode class by using a test class wrapper
for CSeederNode.
Depends on D4449
Details
Details
- Reviewers
deadalnix Fabien jasonbcox - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGINGed4ca8ee11f0: Add unit tests for CSeederNode::ProcessMessage()
rABCed4ca8ee11f0: Add unit tests for CSeederNode::ProcessMessage()
../configure 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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- TestProcessMessage
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 9588 Build 17078: Default Diff Build & Tests Build 17077: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| src/Makefile.seedertest.include | ||
|---|---|---|
| 17 ↗ | (On Diff #16231) | Is LIBUNIVALUE needed? |
| src/seeder/test/seeder_tests.cpp | ||
| 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 |
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 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. |
Comment Actions
Looking much better. Please review the includes, because I don't want to dig into all of them.
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 8 ↗ | (On Diff #16241) | This doesn't appear to be used. |
| 9 ↗ | (On Diff #16241) | ditto |
| 36 ↗ | (On Diff #16241) | Nit: CreateDummyService is sufficient. Sure, it has side effects, but the AndAddress part doesn't really add much IMO. |
Comment Actions
Looking at the test cases exhibits a common and repetitive initialization pattern.
You might want to create a test fixture to avoid this boilerplate.
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 57 ↗ | (On Diff #16320) | User agent is a string, not an integer |
Comment Actions
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.
Comment Actions
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.
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 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) | Dito. |
| 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. |
Comment Actions
Changed FIXTURE test cases to AUTO, renamed some variables and constants, added testNode to the fixture setup, increased the functionality of the wrapper class.
Comment Actions
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.
Comment Actions
Adjusted comments related to ADDR_SOFT_CAP, removed verack message test, and added additional check for the version test.
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 66 ↗ | (On Diff #16508) | I have a build failure with clang here, and solved it with BOOST_CHECK_EQUAL(ret, bool(addrCap)); |
Comment Actions
Adjusted ADDR_SOFT_CAP comments, refined addr message test with advice from Fabien, removed smurf naming of AddrCapacity enum values.
| src/seeder/test/seeder_tests.cpp | ||
|---|---|---|
| 131 | 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. | |
Comment Actions
Discussed with Jason offline, changed enum to better reflect the return value's purpose.