Re: [tor-bugs] #17020 [Tor]: Add test for Config options_act function

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#17020: Add test for Config options_act function
-----------------------------+--------------------------------
     Reporter:  tcz001       |      Owner:
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:  Tor: 0.2.8.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  testing
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+--------------------------------

Comment (by tcz001):

 '''Thanks for feedbacks'''

 '''Code'''

  * I think we want all our unit tests to succeed in tor2web mode. (I might
 br wrong, do we run unit tests in tor2web mode?) Can you make running
 test_config_options_act_Tor2webMode_err conditional onÂ`#ifndef
 ENABLE_TOR2WEB_MODE`?

 Not quite understand this, does it mean we need to set every test case
 with --enable-tor2web-mode?


  * Should the Tor2webRendezvousPoints part of
 test_config_options_act_circuit_change_by_Nodes_update be conditional on
 `#ifdef ENABLE_TOR2WEB_MODE`? Or does it work regardless?

 Yeah agree, this should be only inÂENABLE_TOR2WEB_MODE, but should it also
 have aÂ#ifdef ENABLE_TOR2WEB_MODEfor whereÂTor2webRendezvousPoints exists?

 '''Coverage / lcov'''

  * I'm not sure if we've using the LCOV_EXCL_ directives elsewhere in the
 tor codebase. We have tended to avoid tool-secific directives unless there
 are compelling reasons.

 LCOV_EXCL is related to !#16792, if we don't really enforce the coverage,
 I think it's fine to remove it.

 '''Splitting into commits'''

  * In future, it will be easier for us to review large branches if they
 are split into different commits. For example, this branch could have been
 spit into:
    * MOCK_* and other declaration changes
    * STATIC/static changes
    * LCOV_EXCL_* changes
    * Whitespace-only or comment-only changes
    * Tor code changes (such as geoip.c lines 1210-1213 - are there any
 others?)
    * Test code changes

 Totally agree. Will do this in future patches.

 '''Nitpicks'''

  * There are some unnecessary whitespace changes in this branch, such as
 config.c lines 1829-1834, and some of the changes in connection_or.h.

 It seems my editor's fault, will double check these problems.

 > I see the following warning when running the tests on OS X and Linux:
 > "config/options_act_inform_testing_reachability: [forking] [warn]
 event_add: event has no event_base set.
 > OK"

 Ah this is not good, I'm looking into this now.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17020#comment:7>
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