Page MenuHomePhabricator

Add simple unit tests for parse_name()
ClosedPublic

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
Commits
rABC9fd728bbafd3: Add simple unit tests for parse_name()
Summary

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

Currently, parse_name() has some logic flaws that prevent some test cases.

  • No check for the length of the query name (the maximum length is 255 characters total)
  • An assumption that the length of the output buffer is greater than 0.

Depends on D4417

Test Plan
make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix requested changes to this revision.Jan 20 2020, 18:50
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
67 ↗(On Diff #15621)

I spent some time with @Fabien looking at that block...

81 ↗(On Diff #15621)

... and why it is different from the previous block.

We could not see it and gave up. Clearly something is, because the first one is returning 0 and the second one is returning -2 . But we couldn't find it.

These are test cases, so this information should be obvious. If you are thinking about writing a response as to what is different, don't. The problem isn't that we do not know, and therefore telling us will not solve the problem. Given sufficient time, we *could* know.

The problem is that it is very difficult to know from reading the code. This is because the code is mixing boilerplate with meaningful code, the test cases are dependent on each others and so on. This is simply not readable. This has been through 13 review cycles now, and each one takes a lot of time and attention, and the feedback always end up pretty much the same.

Let's take this small example for instance:

https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/test/monolith_opcodes_tests.cpp$437-441

Very clearly you can see the parameters being tested, and the expected result. Then you dig into the function that is called and see this: https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/test/monolith_opcodes_tests.cpp$399-405

That check the various invariant we expect, it is very clear what the inputs being test are and what the expected result is supposed to be. The inputs which are always identical are not cluttering the view, such as each test case looks like a list of parameters being tested - always different from one call to another - and the result being expected.

This doesn't necessarily have to come in the form of function calls. For instance https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/test/arith_uint256_tests.cpp$488-494

The first line runs the test with parameters, the first one constituting the parameter varying being tested. Then it is followed by a series of BOOST_CHECK ensuring all the results of the execution of the first line are what is expected. Once again, it's in a different form, but it is very clear what is being tested, what the expected results are, and this is not drowned in boilerplate.

Please uses these exemples to figure out where this goes off rail and fix it. A patch such as this one shouldn't go through this many review cycles.

This revision now requires changes to proceed.Jan 20 2020, 18:50
nakihito updated this revision to Diff 15747.Jan 22 2020, 21:46

New test function: CheckParseName().

nakihito planned changes to this revision.Jan 22 2020, 21:46
nakihito edited the summary of this revision. (Show Details)Jan 22 2020, 22:13
nakihito edited the summary of this revision. (Show Details)Jan 24 2020, 21:57
nakihito updated this revision to Diff 15849.Jan 28 2020, 19:15

Added more robust testing for buffer size inputs. Added function to check for errors. Removed redundant branch. Merged the two boost test cases into one.

nakihito edited the summary of this revision. (Show Details)Jan 28 2020, 19:38
jasonbcox requested changes to this revision.Jan 28 2020, 23:04
jasonbcox added inline comments.
src/seeder/test/seeder_tests.cpp
54 ↗(On Diff #15849)

I see what you did where you test the passing cases in the next loop. If you keep it this way, I think this comment is misleading. Should be something like // Test when name field is too short to reach null terminator

66 ↗(On Diff #15849)

Related to above: // Test output buffer sizes with adequate name field length

This revision now requires changes to proceed.Jan 28 2020, 23:04
nakihito updated this revision to Diff 15864.Jan 29 2020, 00:53

Adjusted comments as per feedback.

jasonbcox accepted this revision.Jan 29 2020, 01:42

This is much better than prior versions.

deadalnix requested changes to this revision.Feb 10 2020, 18:47

This is much better, but is missing cases. Thankfully, it is now possible to see which cases are tested an which are not by now.

src/seeder/test/seeder_tests.cpp
112 ↗(On Diff #15864)

There are no guarantee that CheckParseName goes through the happy path, and therefore no guarantee that this doesn't fail.

118 ↗(On Diff #15864)

dito

Also, if this is the limit, then this length + 1 should fail, but this is not checked.

121 ↗(On Diff #15864)

Therefore, you do not know that this with one less character pass, and therefore you are not checking the limit.

This revision now requires changes to proceed.Feb 10 2020, 18:47
nakihito updated this revision to Diff 16227.Feb 10 2020, 20:22

Fixed off by one error.

nakihito added inline comments.Feb 10 2020, 20:23
src/seeder/test/seeder_tests.cpp
112 ↗(On Diff #15864)

MAX_LABEL_LENGTH = 63
"www.".size = 4
".com".size = 4
Total length = 71

MAX_QUERY_NAME_BUFFER_LENGTH = 256 > 71
This is guaranteed to go through the happy path.

121 ↗(On Diff #15864)

This is checking label length. The limit of which is checked on line 112.

deadalnix requested changes to this revision.Feb 24 2020, 01:09
deadalnix added inline comments.
src/seeder/test/seeder_tests.cpp
112 ↗(On Diff #15864)

Presumably, the goal of a test is to ensure this is the case rather than assume this is the case.

121 ↗(On Diff #15864)

If these tests go together, then they should be together.

This revision now requires changes to proceed.Feb 24 2020, 01:09

The fact that you had an off by one error and that the test was passing anyways is a sure sign test test is not design in a robust manner.

nakihito updated this revision to Diff 16500.Feb 25 2020, 21:11

Cutting this diff into smaller pieces. Also split the second for loop in CheckParseName() into two separate loops to remove branching logic.

nakihito planned changes to this revision.Feb 25 2020, 21:11
nakihito retitled this revision from Add unit tests for parse_name() to Add simple unit tests for parse_name().Feb 25 2020, 21:13
nakihito edited the summary of this revision. (Show Details)
nakihito requested review of this revision.Feb 25 2020, 21:21
jasonbcox accepted this revision.Feb 25 2020, 23:04
jasonbcox added inline comments.
src/seeder/test/seeder_tests.cpp
18 ↗(On Diff #16500)

IIRC I mentioned this in passing, but these values should be utilized in the implementation as well. A quick glance shows that it may not be a single drop-in replacement, so I'm ok with this being reserved for a future diff.

nakihito updated this revision to Diff 16627.Feb 28 2020, 20:10

Rebased and split the tests into two different files.

nakihito planned changes to this revision.Feb 28 2020, 20:10
nakihito requested review of this revision.Feb 28 2020, 20:31

Snippet of first build failure:

[20:46:48] :	 [Step 1/1] checking for SSL... yes
[20:46:48] :	 [Step 1/1] checking for CRYPTO... yes
[20:46:48] :	 [Step 1/1] checking for PROTOBUF... yes
[20:46:48] :	 [Step 1/1] checking for QR... yes
[20:46:48] :	 [Step 1/1] checking for EVENT... yes
[20:46:48] :	 [Step 1/1] checking for EVENT_PTHREADS... yes
[20:46:48] :	 [Step 1/1] checking for ZMQ... yes
[20:46:48] :	 [Step 1/1] checking whether EVP_MD_CTX_new is declared... yes
[20:46:48] :	 [Step 1/1] checking rapidcheck.h usability... no
[20:46:48] :	 [Step 1/1] checking rapidcheck.h presence... no
[20:46:48] :	 [Step 1/1] checking for rapidcheck.h... no
[20:46:48] :	 [Step 1/1] checking for protoc... /usr/bin/protoc
[20:46:48] :	 [Step 1/1] checking whether to build bitcoind... yes
[20:46:48] :	 [Step 1/1] checking whether to build bitcoin-seeder... yes
[20:46:48] :	 [Step 1/1] checking whether to build utils (bitcoin-cli bitcoin-tx)... yes
[20:46:48] :	 [Step 1/1] checking whether to build libraries... yes
[20:46:48] :	 [Step 1/1] checking if ccache should be used... yes
[20:46:48] :	 [Step 1/1] checking whether C preprocessor accepts -Qunused-arguments... no
[20:46:48] :	 [Step 1/1] checking if wallet should be enabled... yes
[20:46:48] :	 [Step 1/1] checking whether to build with support for UPnP... yes
[20:46:48] :	 [Step 1/1] checking whether to build with UPnP enabled by default... no
[20:46:48]W:	 [Step 1/1] configure: WARNING: "xgettext is required to update qt translations"
[20:46:48] :	 [Step 1/1] checking whether to build GUI with support for D-Bus... yes
[20:46:48] :	 [Step 1/1] checking whether to build GUI with support for QR codes... yes
[20:46:48] :	 [Step 1/1] checking whether to build test_bitcoin-qt... yes
[20:46:48] :	 [Step 1/1] checking whether to build BIP70 support... yes
[20:46:48] :	 [Step 1/1] checking whether to build test_bitcoin... yes
[20:46:48] :	 [Step 1/1] checking whether to build test_bitcoin-seeder... yes
[20:46:48] :	 [Step 1/1] checking whether to reduce exports... no
[20:46:48] :	 [Step 1/1] checking that generated files are newer than configure... done
[20:46:48] :	 [Step 1/1] configure: creating ./config.status
[20:46:49] :	 [Step 1/1] config.status: creating libbitcoinconsensus.pc
[20:46:49] :	 [Step 1/1] config.status: creating Makefile
[20:46:49] :	 [Step 1/1] config.status: creating src/Makefile
[20:46:49] :	 [Step 1/1] config.status: creating doc/man/Makefile
[20:46:49] :	 [Step 1/1] config.status: creating share/setup.nsi
[20:46:49] :	 [Step 1/1] config.status: creating share/qt/Info.plist
[20:46:49] :	 [Step 1/1] config.status: creating test/config.ini
[20:46:49] :	 [Step 1/1] config.status: creating contrib/devtools/split-debug.sh
[20:46:49] :	 [Step 1/1] config.status: creating doc/Doxyfile
[20:46:49] :	 [Step 1/1] config.status: creating src/config/version.h
[20:46:49] :	 [Step 1/1] config.status: creating src/config/bitcoin-config.h
[20:46:49] :	 [Step 1/1] config.status: linking ../test/functional/test_runner.py to test/functional/test_runner.py
[20:46:49] :	 [Step 1/1] config.status: linking ../test/util/bitcoin-util-test.py to test/util/bitcoin-util-test.py
[20:46:49] :	 [Step 1/1] config.status: linking ../test/util/rpcauth-test.py to test/util/rpcauth-test.py
[20:46:49] :	 [Step 1/1] config.status: executing depfiles commands
[20:46:49]W:	 [Step 1/1] config.status: error: in `/home/teamcity/buildAgent/work/c4a5708f2bae7929/build':
[20:46:49]W:	 [Step 1/1] config.status: error: Something went wrong bootstrapping makefile fragments
[20:46:49]W:	 [Step 1/1]     for automatic dependency tracking.  Try re-running configure with the
[20:46:49]W:	 [Step 1/1]     '--disable-dependency-tracking' option to at least be able to build
[20:46:49]W:	 [Step 1/1]     the package (albeit without support for automatic dependency tracking).
[20:46:49]W:	 [Step 1/1] See `config.log' for more details
[20:46:49] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[20:46:49]W:	 [Step 1/1] ++ print_sanitizers_log
[20:46:49]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[20:46:49]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[20:46:49]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[20:46:49]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[20:46:49]W:	 [Step 1/1] Process exited with code 1
[20:46:49]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
nakihito updated this revision to Diff 16633.Feb 28 2020, 22:02

Fix autotools build.

Snippet of first build failure:

[22:03:05] :	 [Step 1/1] checking for SSL... yes
[22:03:05] :	 [Step 1/1] checking for CRYPTO... yes
[22:03:05] :	 [Step 1/1] checking for PROTOBUF... yes
[22:03:05] :	 [Step 1/1] checking for QR... yes
[22:03:05] :	 [Step 1/1] checking for EVENT... yes
[22:03:05] :	 [Step 1/1] checking for EVENT_PTHREADS... yes
[22:03:05] :	 [Step 1/1] checking for ZMQ... yes
[22:03:05] :	 [Step 1/1] checking whether EVP_MD_CTX_new is declared... yes
[22:03:05] :	 [Step 1/1] checking rapidcheck.h usability... no
[22:03:05] :	 [Step 1/1] checking rapidcheck.h presence... no
[22:03:05] :	 [Step 1/1] checking for rapidcheck.h... no
[22:03:05] :	 [Step 1/1] checking for protoc... /usr/bin/protoc
[22:03:05] :	 [Step 1/1] checking whether to build bitcoind... yes
[22:03:05] :	 [Step 1/1] checking whether to build bitcoin-seeder... yes
[22:03:05] :	 [Step 1/1] checking whether to build utils (bitcoin-cli bitcoin-tx)... yes
[22:03:05] :	 [Step 1/1] checking whether to build libraries... yes
[22:03:05] :	 [Step 1/1] checking if ccache should be used... yes
[22:03:05] :	 [Step 1/1] checking whether C preprocessor accepts -Qunused-arguments... no
[22:03:05] :	 [Step 1/1] checking if wallet should be enabled... yes
[22:03:05] :	 [Step 1/1] checking whether to build with support for UPnP... yes
[22:03:05] :	 [Step 1/1] checking whether to build with UPnP enabled by default... no
[22:03:05] :	 [Step 1/1] checking whether to build GUI with support for D-Bus... yes
[22:03:05] :	 [Step 1/1] checking whether to build GUI with support for QR codes... yes
[22:03:05]W:	 [Step 1/1] configure: WARNING: "xgettext is required to update qt translations"
[22:03:05] :	 [Step 1/1] checking whether to build test_bitcoin-qt... yes
[22:03:05] :	 [Step 1/1] checking whether to build BIP70 support... yes
[22:03:05] :	 [Step 1/1] checking whether to build test_bitcoin... yes
[22:03:05] :	 [Step 1/1] checking whether to build test_bitcoin-seeder... yes
[22:03:05] :	 [Step 1/1] checking whether to reduce exports... no
[22:03:05] :	 [Step 1/1] checking that generated files are newer than configure... done
[22:03:05] :	 [Step 1/1] configure: creating ./config.status
[22:03:07] :	 [Step 1/1] config.status: creating libbitcoinconsensus.pc
[22:03:07] :	 [Step 1/1] config.status: creating Makefile
[22:03:07] :	 [Step 1/1] config.status: creating src/Makefile
[22:03:07] :	 [Step 1/1] config.status: creating doc/man/Makefile
[22:03:07] :	 [Step 1/1] config.status: creating share/setup.nsi
[22:03:07] :	 [Step 1/1] config.status: creating share/qt/Info.plist
[22:03:07] :	 [Step 1/1] config.status: creating test/config.ini
[22:03:07] :	 [Step 1/1] config.status: creating contrib/devtools/split-debug.sh
[22:03:07] :	 [Step 1/1] config.status: creating doc/Doxyfile
[22:03:07] :	 [Step 1/1] config.status: creating src/config/version.h
[22:03:07] :	 [Step 1/1] config.status: creating src/config/bitcoin-config.h
[22:03:07] :	 [Step 1/1] config.status: linking ../test/functional/test_runner.py to test/functional/test_runner.py
[22:03:07] :	 [Step 1/1] config.status: linking ../test/util/bitcoin-util-test.py to test/util/bitcoin-util-test.py
[22:03:07] :	 [Step 1/1] config.status: linking ../test/util/rpcauth-test.py to test/util/rpcauth-test.py
[22:03:07] :	 [Step 1/1] config.status: executing depfiles commands
[22:03:07]W:	 [Step 1/1] config.status: error: in `/home/teamcity/buildAgent/work/c4a5708f2bae7929/build':
[22:03:07]W:	 [Step 1/1] config.status: error: Something went wrong bootstrapping makefile fragments
[22:03:07]W:	 [Step 1/1]     for automatic dependency tracking.  Try re-running configure with the
[22:03:07]W:	 [Step 1/1]     '--disable-dependency-tracking' option to at least be able to build
[22:03:07]W:	 [Step 1/1]     the package (albeit without support for automatic dependency tracking).
[22:03:07]W:	 [Step 1/1] See `config.log' for more details
[22:03:07] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[22:03:07]W:	 [Step 1/1] ++ print_sanitizers_log
[22:03:07]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[22:03:07]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[22:03:07]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[22:03:07]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[22:03:07]W:	 [Step 1/1] Process exited with code 1
[22:03:07]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
nakihito planned changes to this revision.Feb 28 2020, 22:04

Investigating autotools build error.

nakihito requested review of this revision.Feb 29 2020, 04:21
deadalnix accepted this revision.Feb 29 2020, 16:14
deadalnix added inline comments.
src/seeder/test/dns_tests.cpp
68 ↗(On Diff #16633)

Testing the value of nameFieldBegin would be beneficial.

This revision is now accepted and ready to land.Feb 29 2020, 16:15
This revision was automatically updated to reflect the committed changes.