BIND 10 #466: duplicate code in xfrout

[BIND 10 Development]

#466: duplicate code in xfrout
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  minor    |            Milestone:  y2 12 month
                Component:  xfrout   |  milestone
                 Keywords:           |           Resolution:
Estimated Number of Hours:  0.0      |            Sensitive:  0
                Billable?:  1        |  Add Hours to Ticket:  0
                Internal?:  0        |          Total Hours:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:5 zzchen_pku]:

 '''xfrout.py.in'''
 > >  - I think _zone_is_empty() is not intuitive in the context of how it
 is used and what it actually does.  What it actually does is to test if a
 specified zone has an SOA RR.  That is, it could return True even if a
 zone isn't "empty" (in that it has some RRs). I'd rename it to
 zone_has_soa(), and use it with "not".
 > >  - I'd like to see more description for _zone_is_empty() and
 _zone_exist().  Specifically, I'd like to see an explanation how we use
 these two sets of functions.
 > Done.

 I've made some additional suggested updates.  Those include:
  - replaced "NS" with "name server" because "NS" looks like an RR type and
 can be confusing.
  - s/soa/SOA

 '''sqlite3_ds.py'''
 > >  - I'd like zone_exist (or whatever new result) to have more complete
 pydoc style documentation.  It would include description of param/return,
 and provide more detailed explanation (e.g. what "zone exists" means),
 explain in which case an exception can be raised and which exception.
 > I noticed that sqlite3_ds doesn't have any unittests before, I think it
 is because:
 > {{{
 > Test-driven development is difficult to use in situations where full
 functional tests are
 > required to determine success or failure. Examples of these are user
 interfaces, programs
 > that work with databases, and some that depend on specific network
 configurations.
 > }}}
 > It's difficult to test database behaviors. I have updated zone_exist
 according to TDD, and found that we need to use too much fakes and mocks,
 so I wonder whether TDD applies to this specific situation?

 I see some valid points here.  In particular, it's true that writing
 unittests for DB related stuff is difficult (in fact, in a commoly adoped
 definition it wouldn't be considered "unittest").  However:
  - it still cannot be a reason for skipping tests, even if it's not test
 "driven".
  - in case of sqlite3 it should still be relatively easier to provide
 tests because we only need library calls (and pre-populated test DB file),
 i.e, we don't need a separate process or network-based transactions.
  - in fact, the C++ version of sqlite3 data source has detailed "unit"
 tests.
  - so I suspect the main reason why the python version of sqlite3 data
 source didn't have tests was because we made compromise due to the hard
 deadline for the year1 deliverable (or we were simply lazy:-)
  - whether this could/should be test "driven" may be debatable, but for
 the above reasons in this specific case I believe it can, and if it can I
 believe it shoud.
  - at the bottom line I don't see a valid excuse to skip tests for the
 sqlite3 data source python module.
  - and, in fact, it looks like you found and fixed a bug thanks to writing
 the test:-)

 As for documentation, I'd like to use the pydoc style comments.
 Eventually that would apply to other functions of this file.  I wouldn't
 request that within the scope of this ticket, but let's at least use it
 for newer code.

 '''sqlite3_ds_test.py'''
  - I suspect the copyright year should be 2011:-)
  - in this case I suspect we cannot use mocks because this module is our
 interface to sqlite3.  For tests of a user app of this module such as
 xfrout we can use a mock data source, but tests for sqlite3 specific
 module should use sqlite3.
  - exception case(s) should also be tested.

-- 
Ticket URL: <https://bind10.isc.org/ticket/466#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development