Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1825 closed defect (implemented)

Add unit tests for statistics code

Reported by: karsten Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: nickm, Sebastian, ln5 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We still lack unit tests for most of the statistics code in Tor. The recent changes in the course of allowing *Statistics options to be enabled or disabled while Tor is running are a first step in writing good unit tests. We should further refactor the statistics code, e.g., so that it doesn't depend on get_options() and separates generating statistics output from writing it to disk.

Branch stats-tests in my public repository has the refactorings and unit tests for exit port statistics.

I'd like to know if the refactorings are correct and if the tests make sense. If so, let's merge these, and I'll do the same for the other statistics code.

Child Tickets

Change History (6)

comment:1 Changed 9 years ago by karsten

Status: newneeds_review

comment:2 Changed 9 years ago by 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".

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.

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.

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.

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

comment:3 in reply to:  2 Changed 9 years ago by karsten

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

comment:4 Changed 9 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good to me... just a couple things.

  • It seems that there's still a few rep_hist_exit_stats_history calls in test.c that didn't get renamed.
  • There wasn't a changes file.
  • Apparently the way we handled cleaning up temporary directories with atexit() meant that when the child process exited, it would remove the temporary directory, thus making other tests in the main process fail.

I'm fixing up those and merging it. Thanks!

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.