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