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

BIND 10 Development do-not-reply at isc.org
Mon Nov 26 06:41:02 UTC 2012


#2353: write tests for all methods of Bob
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121204
                   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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''bind10_src.py.in'''

 - start_msgq: this hack doesn't look clean:
 {{{#!python
             # this is only used by unittests
             if self.msgq_timeout == 0:
                 break
 ...
         if self.cc_session is not None:
 }}}
   in particular, the relationship between the test-only hack and the
   additional `if` is not immediately clear.  I'd primarily avoid the
   hack; if it's too difficult, I'd at least avoid the `if` (from a
   quick look it seems possible); if even it's too difficult I'd at
   least clarify the relationship in comment (last resort).
 - I'd add description for `_make_process_info`, especially about its
   purpose, and why it's "protected".
 - start_cfgmgr: is this safe?
 {{{#!cpp
         # wait_time is set to 0 only by unittests
         if self.wait_time > 0 and not self.process_running(msg,
 "ConfigManager"):

 }}}
   At least this comment isn't true because wait_time can be set via
   command line option.  And when set to 0, it bypasses the check for
   process_running().  In general, I'd avoid changing the semantics of
   the tested code this way.  As this case indicates, it's often not
   immediately clear whether the added behavior doesn't interfere with
   the non test case.
 - start_ccsession: is it necessary to modify it to return self.ccs?
   can't the test just inspect bob.ccs?

 '''bind10_test.py.in'''

 - test_get_processes: a minor point, but why this?
 {{{#!python
         pids = []
         pids.extend(range(0, 20))
 }}}
   it seems we can simply do `pids = list(range(0, 20))`
 - test_reap_children: isn't it better to check this as a list?
 {{{#!python
         #self.assertFalse(bob.components_to_restart)
         self.assertEqual([], bob.components_to_restart)
 }}}
 - what's the purpose of `with`?
 {{{#!python
         with self.assertRaises(OSError):
             bob.reap_children()
 }}}
   can't we simply call assertRaises directly?
 {{{#!python
         self.assertRaises(OSError, bob.reap_children)
 }}}
 - test_kill_started_components: I guess I commented about this in the
   previous cycle, but: isn't it better to check the value of
   components directly, rather than indirect check via get_processes?
 {{{#!python
         self.assertEqual([[53, 'test', 'Test']], bob.get_processes())
 ...
         self.assertFalse(bob.get_processes())
 }}}
   (besides, `assertFalse` doesn't seem to be a good choice for non
   boolean object)
 - test_start_msgq: I'd use a non trivial (non empty) value for pi.env:
 {{{#!python
         self.assertEqual({}, pi.env)
 }}}
    this applies to other similar cases.
 - test_start_msgq_timeout: isn't it better to completely control the
   behavior of time.time() (e.g. return i on i-th call) and
   time.sleep()?  Then we can avoid making it time consuming regardless
   of the number of retries, and can make the test more deterministic
   and reliable.
 - test_start_msgq_timeout: doesn't seem to check isc.cc.Session() is
   called
 - test_start_msgq_timeout (or the test without "timeout"): likewise,
   doesn't seem to check group_subscribe() is called.
 - test_start_msgq_timeout: see also the comment on the main code.  I
   guess we can avoid intruding the main code if we control the timing
   completely.
 - test_start_msgq_timeout: why not just use `assertRaises`?
 {{{#!python
         thrown = False
         # An exception will be thrown here when it eventually times out.
         try:
             pi = bob.start_msgq()
         except bind10_src.CChannelConnectError as e:
             thrown = True
         # We just check that an exception was thrown, and that several
         # attempts were made to connect.
         self.assertTrue(thrown)
 }}}
 - test_start_msgq_timeout: I think it's safer to restore time.time in
   tearDown().
 - test_start_cfgmgr: I think `DummySession` should return a real
   message from group_recvmsg() and the test confirms the behavior
   explicitly, rather than by tweaking wait_time and bypassing the
   check (it's cheating).
 - test_start_cfgmgr_timeout: same sense of comments applies as the
   msgq counterpart.  also about the comment on the main code.
 - test_start_ccsession: it's probably safer to restore
   `isc.config.ModuleCCSession` in `tearDown`.
 - test_register_process: I'd check the value of `self.components[53]`,
 too.
 - test_start_auth: the env parameter doesn't seem to be checked.
 - test_start_auth, etc: btw, I see a similar pattern of 'non-verbose'
   and 'verbose' is repeating for various components.  They are mostly
   a mere copy.  I'd consider unifying them.
 - test_start_resolver: same for test_start_auth.  and, hmm, I see
   some possibilities of cleaning up the tested method...but that
   should probably go to a separate ticket.
 - test_start_cmdctl: c_channel_env doesn't seem to be checked.
 - test_socket_data: what's the rationale of the magic number of 15?
 {{{#!python
                 if self.throw and self.i > 15:
                     raise socket.error(errno.EAGAIN, 'Try again')
 }}}
 - test_socket_data: I'd use `assertEqual` with `{}`:
 {{{#!python
         self.assertFalse(bob._unix_sockets)
 }}}
 - test_startup: `BoB.c_channel_env` isn't checked (either {} or
   containing BIND10_MSGQ_SOCKET_FILE, depending on self.msgq_socket_file).
 - test_startup: this case isn't tested:
 {{{#!python
             logger.fatal(BIND10_MSGQ_ALREADY_RUNNING)
             return "b10-msgq already running, or socket file not cleaned ,
 cannot start"
 }}}
 - test_startup: related to the previous point, does this test use the
   real `isc.cc.Session` class? if so, it's probably not a good idea as
   it involves network communication.

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


More information about the bind10-tickets mailing list