Page MenuHomePhabricator

Add unit tests for parse_name()
Needs ReviewPublic

Authored by nakihito on Nov 9 2019, 01:00.

Details

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

Adds some unit tests for the seeder's parse_name() function.

Depends on D4417

Test Plan
make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ParseNameUnitTest-2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9032
Build 16026: Bitcoin ABC Buildbot
Build 16025: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fixed typos.

jasonbcox requested changes to this revision.Nov 15 2019, 18:27
jasonbcox added inline comments.
src/seeder/test/seeder_tests.cpp
9 ↗(On Diff #14153)

Third party libs should come after system libs.

This revision now requires changes to proceed.Nov 15 2019, 18:27
nakihito updated this revision to Diff 14154.Nov 15 2019, 18:30

Fixed nit.

jasonbcox accepted this revision.Nov 15 2019, 18:51
nakihito planned changes to this revision.Nov 18 2019, 22:49
nakihito updated this revision to Diff 14276.Nov 20 2019, 21:44

Fixed some names and added one more check to the last test.

nakihito planned changes to this revision.Nov 20 2019, 23:16
nakihito requested review of this revision.Dec 2 2019, 21:45
deadalnix requested changes to this revision.Dec 18 2019, 00:51
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
11 ↗(On Diff #14276)

unlikely this is needed.

61 ↗(On Diff #14276)

You are converting everything in hex, then converting back in binary to put it in the buffer? Thanks makes no sense.

132 ↗(On Diff #14276)

Sounds like using a vector as a result from CreateDNSQuestion would be more appropriate than this. You completely break any encapsulation you created here.

This revision now requires changes to proceed.Dec 18 2019, 00:51
nakihito updated this revision to Diff 15143.Fri, Jan 3, 20:43

Changed const uint8_t *messageEnd (a pointer) to const size_t messageEndIndex and removed unnecssary #include <iostream>.

nakihito planned changes to this revision.Fri, Jan 3, 20:43
nakihito requested review of this revision.Fri, Jan 3, 21:02
deadalnix requested changes to this revision.Sat, Jan 4, 10:32
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
62 ↗(On Diff #15143)
This revision now requires changes to proceed.Sat, Jan 4, 10:32
nakihito updated this revision to Diff 15188.Mon, Jan 6, 22:09

Refactored and simplified CreateDNSQuestion() to return a std::vector<uint8_t>.

nakihito planned changes to this revision.Mon, Jan 6, 22:09
nakihito requested review of this revision.Tue, Jan 7, 19:31
deadalnix requested changes to this revision.Thu, Jan 9, 13:08
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
25 ↗(On Diff #15188)

Query not Question.

26 ↗(On Diff #15188)

This is not hex.

This revision now requires changes to proceed.Thu, Jan 9, 13:08
nakihito updated this revision to Diff 15314.Fri, Jan 10, 21:21

queryhex -> dnsQuestion and dnsMessage -> dnsQuestion. Removed one-time use variable labelLength. Also clarified some comments.

nakihito planned changes to this revision.Fri, Jan 10, 21:21
nakihito requested review of this revision.Mon, Jan 13, 19:43
nakihito added inline comments.
src/seeder/test/seeder_tests.cpp
25 ↗(On Diff #15188)

This is actually only building the Question section of the DNS message (see sections 4.1 and 4.1.2 of https://www.zytrax.com/books/dns/apd/rfc1035.txt). I have adjusted other variable names to better reflect this.

deadalnix requested changes to this revision.Wed, Jan 15, 15:34
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
64 ↗(On Diff #15314)

Wouldn't that be less dangerous to use the size of the question, or at least assert that all these computed size are compatible with each others?

89 ↗(On Diff #15314)

The only thing that seems to be different from the previous test to get that result is the size passed to the function above as last parameter. Reviewing this feels a lot like playing where is Waldo, which is a sure sign the code is unreadable.

You test case should read like "We test X with parameters A, B an C, we expect the result to be Y" with various sets of A, B and C and Y.

I did not review subsequent tests because it would literally takes less time to actually write them than review this.

This revision now requires changes to proceed.Wed, Jan 15, 15:34
nakihito updated this revision to Diff 15514.Thu, Jan 16, 01:22

Various variable name changes and more comments for better clarity. Removed Excess constants and variables. CreateDNSQuestion() -> CreateDNSQuestionNameField() because parse_name() finishes onces it reaches the end of the name field. First three tests collapsed into a singular test. Added assert() to check that nameFieldEndIndex is correct.

jasonbcox accepted this revision.Thu, Jan 16, 22:13
deadalnix requested changes to this revision.Fri, Jan 17, 01:20
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
53 ↗(On Diff #15514)

Is the result always zero terminated? And the zero always fit in the buffer?

There are no checks for this, and frankly I do not know.

60 ↗(On Diff #15514)

BOOST_CHECK or BOOST_REQUIRE

79 ↗(On Diff #15514)

I still have to play where is waldo here. Something in these 10 lines of code differs, I don't now what.

93 ↗(On Diff #15514)

If you want to test if the name is too long, then you want to check if shorter name pass, up to the maximum length, and then that the maximum length + 1 and more fails.

107 ↗(On Diff #15514)

That is what BOOST_CHECK is for.

This revision now requires changes to proceed.Fri, Jan 17, 01:20
nakihito added inline comments.Fri, Jan 17, 20:31
src/seeder/test/seeder_tests.cpp
53 ↗(On Diff #15514)

When successful (return == 0), the result from parse_name() is a 0-terminated c-string. On failure (return != 0), it is a char array that is not 0-terminated. The 0 must be able to fit in the buffer. This is actually tested line 70-79. I have refined the comment on line 73 to better clarify.

79 ↗(On Diff #15514)

There's a comment on line 73 that specifies what is different.

nakihito updated this revision to Diff 15621.Fri, Jan 17, 20:31

assert() -> BOOST_CHECK_EQUAL(). More clarification to comments. Added testing for a maximum sized label.