⚙ D4418 Add simple unit tests for parse

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9032: Diff 15621.Jan 17 2020, 20:48

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

New test function: CheckParseName().

nakihito planned changes to this revision.Jan 22 2020, 21:46

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9110: Diff 15747.Jan 22 2020, 21:53

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

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9176: Diff 15849.Jan 28 2020, 19:20

nakihito edited the summary of this revision. (Show Details)Jan 28 2020, 19:38

nakihito added a child revision: D5091: Make parse_name() fail when passed buffer size = 0.Jan 28 2020, 19:41

jasonbcox requested changes to this revision.Jan 28 2020, 23:04

jasonbcox added inline comments.

src/seeder/test/seeder_tests.cpp
54	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	Related to above: `// Test output buffer sizes with adequate name field length`

This revision now requires changes to proceed.Jan 28 2020, 23:04

Adjusted comments as per feedback.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9183: Diff 15864.Jan 29 2020, 00:59

This is much better than prior versions.

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

Fixed off by one error.

Build Bitcoin-ABC / Diffs / Diff Testing started.

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.

nakihito added a child revision: D5241: Make parse_name() properly fail when the query name is longer than 255 characters.Feb 10 2020, 20:24

nakihito removed a child revision: D4436: Add unit tests for CSeederNode::ProcessMessage().Feb 10 2020, 20:33

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9392: Diff 16227.Feb 10 2020, 20:45

jasonbcox mentioned this in D4436: Add unit tests for CSeederNode::ProcessMessage().Feb 10 2020, 22:23

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.

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

Build Bitcoin-ABC / Diffs / Diff Testing started.

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

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9565: Diff 16500.Feb 25 2020, 21:21

nakihito added a child revision: D5328: Label length unit tests for parse_name().Feb 25 2020, 21:25

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.

jasonbcox mentioned this in D5328: Label length unit tests for parse_name().Feb 26 2020, 00:53

Rebased and split the tests into two different files.

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito planned changes to this revision.Feb 28 2020, 20:10

Harbormaster completed remote builds in B9642: Diff 16627.Feb 28 2020, 20:31

Build Bitcoin-ABC / Diffs / Diff Testing passed.

nakihito requested review of this revision.Feb 28 2020, 20:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

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)

Fix autotools build.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

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)

Investigating autotools build error.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9647: Diff 16633.Feb 28 2020, 22:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

nakihito requested review of this revision.Feb 29 2020, 04:21

deadalnix accepted this revision.Feb 29 2020, 16:14

deadalnix mentioned this in D5380: [seeder] Require good seeds to have latest checkpoint.

deadalnix added inline comments.