BIND 10 #2353: write tests for all methods of Bob

[BIND 10 Development]

#2353: write tests for all methods of Bob
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121120
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  Boss   |           Sub-Project:  DNS
  of BIND                            |  Estimated Difficulty:  8
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:6 jinmei]:
 > '''test_socket_srv'''
 >
 > Ideally we should test some more things:
 > - remove_socket_srv really removes the directory and file and closes
 >   the socket (btw if we test these we do not need to reset
 >   `_srv_socket` etc in the main code).
 > - the local address of `_srv_socket` matches `_socket_path`.
 > - nothing happens if `srv_socket` is None
 > - after init_socket_srv it really listens on the socket

 Done.

 > '''test_get_processes'''
 >
 > - this doesn't confirm what happens if `components` has more than one
 >   components, and whether they are sorted by PID

 Checked.

 > - does runnable have to be set?

 Updated.

 > '''test_reap_children'''
 >
 > it doesn't cover some cases
 > {{{#!python
 >             except OSError as o:
 >                 if o.errno == errno.ECHILD:
 >                     break
 >             if pid in self.components:
 >                 if component.is_running() and self.runnable:
 >                     if not component_restarted:
 > }}}

 Checked.

 > '''test_kill_started_components'''
 >
 > - does runnable have to be set?

 Updated.

 > - it doesn't check kill() is called with parameter of `True`

 Checked.

 > - isn't it better to check if `components` is an empty dict directory?

 Checked now. :)

 > '''test_start_process
 >
 > - if not very difficult, I'd like to use some mock `ProcessInfo` and
 >   check more detailed behavior like whether the expected parameters
 >   are is passed or spawn() is called.

 Done.

 > - in addition to that, in this case maybe invoking an actual process
 >   may be a good idea (normally it's controversial for unit tests), but
 >   /bin/true is not always available.  Also, IMO such a test should
 >   actually go to tests for `ProcessInfo` because invoking processes is
 >   expensive and error prone (and it seems we actually already do it).

 I'm not sure what to check here. ProcessInfo's unittest runs `/bin/echo`
 and tests that the process is invoked.

 > - does runnable have to be set?

 Updated.

 > '''test_register_process'''
 >
 > - does runnable have to be set?

 Updated.

 > - isn't it better to examine components directly?

 Updated.

 > '''test_start_simple'''
 > - does runnable have to be set?

 Updated.

 > - same comments as those for started_components apply

 Done.

 > - verbose case isn't tested

 Done.

 > - in this case I actually think it's sufficient if we confirm
 >   start_process() (we'd fake it for the test) is called with expected
 >   params...and then noticed test_start_auth2, etc do it.  I believe we
 >   can do the same for this method.

 Done. :)

 > '''test_start_auth2/resolver2/cmdctl'''
 >
 > - if possible I'd avoid names like XXX2 because it's often ambiguous
 >   and easily subject to renumbering problems.  same comment applies to
 >   other similar ones.

 Done. It turns out that there's no clash in naming after all.

 > - verbose case isn't tested

 Done.

 > - for cmdctl, port option case isn't tested

 Done.

 > '''Others'''
 >
 > - I guess start_msgq() and start_cfgmgr() need similar tests like
 >   test_start_auth2

 Done. :)

 > - same for start_ccsession (checking if expected things are called
 >   with expected params)

 Done.

 > - from a quick look tests for startup() are not sufficient.

 Added a test for it.

 > - I was not 100% sure tests for shutdown() are sufficient.  Please
 >   check.

 This is checked in `__real_test_kill`, which seems sufficient to me.

 > - `_socket_data` itself doesn't seem to be tested, at least not
 >   sufficiently.

 Added a test for it.

 > - is run() sufficiently tested?  It was not clear from the existing
 >   tests.

 I think a direct test of `run()` is better done after a reorganization of
 code.

 Replying to [comment:8 jinmei]:
 > Replying to [comment:6 jinmei]:
 >
 > > '''Others'''
 > >
 > > - I guess start_msgq() and start_cfgmgr() need similar tests like
 > >   test_start_auth2
 >
 > In case of start_msgq(), it should also test this part:
 > {{{#!python
 >         while self.cc_session is None:
 >             # if we have been trying for "a while" give up
 >             if (time.time() - cc_connect_start) > 5:
 >                 logger.error(BIND10_STARTING_CC_FAIL)
 >                 raise CChannelConnectError("Unable to connect to
 c-channel after 5 seconds")
 >
 >             # try to connect, and if we can't wait a short while
 >             try:
 >                 self.cc_session = isc.cc.Session(self.msgq_socket_file)
 >             except isc.cc.session.SessionError:
 >                 time.sleep(0.1)
 > }}}
 > probably by stealing the time module temporarily.

 Done. :)

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