BIND 10 #527: refactoring auth/query_unittests

[BIND 10 Development]

#527: refactoring auth/query_unittests
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  task     |               Status:  reviewing
                 Priority:  major    |            Milestone:  A-Team-
                Component:           |  Sprint-20110126
  b10-auth                           |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  2.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:4 vorner]:

 Thanks for the prompt review.  I believe I've addressed all comments.

 > It's nice you took this, the MockZone was getting on my nerves as well a
 little. But I have few smaller suggestions:
 >  * It might be worth having another overloaded wrapper for rrsetsCheck,
 taking const std::string&, most of the times it would save one line of
 code to simply pass the string.

 Ah, good suggestion.  Done, and modified the query_unittest to use
 the string version (3ccbc93).

 >  * If rrsetsCheck gets confused if someone puts the same tuple of (name,
 type, class) into the result we check, how much confused results would it
 give? I believe that having the same tuple there means bug, would it fail
 the test, even with a wrong error message at last?

 Hmm, after thinking about it more, I noticed there can be a false
 negative.
 So I decided to check duplicates explicitly so that we won't miss a bug
 due to an error of test data (1a11edc).

 >  * I didn't see the doxygen comment near headerCheck function. I know it
 didn't have it and you just moved it, but one would be nice.

 I was afraid you pointed this out:-)  Now that you've done it, it would be
 good to complete it.  So, done (a8793f5 and 1916a92).

 >  * Looking at the tests, if there was a single call that would combine
 headerCheck and 3 rrsetsCheck calls with the three sections would save a
 lot of code as well. Would you like to introduce such a wrapper?

 Yeah, this is another thing I also considered.  I think this is generally
 a
 good idea, so done. (77c5ed7)

 >  * The MX test doesn't test the authority section. It probably didn't
 before as well, but every other test seems to do so, maybe for
 consistency, it should be added?

 We could, but I'd rather focus on the essential part for conciseness.
 For example, in many cases the authority section would have the same
 content, and if the test writer doesn't care about it, it would be
 (a bit) easier to specify NULL rather than identifying the expected
 value by looking at other tests.  As for consistency, the MXAlias test
 also
 skips (skipped) checking the authority section. So MX is not so special.

 At the moment I'll keep skipping these checks.

 > Anyway, there's nothing serious. If you need this for #526, you probably
 don't have to wait for the next round of review (I don't expect there
 would be anything substantial), you can just probably merge it to your
 #526 branch before merging to trunk.

 Actually, #526 will not so heavily depend on this task if we take the
 simpler initial step as Evan suggested.  Some of the new code may be
 a bit beyond the trivial level (especially for the new string version
 and duplicate checks), so I'll wait for a while.  But now you seem to
 have gone, and a weekend is coming, so if you are not back by the end
 of my day, I'll merge the branch while keeping the ticket open.

-- 
Ticket URL: <http://bind10.isc.org/ticket/527#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development