Page MenuHomePhabricator

Add unit tests for CSeederNode::ProcessMessage()
Changes PlannedPublic

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

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

Depends on D4418 and D4449

Test Plan
make check
ninja check-bitcoin-seeder

Diff Detail

Repository
rABC Bitcoin ABC
Branch
TestProcessMessage
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

nakihito created this revision.Wed, Nov 13, 01:13
Owners added a reviewer: Restricted Owners Package.Wed, Nov 13, 01:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Nov 13, 01:13
nakihito planned changes to this revision.Wed, Nov 13, 01:13
nakihito updated this revision to Diff 14107.Wed, Nov 13, 19:53

Cleaned up TestCSeederNode and rebase.

nakihito planned changes to this revision.Thu, Nov 14, 00:52
nakihito updated this revision to Diff 14118.Thu, Nov 14, 03:58

Split tests into smaller parts and added more constants.

nakihito planned changes to this revision.Thu, Nov 14, 03:58
nakihito updated this revision to Diff 14119.Thu, Nov 14, 04:21

Changed static_cast into a functional cast.

nakihito planned changes to this revision.Thu, Nov 14, 04:21
nakihito requested review of this revision.Thu, Nov 14, 04:41
jasonbcox requested changes to this revision.Thu, Nov 14, 17:35
jasonbcox added inline comments.
src/seeder/test/seeder_tests.cpp
169 ↗(On Diff #14119)

This looks like a bug in the making since forgetting to reset these can produce unexpected results. message and header are only used on a per-message basis, so why are they class members?

263 ↗(On Diff #14119)

I don't know what version 10300 is or why it's useful to test. Please give it a useful name.

276 ↗(On Diff #14119)

ditto

This revision now requires changes to proceed.Thu, Nov 14, 17:35
jasonbcox added inline comments.Thu, Nov 14, 18:25
src/seeder/test/seeder_tests.cpp
323 ↗(On Diff #14119)

This test is hard to follow because the seeder is sending a message to itself with the vAddr list that it already has. This "works" but the intent is impossible to follow. The test should explicitly send some number of addresses to the seeder and test against that expectation. Otherwise you look at this test with no way of knowing what the right behavior should be.

337 ↗(On Diff #14119)

ditto

nakihito added inline comments.Fri, Nov 15, 06:29
src/seeder/test/seeder_tests.cpp
263 ↗(On Diff #14119)

Turns out the path this is testing can be removed after D4449 is landed.

nakihito updated this revision to Diff 14142.Fri, Nov 15, 06:29

Rebased to included changes from D4449 which allow the removal of a test. Also renamed a test and changed addr message tests to specify how many addrs to include in the addr message among other concerns.

nakihito added inline comments.Fri, Nov 15, 06:32
src/seeder/test/seeder_tests.cpp
50 ↗(On Diff #14142)

Should be "...one more address from an addr message." I've already fixed this on my branch.

nakihito planned changes to this revision.Fri, Nov 15, 17:29
nakihito added inline comments.
src/seeder/test/seeder_tests.cpp
258 ↗(On Diff #14142)

This test can actually be removed once D4449 lands.

nakihito updated this revision to Diff 14155.Fri, Nov 15, 19:23

Removed test after rebase on D4449's changes and fixed comment typo.

Fabien requested changes to this revision.Mon, Nov 18, 12:26
Fabien added inline comments.
src/Makefile.seedertest.include
17

All these additions seem unnecessary, especially since the CMake version doesn't need them.

src/seeder/CMakeLists.txt
17

This is unrelated.

This revision now requires changes to proceed.Mon, Nov 18, 12:26
nakihito planned changes to this revision.Thu, Nov 21, 00:08