[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #25425 [Core Tor/Tor]: Add unittests for bridges.c module
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs