Opened 7 years ago

Closed 7 years ago

#2480 closed defect (fixed)

use real data sources in auth QueryTest

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130108
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 17
Total Hours: 0 Internal?: no

Description

Currently bin/auth QueryTest only uses the mock data source and its
zone finder. We need the mock to test some tricky/unusual cases, but
for normal cases we should also use real data sources, at least
the in-memory (only the new version should be enough), and preferably
database-based one (which currently means SQLite3).

I guess it's not difficult to extend the current cases using
value-parameterized tests (TEST_P) or type-parameterized tests
(TYPED_TEST), skipping some specific cases with the real data sources
if they only work with the mock.

We should also unify the extension introduced in #2471, where only
one test case was extended for the in-memory implementation.

Subtickets

Change History (17)

comment:1 follow-up: Changed 7 years ago by shane

I'm not opposed to this idea, but on further thinking I'm not sure what the point is.

Our data sources have individual unit tests.

Unit tests in auth should be testing auth, not the underlying data sources.

In principle the interaction between the two pieces of code should be tested by integration tests, which we currently use Lettuce for.

If this is a sort of "unit test++" and it is the easiest way forward, then I'm not opposed to it, I'm just trying to understand the goal and make sure it is the right way to achieve it.

comment:2 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121204

comment:3 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:4 Changed 7 years ago by jinmei

I'm releasing the ticket for now, considering the current status of
zone loader tasks. I've pushed my latest snapshot for this task
as the trac2480 branch.

comment:5 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to assigned

comment:6 Changed 7 years ago by jelte

  • Milestone changed from Sprint-DHCP-20130103 to Sprint-20130108

comment:7 in reply to: ↑ 1 Changed 7 years ago by jinmei

Replying to shane:

If this is a sort of "unit test++" and it is the easiest way forward, then I'm not opposed to it, I'm just trying to understand the goal and make sure it is the right way to achieve it.

It depends on the definition of "unit test++", but what I intended
(and actually started doing) in this ticket is to use the existing
unit test cases with the mock data source for other types of data
sources (namely in-memory and sqlite3). google test has convenient
frameworks to implement these things, so I believe it's pretty good in
terms of cost-benefit balance.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from assigned to accepted

comment:9 Changed 7 years ago by jinmei

It's now ready for review at the trac2480-2 branch (there was a
"trac2480" branch in the shared repo, but I've rebased it and so
created a separate new branch for review to avoid confusion; ignore
the old "trac2480").

Essentially, this branch only does simple things:

  • use parameterized tests so we can share the same test code with multiple data source implementations
  • share the test data: extract the original hardcoded data into a unified text source, and generate C++ source file/DNS zone file/SQLite3 database file for actual data sources
  • other adjustments to the test cases (tentatively disabling some test cases, etc)

but the resulting diff is quite large, so it may be better to separate
the review process into two phases: adjustments for non-NSEC3 tests
(up to commit 2303506 (inclusive)) and the rest (on and after commit
33ba182).

Most of the hardcoded data in query_unittest.cc were moved to new
query_testzone_data*.txt. The test data were not changed (except for
adding RRSIGs explicitly). An added Python script,
gen-query-testdata.py creates these text files and generates actual
input for tests. I guess these occupy most of the diffs, and the rest
is quite straightforward.

I found a few real bugs in the data source implementations, for which
I've disabled corresponding tests for that type of data source. I'll
create separate tickets for them.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:11 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:12 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

It doesn't compile for me, it seems the set of warnings of clang++ and g++ are
different. I think most of the defined but not used ones is side-effect of the
previous error (whenever there's an error in a test file, g++ spits out bunch
of unrelated warnings), the real one would be the control reaches…:

query_unittest.cc: In function ‘boost::shared_ptr<isc::datasrc::ClientList> {anonymous}::createDataSrcClientList({anonymous}::DataSrcType, isc::datasrc::DataSourceClient&)’:
query_unittest.cc:811:1: error: control reaches end of non-void function [-Werror=return-type]
query_unittest.cc: At global scope:
query_unittest.cc:946:2: error: ‘{anonymous}::gtest_QueryTest_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:985:557: error: ‘{anonymous}::QueryTest_noZone_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:995:585: error: ‘{anonymous}::QueryTest_exactMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1002:691: error: ‘{anonymous}::QueryTest_exactMatchMultipleQueries_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1019:649: error: ‘{anonymous}::QueryTest_exactMatchIgnoreSIG_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1029:614: error: ‘{anonymous}::QueryTest_dnssecPositive_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1046:614: error: ‘{anonymous}::QueryTest_exactAddrMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1059:593: error: ‘{anonymous}::QueryTest_apexNSMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1070:607: error: ‘{anonymous}::QueryTest_exactAnyMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1089:600: error: ‘{anonymous}::QueryTest_apexAnyMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1100:586: error: ‘{anonymous}::QueryTest_mxANYMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1108:600: error: ‘{anonymous}::QueryTest_glueANYMatch_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1115:593: error: ‘{anonymous}::QueryTest_nodomainANY_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1128:572: error: ‘{anonymous}::QueryTest_noApexNS_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1141:586: error: ‘{anonymous}::QueryTest_delegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1150:656: error: ‘{anonymous}::QueryTest_delegationWithDNSSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1161:628: error: ‘{anonymous}::QueryTest_secureDelegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1176:684: error: ‘{anonymous}::QueryTest_secureUnsignedDelegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1191:747: error: ‘{anonymous}::QueryTest_secureUnsignedDelegationWithNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1218:789: error: ‘{anonymous}::QueryTest_secureUnsignedDelegationWithNSEC3OptOut_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1244:649: error: ‘{anonymous}::QueryTest_badSecureDelegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1264:572: error: ‘{anonymous}::QueryTest_nxdomain_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1272:628: error: ‘{anonymous}::QueryTest_nxdomainWithNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1292:635: error: ‘{anonymous}::QueryTest_nxdomainWithNSEC2_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1318:691: error: ‘{anonymous}::QueryTest_nxdomainWithNSECDuplicate_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1340:628: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC1_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1355:628: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC2_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1370:621: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1385:628: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC4_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1399:628: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC5_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1425:628: error: ‘{anonymous}::QueryTest_nxdomainBadNSEC6_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1440:565: error: ‘{anonymous}::QueryTest_nxrrset_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1448:621: error: ‘{anonymous}::QueryTest_nxrrsetWithNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1463:635: error: ‘{anonymous}::QueryTest_emptyNameWithNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1484:642: error: ‘{anonymous}::QueryTest_nxrrsetWithoutNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1497:600: error: ‘{anonymous}::QueryTest_wildcardNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1517:607: error: ‘{anonymous}::QueryTest_CNAMEwildNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1533:607: error: ‘{anonymous}::QueryTest_wildcardNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1567:614: error: ‘{anonymous}::QueryTest_CNAMEwildNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1593:628: error: ‘{anonymous}::QueryTest_badWildcardNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1613:635: error: ‘{anonymous}::QueryTest_badWildcardProof1_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1629:635: error: ‘{anonymous}::QueryTest_badWildcardProof2_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1643:635: error: ‘{anonymous}::QueryTest_badWildcardProof3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1658:740: error: ‘{anonymous}::QueryTest_wildcardNxrrsetWithDuplicateNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1679:677: error: ‘{anonymous}::QueryTest_wildcardNxrrsetWithNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1704:684: error: ‘{anonymous}::QueryTest_wildcardNxrrsetWithNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1743:747: error: ‘{anonymous}::QueryTest_wildcardNxrrsetWithNSEC3Collision_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1762:726: error: ‘{anonymous}::QueryTest_wildcardNxrrsetWithNSEC3Broken_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1784:663: error: ‘{anonymous}::QueryTest_wildcardEmptyWithNSEC_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1807:551: error: ‘{anonymous}::QueryTest_noSOA_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1826:593: error: ‘{anonymous}::QueryTest_noMatchZone_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1839:530: error: ‘{anonymous}::QueryTest_MX_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1853:565: error: ‘{anonymous}::QueryTest_MXAlias_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1873:551: error: ‘{anonymous}::QueryTest_CNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1881:607: error: ‘{anonymous}::QueryTest_explicitCNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1891:614: error: ‘{anonymous}::QueryTest_CNAME_NX_RRSET_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1903:670: error: ‘{anonymous}::QueryTest_explicitCNAME_NX_RRSET_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1912:621: error: ‘{anonymous}::QueryTest_CNAME_NX_DOMAIN_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1926:677: error: ‘{anonymous}::QueryTest_explicitCNAME_NX_DOMAIN_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1935:579: error: ‘{anonymous}::QueryTest_CNAME_OUT_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1951:635: error: ‘{anonymous}::QueryTest_explicitCNAME_OUT_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1968:551: error: ‘{anonymous}::QueryTest_DNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1984:579: error: ‘{anonymous}::QueryTest_DNAME_ANY_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:1993:607: error: ‘{anonymous}::QueryTest_explicitDNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2005:565: error: ‘{anonymous}::QueryTest_DNAME_A_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2017:614: error: ‘{anonymous}::QueryTest_DNAME_NX_RRSET_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2030:579: error: ‘{anonymous}::QueryTest_LongDNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2050:593: error: ‘{anonymous}::QueryTest_MaxLenDNAME_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2106:579: error: ‘{anonymous}::QueryTest_findNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2250:635: error: ‘{anonymous}::QueryTest_dsAboveDelegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2277:677: error: ‘{anonymous}::QueryTest_dsAboveDelegationNoData_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2304:635: error: ‘{anonymous}::QueryTest_dsBelowDelegation_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2320:677: error: ‘{anonymous}::QueryTest_dsBelowDelegationWithDS_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2339:572: error: ‘{anonymous}::QueryTest_dsNoZone_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2347:621: error: ‘{anonymous}::QueryTest_dsAtGrandParent_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2362:677: error: ‘{anonymous}::QueryTest_dsAtGrandParentAndChild_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2387:572: error: ‘{anonymous}::QueryTest_dsAtRoot_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2410:614: error: ‘{anonymous}::QueryTest_dsAtRootWithDS_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2432:628: error: ‘{anonymous}::QueryTest_nxrrsetWithNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2452:649: error: ‘{anonymous}::QueryTest_nxrrsetMissingNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2470:691: error: ‘{anonymous}::QueryTest_nxrrsetWithNSEC3_ds_exact_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2495:712: error: ‘{anonymous}::QueryTest_nxrrsetWithNSEC3_ds_no_exact_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2527:670: error: ‘{anonymous}::QueryTest_nxdomainWithNSEC3Proof_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2569:719: error: ‘{anonymous}::QueryTest_nxdomainWithBadNextNSEC3Proof_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2588:747: error: ‘{anonymous}::QueryTest_nxdomainWithBadWildcardNSEC3Proof_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
query_unittest.cc:2615:642: error: ‘{anonymous}::QueryTest_emptyNameWithNSEC3_Test::gtest_registering_dummy_’ defined but not used [-Werror=unused-variable]
cc1plus: all warnings being treated as errors

Then, I think this kind of rules in Makefile are wrong:

testdata/example-base.zone example_base_inc.cc: testdata/query_testzone_data.txt
       $(PYTHON) $(srcdir)/gen-query-testdata.py \
               $(srcdir)/testdata/query_testzone_data.txt \
               testdata/example-base.zone example_base_inc.cc

Make doesn't really know multi-target rules. This are effectively two
independent rules (one to create testdata/example-base.zone, the other to
generate example_base_inc.cc) which have the same dependencies and the same
body. But they'd be run twice, possibly at the same twice. It could lead to
strange behaviour on some OSes (compilation failures or file that are written
twice, interleaved). Would it be possible to have two modes for the
gen-query-test.py, one for generating each file, and splitting the rule?

Why is this safety-catch removed? It was supposed (I guess) to catch bad
updates to the test data:

-        const string hlabel = hash_map_[name.split(i, labels - i)];
-        if (hlabel.empty()) {
-            isc_throw(isc::Unexpected, "findNSEC3() hash failure for " <<
-                      name.split(i, labels - i));
-        }
+        const string hlabel = nsec3_hash_.calculate(name.split(i, labels - i));

If I look at the file src/bin/auth/tests/testdata/example.zone.in, would it
be better to use the generated directly, instead of a file that contains only
include of it?

The test data contains two „var=“ (without the variable name) in a row, could
the second one be omitted?

#var=
example.com. 3600 IN RRSIG SOA 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE
example.com. 3600 IN RRSIG NS 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE

#var=
noglue.example.com. 3600 IN RRSIG A 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE

Thinking about it, could it be directly a master file and #var= would be ;var=?
In which case, only the .cc and sqlite3 files would need to be generated.

Also, about the way the tests are selectively disabled, I don't really like
this much. This looks like the thing is tested and passes (because the test is
listed in the output), but it is actually not tested. Wouldn't there be a way to
run only the right tests? I could think of a way with defining separate lists
of values (MOCK, MOCK+SQLITE3, everything) and having 3 different typedef
aliases for the test class, but I don't think that is an elegant solution (but
maybe still slightly better).

TEST_P(QueryTest, exactAnyMatch) {
    // there's a bug in the in-memory data source and this doesn't work
    if (GetParam() == INMEMORY) {
        return;
    }

comment:13 in reply to: ↑ 12 Changed 7 years ago by jinmei

Thanks for the review.

Replying to vorner:

It doesn't compile for me, it seems the set of warnings of clang++ and g++ are
different. I think most of the defined but not used ones is side-effect of the
previous error (whenever there's an error in a test file, g++ spits out bunch
of unrelated warnings), the real one would be the control reaches…:

Ah, okay. I've reproduced it on a different machine, and fixed it. I
believe it works for you too.

Then, I think this kind of rules in Makefile are wrong:

testdata/example-base.zone example_base_inc.cc: testdata/query_testzone_data.txt
       $(PYTHON) $(srcdir)/gen-query-testdata.py \
               $(srcdir)/testdata/query_testzone_data.txt \
               testdata/example-base.zone example_base_inc.cc

Make doesn't really know multi-target rules.

[...]

If I look at the file src/bin/auth/tests/testdata/example.zone.in, would it
be better to use the generated directly, instead of a file that contains only
include of it?

[...]

Thinking about it, could it be directly a master file and #var= would be ;var=?
In which case, only the .cc and sqlite3 files would need to be generated.

I think these are all related, and your suggestion in the last should
address all of the issues. I updated the branch based on the
suggestion: the source data are now in zone file format and
multi-target rules are gone. We might still avoid include-only top
zone files, but include will be used for other purpose in a later
change (see below). So it's still kept.

Why is this safety-catch removed? It was supposed (I guess) to catch bad
updates to the test data:

-        const string hlabel = hash_map_[name.split(i, labels - i)];
-        if (hlabel.empty()) {
-            isc_throw(isc::Unexpected, "findNSEC3() hash failure for " <<
-                      name.split(i, labels - i));
-        }
+        const string hlabel = nsec3_hash_.calculate(name.split(i, labels - i));

Ah, maybe I should note it explicitly, but nsec3_hash_ internally
performs this check.

The test data contains two „var=“ (without the variable name) in a row, could
the second one be omitted?

It could. But I've kept it with some additional notes in comments
including this point. This point is a bit tricky so it should
be worth commenting anyway. We can still remove the
technically-unnecessary "var=" markup if you want.

Also, about the way the tests are selectively disabled, I don't really like
this much. This looks like the thing is tested and passes (because the test is
listed in the output), but it is actually not tested. Wouldn't there be a way to
run only the right tests?

Okay, I tried to improve the situation. I introduced a derived test
class that only uses the mock data source. I also updated some other
test cases that excluded in-memory due to the need for adding RRs
dynamically, at the cost of additional setup complexity.

This leaves cases that are disabled due to actual bugs of the
corresponding data sources. At least for in-memory I expect they'll
be fixed and re-enabled soon (#2585). If the SQLite3 case is really
difficult as I expected in #2586, they'll remain disabled this way
until they are fixed, but there are only two cases, so I think it's
acceptable.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:15 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I noticed two minor things. This comment in the generator script talks about
various forms, but now there's only one.

This is a supplemental script to generate various forms of test data
from a unified source file.

Also, the addRRsets function does not (if I'm not mistaken) work twice in a
row for in-memory. It'd remove the RR sets added by the first call during the
second call. There should at least be a comment about it (as the two calls are
not needed), or it should be solved.

It could probably be solved by getting its iterator, dumping the content of the
in-memory to file, then append the RRsets to add and load that one. But maybe
that's just needlessly complicated.

So, if you're going to add just the comment, I think I don't need to review it
again.

comment:16 in reply to: ↑ 15 Changed 7 years ago by jinmei

Replying to vorner:

I noticed two minor things. This comment in the generator script talks about
various forms, but now there's only one.

This is a supplemental script to generate various forms of test data
from a unified source file.

Oh, good catch, fixed.

Also, the addRRsets function does not (if I'm not mistaken) work twice in a
row for in-memory. It'd remove the RR sets added by the first call during the
second call. There should at least be a comment about it (as the two calls are
not needed), or it should be solved.

Good point...

It could probably be solved by getting its iterator, dumping the content of the
in-memory to file, then append the RRsets to add and load that one. But maybe
that's just needlessly complicated.

...yeah, I think it's okay with the restriction for now. I've added
comments, and also added explicit an ASSERT test to detect any
accidental duplicate call.

There's one additional missing EXTRA_DIST, so I've added it, too.

I believe all of these are trivial, so I've merged it to master, and
am now closing the ticket.

comment:17 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 17
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.