Add unittests for bridges.c module
It didn't have any tests explicitly for it, and it currently has:
| make directive | line coverage | function coverage |
|---|---|---|
| coverage-html | 19.9% | 34.4% |
| coverage-html-full | 58.1% | 78.1 % |
Activity
changed milestone to %Tor: 0.3.4.x-final in legacy/trac
added component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.4.x-final in Legacy / Trac owner::isis in Legacy / Trac points::2 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac review-group-35 in Legacy / Trac reviewer::ahf in Legacy / Trac severity::normal in Legacy / Trac sponsor::8-can in Legacy / Trac status::closed in Legacy / Trac tor-bridge in Legacy / Trac tor-unittests in Legacy / Trac type::enhancement in Legacy / Trac labels
Patch in my
bug25425branch. It roughly doubles the test coverage of that module.make directive line coverage function coverage coverage-html 43.0% 59.4% coverage-html-full ??? ??? (The stem tests freeze upon
test.integ.descriptor.remote.TestDescriptorDownloaderand turn into three zombie processes on my development machine for some reason that I didn't bother looking into, so I don't know whatcoverage-html-fullwill say.)I built with
--enable-fatal-warnings --enable-expensive-hardeningand also ran it under valgrind to check for accidental leaks. Also TravisCI passes.Trac:
Type: defect to enhancement
Version: Tor: 0.3.3.3-alpha to N/A
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final
Status: accepted to needs_review
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 *fooinstead offoo_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()thett_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 likett_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 ofbridge_list_get()? As far as I can tell it's only used viasmartlist_get()which accepts aconst smartlist_t *. You also don't have to check for a possibleNULLvalue of theorportsvariable since theFREE_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(), andtest_bridges_get_transport_by_bridge_addrport(): No need to check forNULLbefore freeing theaddrvariable.In
test_bridges_get_transport_by_bridge_addrport():- No need to check for
NULLbefore freeing thetransportvariable. - 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.
Trac:
Status: needs_review to needs_revision- In
Replying to 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 *fooinstead offoo_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 inc6610daa53.- 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()thett_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 likett_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 errorsI've made a
tt_finished()macro inb0538e2017.In
test_bridges_get_configured_bridge_by_orports_digest()do you have to remove constness from the return value ofbridge_list_get()? As far as I can tell it's only used viasmartlist_get()which accepts aconst smartlist_t *. You also don't have to check for a possibleNULLvalue of theorportsvariable since theFREE_AND_NULL()macro handles those gracefully.Ah, yeah, I don't know why I did that. And Nick has also told me to stop doing the
if(x) free(x)thing, so I've gone through and fixed these things in52b36cec89.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(), andtest_bridges_get_transport_by_bridge_addrport(): No need to check forNULLbefore freeing theaddrvariable.Also fixed in
52b36cec89! :)In
test_bridges_get_transport_by_bridge_addrport():- No need to check for
NULLbefore freeing thetransportvariable.
Fixed in
52b36cec89as 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 atransport_t*than this?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 abug25425_squashedbranch that can be merged.Trac:
Status: needs_revision to needs_review- In
-
I think this looks good!
I only have some minor thing related to the
tt_finished()macro:- The
;in the macro definition is not needed, sincett_finished();will then get expanded tott_finished();;(double ;). - Maybe it would make more sense to define
tt_finished()as#define tt_finished() TT_EXIT_TEST_FUNCTION. I'm not sure here, but maybe Nick would have a good comment to add here since he knows the test subsystem very well.
Once 1 is fixed I think this should be marked as merge ready.
(I laughed at the comment in c6610daa531690fee637eaac53fd3073d5b00e69)
Trac:
Status: needs_review to needs_revision - The
Replying to ahf:
I think this looks good!
I only have some minor thing related to the
tt_finished()macro:- The
;in the macro definition is not needed, sincett_finished();will then get expanded tott_finished();;(double ;).
Ah right, good catch.
- Maybe it would make more sense to define
tt_finished()as#define tt_finished() TT_EXIT_TEST_FUNCTION. I'm not sure here, but maybe Nick would have a good comment to add here since he knows the test subsystem very well.
Once 1 is fixed I think this should be marked as merge ready.
Okay, I've fixed both of these in 0b8941b0b4, and I've re-squashed everything into a
bug25425_squashed2branch (Travis passes) which should be mergeable.(I laughed at the comment in c6610daa531690fee637eaac53fd3073d5b00e69)
:) lol, I think I would die if I saw whitespace like that
Trac:
Milestone: Tor: unspecified to Tor: 0.3.4.x-final- The
What tag do I use to put this back in for the next triage?
Whoops. If somebody has written the code, then yeah -- it can just go into the release milestone.
If you remove the 034-triaged keyword, it will at least get looked at again when next we triage.
Trac:
Keywords: 034-removed-20180328, 034-triage-20180328 deleted, N/A addedmoved from legacy/trac#25425 (moved)
added Feature label and removed 1 deleted label
added Bridge label and removed 1 deleted label
