Re: [tor-bugs] #1825 [Tor Relay]: Add unit tests for statistics code

["Tor Bug Tracker & Wiki" <torproject-admin@xxxxxxxxxxxxxx>]

#1825: Add unit tests for statistics code
-----------------------+----------------------------------------------------
 Reporter:  karsten    |       Owner:              
     Type:  defect     |      Status:  needs_review
 Priority:  normal     |   Milestone:              
Component:  Tor Relay  |     Version:              
 Keywords:             |      Parent:              
-----------------------+----------------------------------------------------

Comment(by karsten):

 Replying to [comment:2 nickm]:
 > The function name "rep_hist_exit_stats_history" is has no verb; it
 should probably be rep_hist_get_exit_stats_history() or
 rep_hist_format_exit_stats_history() or something.  (Come to think of it,
 I think "history" is redundant there; the "hist" means "history".

 Renamed to rep_hist_format_exit_stats().

 > The asnprintf-and-join code makes me slightly concerned about
 fragmentation, but I hope it won't be a big problem, given how
 infrequently this function is called.

 We could count an upper bound of ports to write when going through all
 ports the first time and malloc a large enough string. Should I do that?
 Or is there a better approach? (This is something we could delay until
 0.2.3.x and backport to 0.2.2.x when we're sure it works. Or not
 backport.)

 > It looks like you're leaking at least written_string, read_string, and
 streams_string in rep_hist_exit_stats_history().  Before I go hunting for
 more leaks, it might be a good idea to run the new unit test under
 valgrind or using dmalloc.

 Good catch! I ran valgrind and didn't find any more leaks after fixing
 those three. I also tried to run with dmalloc (see #1832), but I'm not
 sure how another leak would look like in the dmalloc.log.

 > Given that the test_stats test touches global state, it might be a good
 idea to run it in a subprocess.  An entry like this would make it happen:
 >     { "stats", legacy_test_helper, TT_FORK, &legacy_setup, test_stats }
 > or define a new ENT-like macro, or make ENT take a flags argument to say
 what the flags are.  I can do this if you'd rather not.

 I defined a new ENT-like macro FORK to do this.

 > Since this is done and makes stuff more tested, we may as well merge it
 into 0.2.2.x, but it seems like a near thing. Arguably, we should hold off
 merging non-bugfixes till 0.2.3.x.  We can call "not being tested enough"
 a bug, I suppose. ;)

 My preference is merging this into 0.2.2.x, even if that means we'll have
 to hurry up writing unit tests for the other stats code. If we merged into
 0.2.3.x, we might not find bugs in 0.2.2.x, because we fixed them
 unnoticed when making the code easier to test.

 See updated branch stats-tests in my public repository.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1825#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs