Opened 5 years ago

Closed 5 years ago

#12207 closed defect (implemented)

Improve unittest coverage of entrynodes.c

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client tor-guard
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

entrynodes.c is not very well tested unfortunately. Only 2% of it is.

As part of the #11480 adventures, we should aim to write some more unit tests.

I'm planning to try to write at least unit tests for choose_random_entry_impl(), which is one of the stars of entrynodes.c.

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by asn

It would also be smart to test add_an_entry_guard() and entry_guard_register_connect_status() (although the latter might be a bit messy to test).

Also entry_guards_set_from_config() and entry_guards_parse_state() would be useful to test.

comment:2 Changed 5 years ago by asn

OK, just so that I don't spend more time for something we don't like, I pushed a first version of my unittests in bug12207_first_draft of https://git.torproject.org/user/asn/tor.git.

I tried to be careful during refactoring, so I splitted it into multiple commits. Hope it's readable.

BTW, the unittests currently all fail because the loading of router descriptors fails because of OLD_ROUTER_DESC_MAX_AGE and:

  if (!in_consensus && from_cache &&
      router->cache_info.published_on < time(NULL) - OLD_ROUTER_DESC_MAX_AGE) {
    *msg = "Router descriptor was really old.";
    routerinfo_free(router);
    return ROUTER_WAS_NOT_NEW;
  }

in router_add_to_routerlist(). We should make that piece of code not apply during unittests.
I didn't find a nice way of doing this, but I still want to show you the code, so this is still TODO.
You can check that the unittests pass if you increase the value of OLD_ROUTER_DESC_MAX_AGE.

It's marked as draft because of the above issue, and also because I would like to reread the code before marking for merge. I also need to run make check-spaces on this.

FWIW, this seems to bring entrynodes.c coverage to:

File 'src/or/entrynodes.c'
Lines executed:33.63% of 1017
Creating 'entrynodes.c.gcov'

comment:3 Changed 5 years ago by asn

Status: newneeds_revision

Hm, I think I know a nicer way to solve the problem with that check. I can make that check a function (bool is_router_descriptor_too_old(router) or something), and then in the unittests I can mock it to be a NOP.

Also, it seems like I forgot a hardcoded local path I used while writing the unittests. I will fix that soon too.

comment:4 Changed 5 years ago by asn

Status: needs_revisionneeds_review

OK fixed both issues, and pushed bug12207_second_draft. Enjoy!

comment:5 Changed 5 years ago by asn

Milestone: Tor: 0.2.6.x-final

comment:6 Changed 5 years ago by asn

Status: needs_reviewneeds_revision

Note to self. Nick asked me to rename the router_descriptor_is_too_old() function to something else.

He suggested router_descriptor_is_older_than() with the maximum age as an argument.

comment:7 Changed 5 years ago by asn

Status: needs_revisionneeds_review

Pushed the above interface change to my bug12207_second_draft.

Ready for review again.

comment:8 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay, I don't see any more trouble here. I didn't read the tests themselves too closely, but they look reasonable, and I'm fairly confident that the entrynodes.c refactoring is correct. Merging this to master.

Note: See TracTickets for help on using tickets.