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

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

#25425: Add unittests for bridges.c module
-------------------------------------------------+-------------------------
 Reporter:  isis                                 |          Owner:  isis
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 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                                       |
Parent ID:                                       |         Points:  2
 Reviewer:  ahf                                  |        Sponsor:
                                                 |  Sponsor8-can
-------------------------------------------------+-------------------------
Changes (by ahf):

 * status:  needs_review => needs_revision


Comment:

 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 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.
 - The `ADD_BRIDGE()` macro should probably get undefined after it's no
 longer needed.

 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?

 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.

 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.

 In `test_bridges_get_transport_by_bridge_addrport()`:
 - No need to check for `NULL` before freeing the `transport` variable.
 - Constness is added to some variables before they are passed to
 `get_transport_by_bridge_addrport()` - is that needed here?


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

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