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

Comment (by jinmei):

 The revised version looks pretty good.  I have a few minor followup
 comments.

 Replying to [comment:12 muks]:

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

 Is this addressed?  I don't see changes on this point, or am I missing
 something?

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

 What about test_start_cfgmgr and test_start_simple?

 Some other points:

 '''bind10_src.py.in'''

 - I'd rename `Bob.run_under_unittests` to `_run_under_unittests` to
   (weakly) indicate apps shouldn't touch/refer to it.  I'd also
   add a comment about what it is where it's defined.

 '''bind10_test.py.in'''

 - test_start_msgq_timeout: this comment doesn't seem to be really correct:
 {{{#!python
         # keep the timeout small for the test to complete quickly
         bob.msgq_timeout = 1
 }}}
   Even if it's 5 the test will be done "quickly".
 - test_start_msgq_timeout: this comment doesn't seem to be correct or
   at least confusing:
 {{{#!python
         # 1 second of attempts every 0.1 seconds should result in 10
         # attempts. 1 attempt happens at the start. 2 more attempts seem
         # to happen inside start_msgq().
         self.assertEqual(attempts, 13)
 }}}
   time.time() should be called 12 times within the while loop:
   starting from 0, and 11 more times from 0.1 to 1.1.  There's another
   call to time.time() outside the loop, which makes it 13.

 - test_start_cfgmgr_timeout: awkward comment.  see above.
 {{{#!python
         # keep the wait time small for the test to complete quickly
         bob.wait_time = 2
 }}}
 - test_start_cfgmgr_timeout: this should use `assertRaises`:
 {{{#!python
         thrown = False
         # An exception will be thrown here when it eventually times out.
         try:
             pi = bob.start_cfgmgr()
         except bind10_src.ProcessStartError as e:
             thrown = True

         # We just check that an exception was thrown, and that several
         # attempts were made to connect.
         self.assertTrue(thrown)
 }}}

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

 So it's just an arbitrary choice?  In that case I'd simply say so in
 the comment.

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