[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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

#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 teor):

 Replying to [comment:7 tcz001]:
 >  * 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?

 I don't think so. That would mean we weren't running the unit tests under
 the standard mode (tor2web mode and standard mode are different options at
 compile time, you can't choose both).

 I don't know what we do with unit tests and tor2web mode at the moment. I
 need to ask nickm or virgil:
 * Do we just ignore tor2web mode when unit testing?
 * Or do we expect unit tests to pass in both modes?

 >  * 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?

 If the tests work in normal mode, please leave them in for now.

 > '''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.

 I'm happy to go with nickm's comment on this, please leave it in:
 "Actually I'm okay with the lcov business: there isn't a portable
 crosstool solution there yet afaik. If we find one, we can switch to it
 easily enough."

 > '''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.

 Some editors will show or automatically trim trailing whitespace.
 Once you've done that, run `make check-spaces` on your code. It will find
 some of the most important whitespace issues.

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

 Thanks.

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