Opened 9 months ago

Last modified 6 months ago

#25118 needs_revision defect

We need circuit launch and cannibalization unit tests

Reported by: dgoulet Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-circuit, tor-test, 034-triage-20180328, 034-removed-20180328
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In particular, these functions circuit_launch_by_extend_info() and circuit_find_to_cannibalize() really needs unit tests!

They do so much and they are so core that we need to assess what they are doing is ok. See #24469 for an example of what it should have done.

Child Tickets

Change History (5)

comment:1 Changed 7 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 7 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:3 Changed 6 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:4 Changed 6 months ago by ffmancera

Status: newneeds_review

I have been working on a test. I think I came with a good approach but the test is giving me an error. I don't know if the function tested is just not working properly or the test is wrong.

Please check my github branch bug25118! Thank you =)

comment:5 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

Ok so basically here is the problem:

May 04 09:31:55.761 [debug] circuit_launch_by_extend_info(): Haven't fetched enough directory info yet; canceling circuit launch.

A useful trick for you is to use ./src/test/test <your-test> --debug which will output the debug logs of the test. You can see that you don't get at all in the function but rather exit very fast with a NULL.

So couple of things:

  1. Use tt_assert(new_circ) to make sure you do have a circuit that you expected.
  1. router_have_minimum_dir_info() is mockable so you'll have to mock it to return true.

This circuit_launch_by_extend_info() is really not simple to unit test, you might need to mock lots of things. So instead, you could skip it and just unit tests circuit_find_to_cannibalize() which it self will be something intense as well.

Note: See TracTickets for help on using tickets.