[tor-bugs] #25425 [Core Tor/Tor]: Add unittests for bridges.c module

Tor Bug Tracker & Wiki blackhole at torproject.org
Fri Mar 30 00:21:26 UTC 2018


#25425: Add unittests for bridges.c module
-------------------------------------------------+-------------------------
 Reporter:  isis                                 |          Owner:  isis
     Type:  enhancement                          |         Status:
                                                 |  needs_review
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-unittests, tor-bridge, review-   |  Actual Points:
  group-35, 034-triage-20180328,                 |
  034-removed-20180328                           |
Parent ID:                                       |         Points:  2
 Reviewer:  ahf                                  |        Sponsor:
                                                 |  Sponsor8-can
-------------------------------------------------+-------------------------
Changes (by isis):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:6 ahf]:
 > Very nice with more tests!
 >
 > I have some tiny nitpicks where some of them goes for most of the code:
 >
 > - In `test_bridges.c`: I have no idea if we do personal copyright as
 well as TPI copyright. Someone else from the team probably knows I just
 haven't seen it before.

 I've gone ahead and just removed it in `7e3cfa3571`. I only feel somewhat
 strongly about it if I develop something new/impressive, and this is just
 unittests.

 > - I think we usually try to bind the * in pointer declarations to the
 symbol name (`foo_t *foo` instead of `foo_t* foo`). This is a minor
 nitpick, but might be worth going over.

 I've seen both and didn't really know which to use. (Normally, I think of
 it like "`foo_t*` is the type, so obviously part of the type shouldn't be
 attached to the variable name".) Fixed in `c6610daa53`.

 > - The `ADD_BRIDGE()` macro should probably get undefined after it's no
 longer needed.

 Oops, good catch, fixed in `cf928621f1`.

 > In `test_bridges_helper_func_add_bridges_to_bridgelist()` the
 `tt_int_op(0, OP_EQ, 0)` looks a bit odd - if it's needed maybe add a
 comment for why? Maybe it should be some kind of macro like `tt_skip()`
 but where the test isn't marked as skipped?

 It seems to complain (when using `--enable-fatal-warning`) with:
 {{{
 _test-test_bridges.Tpo -c -o src/test/src_test_test-test_bridges.o
 ../tor/src/test/test_bridges.c
   AR       src/or/libtor.a
   AR       src/or/libtor-testing.a
 ../tor/src/test/test_bridges.c: In function
 ‘test_bridges_helper_func_add_bridges_to_bridgelist’:
 ../tor/src/test/test_bridges.c:111:2: error: label ‘done’ defined but not
 used [-Werror=unused-label]
   done:
   ^
 cc1: all warnings being treated as errors
 }}}

 I've made a `tt_finished()` macro in `b0538e2017`.

 > In `test_bridges_get_configured_bridge_by_orports_digest()` do you have
 to remove constness from the return value of `bridge_list_get()`? As far
 as I can tell it's only used via `smartlist_get()` which accepts a `const
 smartlist_t *`. You also don't have to check for a possible `NULL` value
 of the `orports` variable since the `FREE_AND_NULL()` macro handles those
 gracefully.

 Ah, yeah, I don't know why I did that. And
 [https://trac.torproject.org/projects/tor/ticket/25605#comment:6 Nick has
 also told me] to stop doing the `if(x) free(x)` thing, so I've gone
 through and fixed these things in `52b36cec89`.

 > In
 `test_bridges_get_configured_bridge_by_addr_port_digest_digest_only()`,
 `test_bridges_get_configured_bridge_by_addr_port_digest_address_only()`,
 `test_bridges_get_configured_bridge_by_exact_addr_port_digest_donly()`,
 `test_bridges_get_configured_bridge_by_exact_addr_port_digest_both()`,
 `test_bridges_get_configured_bridge_by_exact_addr_port_digest_aonly()`,
 `test_bridges_get_transport_by_bridge_addrport_no_ptlist()`, and
 `test_bridges_get_transport_by_bridge_addrport()`: No need to check for
 `NULL` before freeing the `addr` variable.

 Also fixed in `52b36cec89`! :)

 > In `test_bridges_get_transport_by_bridge_addrport()`:
 > - No need to check for `NULL` before freeing the `transport` variable.

 Fixed in `52b36cec89` as well.

 > - Constness is added to some variables before they are passed to
 `get_transport_by_bridge_addrport()` - is that needed here?

 Yes, I believe so? Is there a cleaner/better way to make a `const
 transport_t**` and then use it later as a `transport_t*` than this?

 {{{#!C
   transport_t *transport = NULL;
   // ...
   ret = get_transport_by_bridge_addrport((const tor_addr_t*)addr, port,
                                          (const transport_t**)&transport);
   tt_ptr_op(transport, OP_EQ, NULL);
 }}}

 > These were the only things that caught my attention when going over it.

 Thanks! I've added all the commits as `fixup!` commits to the same branch,
 so if you're okay with those, there's a `bug25425_squashed` branch that
 can be merged.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25425#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online


More information about the tor-bugs mailing list