Page MenuHomePhabricator
Differential D4418 Diff 15621 src/seeder/test/seeder_tests.cpp

Changeset View

Standalone View

View Options

src/seeder/test/seeder_tests.cpp

// Copyright (c) 2019 The Bitcoin developers // Copyright (c) 2019 The Bitcoin developers
// Distributed under the MIT software license, see the accompanying // Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php. // file COPYING or http://www.opensource.org/licenses/mit-license.php.
#define BOOST_TEST_MODULE Bitcoin Seeder Test Suite #define BOOST_TEST_MODULE Bitcoin Seeder Test Suite
#include <seeder/dns.h>
#include <array>
#include <string>
#include <vector>
#include <boost/test/unit_test.hpp> #include <boost/test/unit_test.hpp>
BOOST_AUTO_TEST_SUITE(seeder_tests) BOOST_AUTO_TEST_SUITE(seeder_tests)
BOOST_AUTO_TEST_CASE(test_stub) { static const int QUERY_NAME_BUFFER_LENGTH = 256;
BOOST_CHECK_EQUAL(true, true); static const uint8_t END_OF_NAME_FIELD = 0;
static const size_t MAX_LABEL_LENGTH = 63;
// Builds the name field of the question section of a DNS query
static std::vector<uint8_t>
CreateDNSQuestionNameField(const std::string &queryName) {
std::vector<uint8_t> nameField;
size_t i = 0;
size_t labelIndex = 0;
while (i < queryName.size()) {
if (queryName[i] == '.') {
// Push the length of the label and then the label
nameField.push_back(i - labelIndex);
while (labelIndex < i) {
nameField.push_back(queryName[labelIndex]);
labelIndex++;
}
labelIndex = i + 1;
}
i++;
}
// Push the length of the label and then the label
nameField.push_back(i - labelIndex);
while (labelIndex < i) {
nameField.push_back(queryName[labelIndex]);
labelIndex++;
}
nameField.push_back(END_OF_NAME_FIELD);
return nameField;
}
BOOST_AUTO_TEST_CASE(parse_name_simple) {
// Happy path
const std::string queryName = "www.mydomain.com";
std::vector<uint8_t> nameField = CreateDNSQuestionNameField(queryName);
std::array<char, QUERY_NAME_BUFFER_LENGTH> parsedQueryName;
const uint8_t *nameFieldBegin = nameField.data();
const size_t nameFieldEndIndex = nameField.size();
// nameFieldEndIndex should be equal to the number of alphanumeric
// characters in the query name, +1 for each label (each label length is 1
// octet), +1 for the length of the END_NAME_FIELD octet.
BOOST_CHECK_EQUAL(nameFieldEndIndex, queryName.size() + 2);
int ret = parse_name(&nameFieldBegin, nameFieldBegin + nameFieldEndIndex,
nameField.data(), parsedQueryName.data(),
parsedQueryName.size());
BOOST_CHECK_EQUAL(ret, 0);
BOOST_CHECK_EQUAL(parsedQueryName.data(), queryName);
deadalnixUnsubmitted
Not Done
Inline Actions

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

deadalnix: I spent some time with @Fabien looking at that block...
// Test for insufficient output buffer size
parsedQueryName.fill(0);
nameFieldBegin = nameField.data();
// The size of the buffer being written to is 1 octect too small. This would
// cause the result to be a c-string because it is not that is not
// null-terminated. Instead, the function writes up to just before the last
// character and then fails returning -2.
ret =
parse_name(&nameFieldBegin, nameFieldBegin + nameFieldEndIndex,
nameField.data(), parsedQueryName.data(), queryName.size());
BOOST_CHECK_EQUAL(ret, -2);
BOOST_CHECK_EQUAL(parsedQueryName.data(),
queryName.substr(0, queryName.size() - 1));
deadalnixUnsubmitted
Not Done
Inline Actions

... 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.

deadalnix: ... and why it is different from the previous block. We could not see it and gave up. Clearly…
// Test for premature end of input buffer
parsedQueryName.fill(0);
nameFieldBegin = nameField.data();
// The end index pointer for the DNS message buffer passed is located two
// octets away from the beginning
ret = parse_name(&nameFieldBegin, nameFieldBegin + 2, nameField.data(),
parsedQueryName.data(), parsedQueryName.size());
BOOST_CHECK_EQUAL(ret, -1);
BOOST_CHECK_EQUAL(parsedQueryName.data(), queryName.substr(0, 1));
}
// Test for when name field is too long
BOOST_AUTO_TEST_CASE(parse_name_field_name_too_long) {
// First test that the MAX_LABEL_LENGTH passes
std::string maxLabelLengthQName = "www.";
for (size_t i = 0; i < MAX_LABEL_LENGTH; i++) {
maxLabelLengthQName += 'a';
}
maxLabelLengthQName += ".com";
std::vector<uint8_t> maxLabelLengthNameField =
CreateDNSQuestionNameField(maxLabelLengthQName);
std::array<char, QUERY_NAME_BUFFER_LENGTH> parsedMaxLabelLengthQueryName;
const uint8_t *maxLabelLengthNameFieldBegin =
maxLabelLengthNameField.data();
size_t maxLabelLengthNameFieldEndIndex = maxLabelLengthNameField.size();
// nameFieldEndIndex should be equal to the number of alphanumeric
// characters in the query name, +1 for each label (each label length is 1
// octet), +1 for the length of the END_NAME_FIELD octet.
BOOST_CHECK_EQUAL(maxLabelLengthNameFieldEndIndex,
maxLabelLengthQName.size() + 2);
int ret = parse_name(
&maxLabelLengthNameFieldBegin,
maxLabelLengthNameFieldBegin + maxLabelLengthNameFieldEndIndex,
maxLabelLengthNameField.data(), parsedMaxLabelLengthQueryName.data(),
QUERY_NAME_BUFFER_LENGTH);
BOOST_CHECK_EQUAL(ret, 0);
BOOST_CHECK_EQUAL(parsedMaxLabelLengthQueryName.data(),
maxLabelLengthQName);
// Verify that MAX_LABEL_LENGTH + 1 fails
std::string tooLongQName = "www.";
for (size_t i = 0; i < MAX_LABEL_LENGTH; i++) {
tooLongQName += 'a';
}
tooLongQName += "a.com";
std::vector<uint8_t> tooLongNameField =
CreateDNSQuestionNameField(tooLongQName);
std::array<char, QUERY_NAME_BUFFER_LENGTH> parsedTooLongQueryName;
parsedTooLongQueryName.fill(0);
const uint8_t *tooLongNameFieldBegin = tooLongNameField.data();
size_t tooLongNameFieldEndIndex = tooLongNameField.size();
// nameFieldEndIndex should be equal to the number of alphanumeric
// characters in the query name, +1 for each label (each label length is 1
// octet), +1 for the length of the END_NAME_FIELD octet.
BOOST_CHECK_EQUAL(tooLongNameFieldEndIndex, tooLongQName.size() + 2);
ret = parse_name(&tooLongNameFieldBegin,
tooLongNameFieldBegin + tooLongNameFieldEndIndex,
tooLongNameField.data(), parsedTooLongQueryName.data(),
QUERY_NAME_BUFFER_LENGTH);
BOOST_CHECK_EQUAL(ret, -1);
BOOST_CHECK_EQUAL(parsedTooLongQueryName.data(), tooLongQName.substr(0, 4));
} }
BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END()