Opened 7 years ago

Closed 7 years ago

#2223 closed defect (fixed)

Update tests for code disabled in rrsetsCheck()

Reported by: muks Owned by: jelte
Priority: medium Milestone: Sprint-20121023
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 1.25 Internal?: no

Description

As part of #2165, some code that compared vector index differences in rrsetsCheck() (in dnsmessage_test.h) was commented out. This code doesn't make sense anymore as RRSIGs are no longer inline in Message class's rrsets vector.

A similar way of achieving the same effect as what that code checked for should be implemented. This could be something simple, or could mean modifying several tests.

Subtickets

Change History (18)

comment:1 Changed 7 years ago by vorner

  • Milestone changed from New Tasks to Next-Sprint-Proposed

As a ticket follow-up/cleanup, this should be done as soon as possible.

comment:2 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jelte

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

we ran out of tickets, moving this one to current sprint (if not taken at sprint planning we can re-discuss then)

comment:4 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte
  • Status changed from new to assigned

comment:5 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review

I kept it compatible to existing tests; it uses a rather inefficient, but effective comparison technique; instead of just checking distances it converts all rrsets in both iterators to the RRset string representation, sorts the resulting vectors, and compares them. Now the entire list and their actual contents are checked.

To be compatible with RRSigs that are in the iterator as separate RRsets, they are split out (so an RRset that has inline signatures gets two strings in the vector, one for the rrset data itself, one for the signatures).

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

Replying to jelte:

Ready for review

Hmm, maybe I misunderstand something, but doesn't this version do
something redundant? In the first for loop it actually compares the
content of the two sets of RRsets:

        if (found_rrset_it != expected_end) {
            rrsetCheck(*found_rrset_it, *it);
            ++rrset_matched;
            rrset_matched += (*it)->getRRsigDataCount();
        }

(although it doesn't handle "embedded" RRSIGs in the actual sets).
The latter half of the original version just check if there's any
missing or redundant RRset by checking the redundant.

So, except for the RRSIGs handling it already does what the string
comparison does.

Also, comparing toText() results is less reliable because it doesn't
take into account case-insensitive RDATA fields. That's why
rrsetCheck() uses Rdata::compare().

If my understanding so far is correct, I guess what should be done is
to "flatten" the "actual" sets, extracting RRSIGs from RRsets between
actual_begin and actual_end and reconstruct actual_begin and
actual_end (it would be okay to just naively build a fresh
vector<ConstRRset> and use its bein() and end()) before the first
for loop. I guess you don't have to touch the expected sets since
they should be already flattened as commented in the original version.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jelte

comment:8 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

True, it would incorrectly fail on case, which doesn't actually happen.

But as noted on jabber, and in keeping with the original intent, in that case it can be done even easier, by just looping over the iterators and counting the number of rrsets, counting signed rrsets twice.

This makes the replacement even smaller

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

Replying to jelte:

True, it would incorrectly fail on case, which doesn't actually happen.

But as noted on jabber, and in keeping with the original intent, in that case it can be done even easier, by just looping over the iterators and counting the number of rrsets, counting signed rrsets twice.

This makes the replacement even smaller

Hmm, does this version regain the very original (pre 'if 0') behavior?
It seems it doesn't compare the content of RRSIGs, so I suspect it
passes the case like this:

Actual iterator: RRset-A (containing RRSIG RRset-A'), RRset-B
Expected iterator: RRset-A, RRSIG RRset-A, RRset-B

If I remember it correctly, in the pre 'if 0' version, "actual
iterator" separated RRset-A and contained RRSIG RRset, and it's
compared to RRset-A in the for loop. So we should have been able to
detect these two sets are not equal.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

OK, take 3, with a bit of both earlier iterations :)

presuming the checkRRset keeps its behaviour of not checking sigs, sigs from both iterators are now pulled, and added to vectors, and the original first part of the code (that checks for duplicates in actual, and for the existence of all 'actual' sets in expected) now runs on those vectors.

While this code isn't effient anyway, I did put the toText() conversion used in the scoped_trace in that helper function as well (and remove the RRsetDumper), so we don't have to loop yet another time.

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

In terms of functionality this version looks okay. I have a few more
minor comments.

  • pullSigs(): do we need to call toText() for the RRSIG? I thought RRset::toText() now includes both the main and (if any) RRSIGs.
  • s/checkRRset()/rrsetCheck()/?
        // separate RRsets (checkRRset() later does not check signatures
    
  • very minor point, but I suggest placing expected_rrsets first:
        std::vector<isc::dns::ConstRRsetPtr> actual_rrsets, expected_rrsets;
    
    as "expectedXXX" are generally placed first in this function (and that's gtest's EXPECT_xx convention).
  • this comment doesn't seem to match the actual code any more:
            // search).  Since the actual set is guaranteed to be unique, we can
            // detect it if the expected data has a duplicate by the match/size
            // checks at the end of the function.
    
    I guess this should now be something like "By guaranteeing the actual set is unique and the size of both vectors are the same, we can conclude the two sets are identical after this loop".

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

In terms of functionality this version looks okay. I have a few more
minor comments.

  • pullSigs(): do we need to call toText() for the RRSIG? I thought RRset::toText() now includes both the main and (if any) RRSIGs.

the default toText() implementation certainly does not

  • s/checkRRset()/rrsetCheck()/?
        // separate RRsets (checkRRset() later does not check signatures
    

ack, changed

  • very minor point, but I suggest placing expected_rrsets first:
        std::vector<isc::dns::ConstRRsetPtr> actual_rrsets, expected_rrsets;
    
    as "expectedXXX" are generally placed first in this function (and that's gtest's EXPECT_xx convention).

ok

  • this comment doesn't seem to match the actual code any more:
            // search).  Since the actual set is guaranteed to be unique, we can
            // detect it if the expected data has a duplicate by the match/size
            // checks at the end of the function.
    
    I guess this should now be something like "By guaranteeing the actual set is unique and the size of both vectors are the same, we can conclude the two sets are identical after this loop".

ok, changed

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

Replying to jelte:

  • pullSigs(): do we need to call toText() for the RRSIG? I thought RRset::toText() now includes both the main and (if any) RRSIGs.

the default toText() implementation certainly does not

Hmm, that seems to be different from the TreeNodeRRset
specialization. We should be consistent, but that should go to a
separate task.

While that could cause some confusion when this test fails, I think
it's okay for now since it only happens when test fails and it's used
in SCOPED_TRACE().

I've made one last small editorial fix. With that please merge.

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 1.25

comment:18 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged, closing ticket.

Note: See TracTickets for help on using tickets.