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

[BIND 10 Development]

#2353: write tests for all methods of Bob
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  Boss of BIND  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  8             |                 CVSS Scoring:
         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:

 Hi Jinmei

 Replying to [comment:10 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've retained the hack in just `start_msgq()` so that testing is a bit
 cleaner.
 But this now uses a different variable `run_under_unittests` and does not
 overload any others.

 The other use of such a hack is gone.

 > - I'd add description for `_make_process_info`, especially about its
 >   purpose, and why it's "protected".

 Done.

 > - 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.

 This hack is gone.

 > - start_ccsession: is it necessary to modify it to return self.ccs?
 >   can't the test just inspect bob.ccs?

 Done.

 > '''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))`

 Done. :)

 > - 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)
 > }}}

 Done.

 > - 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)
 > }}}

 I prefer the former syntax actually. :) For any kind of assertRaises()
 which may check a block of code for an exception, the `with` is
 consistent syntax.

 > - 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)

 Done. This True/False check works similar to how we test for true/false
 in `scoped_ptr`, etc. in C++. Anyway, the suggested syntax makes it
 clearer.

 > - 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.

 All are fixed.

 > - 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.

 Done.

 > - 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.

 We check now.

 > - 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.

 Intrusions are gone, except for that one `run_under_unittests` boolean
 which helps to split the unittests into two.

 > - 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)
 > }}}

 *facepalm*. Fixed. :)

 > - test_start_msgq_timeout: I think it's safer to restore time.time in
 >   tearDown().

 Done.

 > - 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).

 Agreed and fixed.

 > - test_start_cfgmgr_timeout: same sense of comments applies as the
 >   msgq counterpart.  also about the comment on the main code.

 Done.

 > - test_start_ccsession: it's probably safer to restore
 >   `isc.config.ModuleCCSession` in `tearDown`.

 Done.

 > - test_register_process: I'd check the value of `self.components[53]`,
 too.

 Done.

 > - test_start_auth: the env parameter doesn't seem to be checked.

 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.

 Unified. But the code still uses separate checkers per each tested method.
 Otherwise the test code gets a bit ugly with more unification.

 > - 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.

 Done.

 > - test_start_cmdctl: c_channel_env doesn't seem to be checked.

 Checked now.

 > - 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')
 > }}}

 This has been clarified with a comment. It's just to check that it saves
 back the read data when `EAGAIN` is returned.

 > - test_socket_data: I'd use `assertEqual` with `{}`:
 > {{{#!python
 >         self.assertFalse(bob._unix_sockets)
 > }}}

 Done.

 > - test_startup: `BoB.c_channel_env` isn't checked (either {} or
 >   containing BIND10_MSGQ_SOCKET_FILE, depending on
 self.msgq_socket_file).

 Updated.

 > - 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.

 These two are also fixed now.

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