Opened 7 years ago

Closed 7 years ago

#2353 closed defect (fixed)

write tests for all methods of Bob

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121218
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 8 Add Hours to Ticket: 6.5
Total Hours: 0 Internal?: no

Description (last modified by jinmei)

As mentioned in http://bind10.isc.org/ticket/2244#comment:11,
the Bob class is currently only poorly tested (there are several
methods that have no tests). It's not good, particularly because
it has very important roles.

I guess it's partly because of a historical reason (the implementation
has been there since we were not strict about writing tests) and
partly because it relies on some low level features like process
handling. Obviously the first one cannot be an excuse, and I don't
think the second one a very difficult issue either, since it's quite
easy to steal such low layer interfaces.

So, in this ticket I propose writing tests for all untested methods of
the Bob class. For the scope of this ticket it's probably okay to
just provide some normal and major error cases (ignoring tricky corner
cases for now).

On top of that I'd suggest refactoring the class to improve
readability. It's now a very big and monolithic class consisting of
several hundresds of lines. I guess we should consider extracting
some of the responsibility to some helper classes or other refactoring
techniques to improve overall readability. But that would be the topic
of a separate ticket.

Subtickets

Change History (18)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121106

comment:3 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:4 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing
  • ProcessInfo class is tested.
  • In BoB, the following are already tested:
    config_handler
    command_handler
    start_all_components
    stop_process
    component_shutdown
    shutdown
    restart_processes
    socket_request_handler
    socket_consumer_dead
    run
    
  • In BoB, the following tests were added:
    test_reap_children
    test_get_processes
    test_kill_started_components
    test_start_process
    test_register_process
    test_start_auth
    test_start_resolver
    test_socket_srv
    
  • In BoB, the following are not directly tested:
    log_starting
    log_started
    start_msgq
    start_cfgmgr
    start_ccsession
    startup
    
  • In the bind10_src module, the following are already tested:
    dump_pid
    unlink_pid_file
    remove_lock_files
    
  • In the bind10_src module, the following tests were added:
    test_get_signame
    test_fatal_signal
    

comment:5 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 follow-up: Changed 7 years ago by jinmei

test_socket_srv

Ideally we should test some more things:

  • remove_socket_srv really removes the directory and file and closes the socket (btw if we test these we do not need to reset _srv_socket etc in the main code).
  • the local address of _srv_socket matches _socket_path.
  • nothing happens if srv_socket is None
  • after init_socket_srv it really listens on the socket

test_get_processes

  • this doesn't confirm what happens if components has more than one components, and whether they are sorted by PID
  • does runnable have to be set?

test_reap_children

it doesn't cover some cases

            except OSError as o:
                if o.errno == errno.ECHILD:
                    break
            if pid in self.components:
                if component.is_running() and self.runnable:
                    if not component_restarted:

test_kill_started_components

  • does runnable have to be set?
  • it doesn't check kill() is called with parameter of True
  • isn't it better to check if components is an empty dict directory?

test_start_process

  • if not very difficult, I'd like to use some mock ProcessInfo and check more detailed behavior like whether the expected parameters are is passed or spawn() is called.
  • in addition to that, in this case maybe invoking an actual process may be a good idea (normally it's controversial for unit tests), but /bin/true is not always available. Also, IMO such a test should actually go to tests for ProcessInfo because invoking processes is expensive and error prone (and it seems we actually already do it).
  • does runnable have to be set?

test_register_process

  • does runnable have to be set?
  • isn't it better to examine components directly?

test_start_simple

  • does runnable have to be set?
  • same comments as those for started_components apply
  • verbose case isn't tested
  • in this case I actually think it's sufficient if we confirm start_process() (we'd fake it for the test) is called with expected params...and then noticed test_start_auth2, etc do it. I believe we can do the same for this method.

test_start_auth2/resolver2/cmdctl

  • if possible I'd avoid names like XXX2 because it's often ambiguous and easily subject to renumbering problems. same comment applies to other similar ones.
  • verbose case isn't tested
  • for cmdctl, port option case isn't tested

Others

  • I guess start_msgq() and start_cfgmgr() need similar tests like test_start_auth2
  • same for start_ccsession (checking if expected things are called with expected params)
  • from a quick look tests for startup() are not sufficient.
  • I was not 100% sure tests for shutdown() are sufficient. Please check.
  • _socket_data itself doesn't seem to be tested, at least not sufficiently.
  • is run() sufficiently tested? It was not clear from the existing tests.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by jinmei

Replying to jinmei:

Others

  • I guess start_msgq() and start_cfgmgr() need similar tests like test_start_auth2

In case of start_msgq(), it should also test this part:

        while self.cc_session is None:
            # if we have been trying for "a while" give up
            if (time.time() - cc_connect_start) > 5:
                logger.error(BIND10_STARTING_CC_FAIL)
                raise CChannelConnectError("Unable to connect to c-channel after 5 seconds")

            # try to connect, and if we can't wait a short while
            try:
                self.cc_session = isc.cc.Session(self.msgq_socket_file)
            except isc.cc.session.SessionError:
                time.sleep(0.1)

probably by stealing the time module temporarily.

comment:9 in reply to: ↑ 8 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

test_socket_srv

Ideally we should test some more things:

  • remove_socket_srv really removes the directory and file and closes the socket (btw if we test these we do not need to reset _srv_socket etc in the main code).
  • the local address of _srv_socket matches _socket_path.
  • nothing happens if srv_socket is None
  • after init_socket_srv it really listens on the socket

Done.

test_get_processes

  • this doesn't confirm what happens if components has more than one components, and whether they are sorted by PID

Checked.

  • does runnable have to be set?

Updated.

test_reap_children

it doesn't cover some cases

            except OSError as o:
                if o.errno == errno.ECHILD:
                    break
            if pid in self.components:
                if component.is_running() and self.runnable:
                    if not component_restarted:

Checked.

test_kill_started_components

  • does runnable have to be set?

Updated.

  • it doesn't check kill() is called with parameter of True

Checked.

  • isn't it better to check if components is an empty dict directory?

Checked now. :)

test_start_process

  • if not very difficult, I'd like to use some mock ProcessInfo and check more detailed behavior like whether the expected parameters are is passed or spawn() is called.

Done.

  • in addition to that, in this case maybe invoking an actual process may be a good idea (normally it's controversial for unit tests), but /bin/true is not always available. Also, IMO such a test should actually go to tests for ProcessInfo because invoking processes is expensive and error prone (and it seems we actually already do it).

I'm not sure what to check here. ProcessInfo?'s unittest runs /bin/echo and tests that the process is invoked.

  • does runnable have to be set?

Updated.

test_register_process

  • does runnable have to be set?

Updated.

  • isn't it better to examine components directly?

Updated.

test_start_simple

  • does runnable have to be set?

Updated.

  • same comments as those for started_components apply

Done.

  • verbose case isn't tested

Done.

  • in this case I actually think it's sufficient if we confirm start_process() (we'd fake it for the test) is called with expected params...and then noticed test_start_auth2, etc do it. I believe we can do the same for this method.

Done. :)

test_start_auth2/resolver2/cmdctl

  • if possible I'd avoid names like XXX2 because it's often ambiguous and easily subject to renumbering problems. same comment applies to other similar ones.

Done. It turns out that there's no clash in naming after all.

  • verbose case isn't tested

Done.

  • for cmdctl, port option case isn't tested

Done.

Others

  • I guess start_msgq() and start_cfgmgr() need similar tests like test_start_auth2

Done. :)

  • same for start_ccsession (checking if expected things are called with expected params)

Done.

  • from a quick look tests for startup() are not sufficient.

Added a test for it.

  • I was not 100% sure tests for shutdown() are sufficient. Please check.

This is checked in __real_test_kill, which seems sufficient to me.

  • _socket_data itself doesn't seem to be tested, at least not sufficiently.

Added a test for it.

  • is run() sufficiently tested? It was not clear from the existing tests.

I think a direct test of run() is better done after a reorganization of code.

Replying to jinmei:

Replying to jinmei:

Others

  • I guess start_msgq() and start_cfgmgr() need similar tests like test_start_auth2

In case of start_msgq(), it should also test this part:

        while self.cc_session is None:
            # if we have been trying for "a while" give up
            if (time.time() - cc_connect_start) > 5:
                logger.error(BIND10_STARTING_CC_FAIL)
                raise CChannelConnectError("Unable to connect to c-channel after 5 seconds")

            # try to connect, and if we can't wait a short while
            try:
                self.cc_session = isc.cc.Session(self.msgq_socket_file)
            except isc.cc.session.SessionError:
                time.sleep(0.1)

probably by stealing the time module temporarily.

Done. :)

comment:10 follow-up: Changed 7 years ago by jinmei

bind10_src.py.in

  • start_msgq: this hack doesn't look clean:
                # 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?
            # 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?
            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?
            #self.assertFalse(bob.components_to_restart)
            self.assertEqual([], bob.components_to_restart)
    
  • what's the purpose of with?
            with self.assertRaises(OSError):
                bob.reap_children()
    
    can't we simply call assertRaises directly?
            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?
            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:
            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?
            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?
                    if self.throw and self.i > 15:
                        raise socket.error(errno.EAGAIN, 'Try again')
    
  • test_socket_data: I'd use assertEqual with {}:
            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:
                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.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

bind10_src.py.in

  • start_msgq: this hack doesn't look clean:
                # 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?
            # 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?
            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?
            #self.assertFalse(bob.components_to_restart)
            self.assertEqual([], bob.components_to_restart)
    

Done.

  • what's the purpose of with?
            with self.assertRaises(OSError):
                bob.reap_children()
    
    can't we simply call assertRaises directly?
            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?
            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:
            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?
            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?
                    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 {}:
            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:
                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.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jinmei

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

Replying to 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?
            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:
            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:
            # 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:
            # 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.
            # keep the wait time small for the test to complete quickly
            bob.wait_time = 2
    
  • test_start_cfgmgr_timeout: this should use assertRaises:
            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?
                    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.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:15 in reply to: ↑ 13 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Replying to 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?
            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:
            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:
            # 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:
            # 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.
            # keep the wait time small for the test to complete quickly
            bob.wait_time = 2
    

Updated.

  • test_start_cfgmgr_timeout: this should use assertRaises:
            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?
                    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.

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

Okay, I now think it's ready for merge.

comment:17 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 6.5

comment:18 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Pushed to master branch in commit c0be62cb0f27a0855c007beabeb0c3ba3c4fb0ff:

* 3e2d8b0 [2353] Update comments (recommended by review)
* 32011f9 [2353] Rename and document BoB._run_under_unittests
* 02f306c [2353] Unify repeated test code
* 6af3de2 [2353] Use a test environment instead of an empty one
* 366f210 [2353] Use assertRaises() instead of the excessive code
* e6ebe1d [2353] Check that BoB.start_msgq() calls isc.cc.Session() and group_subscribe()
* de66fdd [2353] Test BoB.startup() when socket already exists
* cdbe6de [2353] Remove unused assignments
* 029ac58 [2353] Rename variable
* e8a23d3 [2353] Remove run_under_unittests hack from BoB.start_cfgmgr()
* f7e3012 [2353] Make DummySession() return a real message in group_recvmsg()
* 4f80651 [2353] Restore isc.config.ModuleCCSession during tearDown()
* 87cd15b [2353] Restore time.time() and time.sleep() during tearDown()
* 3c17e95 [2353] Unify verbose and non-verbose cases
* bae03c0 [2353] Don't call real sleep() in mock function
* ec4581d [2353] Check if msgq_socket_file is carried over in BoB.startup()
* 70db0c5 [2353] Change assertion type for dict
* 609e5bf [2353] Explain why we throw after 15 recv()s, in a comment
* 71cf0a2 [2353] Test the env string in various tests
* 40d54f7 [2353] Test the values of component registered by BoB.register_process()
* 30e529a [2353] Make test more deterministic
* f1035ba [2353] Use assertRaises() instead of the excessive code
* 0740a5d [2353] Check an environment string in BoB.start_msgq() test
* 045d32e [2353] Check BoB.get_processes() return value as a list
* eba06ee [2353] Use assertIn() in some places
* 93d27cb [2353] Check BoB.components_to_restart as a list
* 8a966b8 [2353] Update syntax used, combining two statements into one
* 2c2459f [2353] Rewrite code that depended on BoB.start_ccsession() returning a value
* 145a4ed [2353] Don't run block of code when requested not to by unittests
* 41ccdad [2353] Use a flag instead of overriding BoB.wait_time
* 8682cdc [2353] Add description for BoB._make_process_info()
* 0688fdb [2353] Use a flag instead of overriding BoB.msgq_timeout
* cfdfb58 [2353] unrelated cleanup: removed an obsolete comment.
* 08a5a3d [2353] style fixes: folded long lines
* 7f6ecb5 [2353] style fix: folded a long line
* f6c776b [2353] Update comments
* bc4926d [2353] Test BoB.startup()
* fa3e24b [2353] Test BoB._socket_data() directly
* a11abf3 [2353] Indent comment
* 996d6ff [2353] Test BoB.start_cfgmgr() timeout checks
* 924c7a4 [2353] Restore the saved time.time function
* 8cb9e9e [2353] Don't check an upper bound on number of attempts
* f117a66 [2353] Test BoB.start_msgq() timeout checks
* 77c012c [2353] Test BoB.start_ccsession()
* d514212 [2353] Fix an error in comments
* a9ebc37 [2353] Test options for BoB.start_cfgmgr()
* 1ab0c5a [2353] Test defaults for BoB.start_cfgmgr()
* 15249e5 [2353] Test BoB.start_msgq()
* da089cd [2353] Remove obsolete comments
* 5450b25 [2353] Use a MockProcessInfo to test BoB.start_process()
* b1c8e1c [2353] Test various combinations of booleans in BoB.reap_children()
* 58f0aac [2353] Test unknown pids in BoB.reap_children()
* 84fbdae [2353] Simply assertFalse() for the list
* 09f21f4 [2353] Test exception handling in BoB.reap_children()
* 36d2b84 [2353] Test port arg in BoB.start_cmdctl()
* 4f52e89 [2353] Test verbose case of BoB.start_cmdctl()
* c2c64ea [2353] Test verbose case of BoB.start_resolver()
* 230de14 [2353] Test verbose case of BoB.start_auth()
* 54a707d [2353] Remove the digit suffix from tests
* 51bd304 [2353] Rename MockBob2 to MockBobSimple
* da1b2d7 [2353] Rewrite test_start_simple() to use MockBob2
* d5c0c97 [2353] Test verbose case of BoB.start_simple()
* a7b31b7 [2353] runnable doesn't have to be set here
* 7503458 [2353] Directly use bob.components instead of bob.get_processes()
* 37da25f [2353] Just check the output of get_processes() is empty (as a boolean)
* 0f228f2 [2353] Check that kill_started_components() forcefully kills components
* 8b31b93 [2353] runnable doesn't have to be set here
* f9445ff [2353] runnable doesn't have to be set here
* 494bbca [2353] Ensure that get_processes() returns a sorted list
* 67f4798 [2353] Check remove_socket_srv() with different states of BoB._srv_socket
* 32e8885 [2353] Ensure that the server listens on the socket
* 9d23c50 [2353] Check that getsockname() on the socket returns what we expect
* 83d4f41 [2353] Test that the socket and tmpdir are deleted
* 424b8d8 [2353] Test bind10_src.fatal_signal()
* 3590bad [2353] Test bind10_src.get_signame()
* 4a9b794 [2353] Test BoB.init_socket_srv() and BoB.remove_socket_srv()
* 180a437 [2353] Test more component startup methods
* 6210dae [2353] Add test for BoB.start_simple()
* 6b8c62a [2353] Add test for BoB.register_process()
* b0db66c [2353] Add test for BoB.start_process()
* 7173928 [2353] Add test for BoB.kill_started_components()
* 7ba306a [2353] Add test for BoB.get_processes()
* 9b6c74f [2353] Indent code
* 356a8b2 [2353] Test BoB.reap_children

Also pushed to master branch post-merge to fix for new code in master (reviewed by Jinmei on Jabber):

* 230ccce (trac2353) [master] Clear components dict as documented in BoB.kill_started_components()

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.