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:13 jinmei]:
 > 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?

 The `assertFalse()` bit was addressed. But I feel that the
 `get_processes()` interface is the right way to test components in this
 particular test (for other tests, I have changed it to directly query
 `BoB.components`). I'm sorry I didn't reply to this particular review
 comment last time.

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

 I had left a couple of them as empty to have an empty env test (the others
 had a populated environment). Anyway, have populated them now. :)

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

 Done.

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

 Updated.

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

 Replaced the old comment with the text above.

 > - test_start_cfgmgr_timeout: awkward comment.  see above.
 > {{{#!python
 >         # keep the wait time small for the test to complete quickly
 >         bob.wait_time = 2
 > }}}

 Updated.

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

 Eek. I thought I had got all of them the last time. Fixed.

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

 This is now explained with a comment.

 Also made some cleanup.

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