Page MenuHomePhabricator

Add unit tests for parse_name()
Needs RevisionPublic

Authored by nakihito on Sat, Nov 9, 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 8041
Build 14077: Bitcoin ABC Buildbot
Build 14076: arc lint + arc unit

Event Timeline

nakihito created this revision.Sat, Nov 9, 01:00
jasonbcox added inline comments.Sat, Nov 9, 01:32
src/seeder/test/seeder_tests.cpp
23

It seemed a little suspicious to me that these constants would be defined in the test. But after looking at the existing seeder source, it's clear that these magic values are mostly hardcoded everywhere. So while this is the right approach for now (to test existing behavior), I expect at least some of these constants to eventually move to more appropriate locations.

deadalnix requested changes to this revision.Sun, Nov 10, 15:50
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
43

Why all these static casts?

43

On a second note, why not just push strings instead of this?

128

Honestly, this test is very hard to follow. The variable names are not meaningful (qname? inbuf?). There are magic numbers everywhere. Almost everything is mutated thourough the test such as it means different things at different points in time.

The whole thing is convoluted for no apparent reason. For instance, what does this achieve:

std::array<uint8_t, BUFFER_LENGTH> longInbuf;
longInbuf.fill(0);
longInbuf = CreateDNSMessage(longMessageQName);

over

std::array<uint8_t, BUFFER_LENGTH> longInbuf = CreateDNSMessage(longMessageQName);

?

I think this code would benefit a lot from a few passes of self review.

This revision now requires changes to proceed.Sun, Nov 10, 15:50