Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#85 closed task (fixed)

test case for ./DS query

Reported by: jinmei Owned by: UnAssigned
Priority: low Milestone:
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description (last modified by jreed)

See DataSrcTest?.RootDSQuery.

Even though a DS query for the root name is not sensible, it shouldn't trigger an internal exception. In fact, both f-root (BIND9) and k-root (BIND9) simply treat it as ordinary query and returns an empty answer (NXRRSET).

I categorized this "critical" as it could crash a BIND10 program from a remote packet ((un)fortunately the auth server catches the exception and converts it to SERVFAIL, but I don't think it's the right action for this query).

Subtickets

Change History (16)

comment:1 Changed 10 years ago by jinmei

  • Owner set to each
  • Status changed from new to assigned

comment:2 Changed 10 years ago by each

  • Owner changed from each to jinmei

Fixed in r1433 and unit test enabled. It now queries for ./DS at the root zone, rather than trying to find its nonexistent parent. This matches BIND9 behavior.

comment:3 Changed 10 years ago by jinmei

  • Owner changed from jinmei to each

unittest is incomplete. it should examine the result using EXPECT_EQ (or EXPECT_THROW if this case is expected to cause exception)

comment:4 Changed 10 years ago by each

With the current test data there shouldn't really be a result. The test data source isn't authoritative for the root zone. The purpose of the test was to verify that this query doesn't cause a segmentation fault anymore. But I'll add EXPECT_NO_THROW and check for REFUSED.

comment:5 follow-up: Changed 10 years ago by jinmei

I would add a root test zone and specifically test the expected results. I did a similar thing for other tests.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by each

I would add a root test zone and specifically test the expected results. I did a similar thing for other tests.

A root zone in the test data source sounds like a good idea--not just for this but for other queries--but is it necessary for Y1?

(I have loaded a root zone and tested it, BTW; I just haven't incorporated it into the unit tests as yet.)

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

I would add a root test zone and specifically test the expected results. I did a similar thing for other tests.

A root zone in the test data source sounds like a good idea--not just for this but for other queries--but is it necessary for Y1?

I wish I could say yes, but in reality we deferred so many tests as a post-Y1 fodder with an excuse of tough deadline. From now on, we shouldn't repeat this: No added feature that lacks sufficient and explicit tests. If we can't complete tests, what we should do is to drop the corresponding feature, not releasing it without tests.

(This is a general comment for the development process, rather than a specific response to this ticket)

(I have loaded a root zone and tested it, BTW; I just haven't incorporated it into the unit tests as yet.)

I'm giving this ticket back to you - please complete the test case and then (after confirming it) close the ticket.

comment:8 follow-up: Changed 10 years ago by shane

  • Component changed from Unclassified to b10-auth

Has this been fixed?

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 10 years ago by each

Replying to shane:

Has this been fixed?

Yes, back in the Y1 release. However, Jinmei asked me to add some unit tests with the test data source serving a simulated root zone, and I haven't done that yet. We could close this ticket and open a new one, if you like.

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

  • Milestone changed from 03. 1st Incremental Release to feature backlog item
  • Priority changed from critical to minor
  • Summary changed from ./DS query triggers an exception to test case for ./DS query

Replying to each:

Replying to shane:

Has this been fixed?

Yes, back in the Y1 release. However, Jinmei asked me to add some unit tests with the test data source serving a simulated root zone, and I haven't done that yet. We could close this ticket and open a new one, if you like.

comment:11 in reply to: ↑ 9 Changed 10 years ago by jinmei

  • Type changed from defect to task

(oops, failed to update the comment field) I'm also changing the type from defect to task.

Replying to each:

Replying to shane:

Has this been fixed?

Yes, back in the Y1 release. However, Jinmei asked me to add some unit tests with the test data source serving a simulated root zone, and I haven't done that yet. We could close this ticket and open a new one, if you like.

Okay, this doesn't have to be in the 2nd release, so I'll lower the priority, update the subject, and move it to the backlog.

comment:12 Changed 9 years ago by jreed

  • billable set to 0
  • Description modified (diff)
  • Estimated Difficulty set to 0.0
  • Internal? unset

comment:13 Changed 9 years ago by vorner

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

I added the test (I hope I understood correctly which test should be added), it is in branches/trac85 (r3833).

Do we want a changelog entry for this? This seems really small change with no effect on functionality or API.

comment:14 Changed 9 years ago by stephen

Sees OK - please merge.

comment:15 Changed 9 years ago by vorner

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

Merged, closing.

comment:16 Changed 9 years ago by stephen

  • Milestone feature backlog item deleted

Milestone feature backlog item deleted

Note: See TracTickets for help on using tickets.