BIND 10 #2689: Investigate work needed for removing threads from stats tests

[BIND 10 Development]

#2689: Investigate work needed for removing threads from stats tests
-------------------------------------+-------------------------------------
            Reporter:  jelte         |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  statistics    |  accepted
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130305
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Please review trac2689.

 I've spent some amount of time to understand how the stats tests
 (and stats module itself) work and tried to eliminate the use of
 threads from some of the test cases with keeping the original test
 semantics as much as possible (if not completely).  The conversion
 hasn't been completed yet, but I think this is enough for a size-5
 task (and the diff is getting larger, so a complete fix would be
 painful for the reviewer).  I propose deferring the rest of the task
 to a separate ticket with a separate estimation.

 As far as I can see so far, threads are used mainly if not only
 because of the network I/O via `ModuleCCSession`.  It should generally
 be avoidable by faking ModuleCCSession` and stealing/modifying the
 messages sent/received over it.  I implemented this idea in a new
 mock class named `test_utils.SimpleStats` (as I covered more tests
 it's becoming not really "simple", but I'm intending to replace
 `MyStats` with the new one when conversion is completed anyway).  So I
 suggest reviewing this class first and then each converted test case.

 Another problem of the original tests are it was so indirect; they
 heavily rely on how the behavior of other modules running on different
 threads, even for testing one single method of the `Stats` class.
 Even without the pain due to threads, that's bad because indirect
 tests often lead to unreliable results or uncovered cases (and these
 are actually happening in the current tests).  So I tried to make them
 more reasonable "unit tests"; call the specific method explicitly
 (after some necessary setup) and check the result as closely as
 possible to that method.  Due to this some tests needed to be modified
 more substantially.  I believe the intent of the tests was generally
 kept, though.

 I also made some trivial cleanups to the main stats implementation,
 and there's one non-trivial bug fix:
 c8aea8a73e16028bb07d51caafe5bf8188d0ee14
 the converted test didn't pass without this change.  While it's not
 ideal to change both the test and the tested code at the same time,
 I believe this fix is correct and will help keep both simpler.

 Finally, I made one additional, not directly related change to
 stats-httpd test: 5adf1f3.  Due to this the stats-httpd test often
 failed for me due to the timeout of `socket.getfqdn('0.0.0.0')`
 (which would be reported as "(might be) dead lock").  I suspect it
 also happened on some buildbots, so it would be nice to fix it here.

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