#25425 closed enhancement (implemented)

Add unittests for bridges.c module

Reported by: isis Owned by: isis
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-unittests, tor-bridge, review-group-35
Cc: Actual Points:
Parent ID: Points: 2
Reviewer: ahf Sponsor: Sponsor8-can

Description

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 %

Child Tickets

Change History (18)

comment:1 Changed 22 months ago by isis

Milestone: Tor: 0.3.3.x-final

Are we allowed to put in merge requests for unittests for the currently closed (but still stabilising) release (0.3.3)? I'm not sure. Feel free to bump into 0.3.4 if not. :)

comment:2 Changed 22 months ago by isis

Owner: set to isis
Status: newaccepted

comment:3 Changed 22 months ago by isis

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: acceptedneeds_review
Type: defectenhancement
Version: Tor: 0.3.3.3-alpha

Patch in my bug25425 branch. 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.TestDescriptorDownloader and turn into three zombie processes on my development machine for some reason that I didn't bother looking into, so I don't know what coverage-html-full will say.)

I built with --enable-fatal-warnings --enable-expensive-hardening and also ran it under valgrind to check for accidental leaks. Also TravisCI passes.

comment:4 Changed 22 months ago by isis

Keywords: review-group-35 added

Putting in review-group-35, since it's not priority and we can do this after Rome.

comment:5 Changed 21 months ago by dgoulet

Reviewer: ahf

Reviewer week 03/16th

comment:6 Changed 21 months ago by ahf

Status: needs_reviewneeds_revision

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.

comment:7 Changed 21 months ago by nickm

Keywords: 034-triage-20180328 added

comment:8 Changed 21 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:9 in reply to:  6 Changed 21 months ago by isis

Status: needs_revisionneeds_review

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

  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.

comment:10 Changed 21 months ago by ahf

Status: needs_reviewneeds_revision

I think this looks good!

I only have some minor thing related to the tt_finished() macro:

  1. The ; in the macro definition is not needed, since tt_finished(); will then get expanded to tt_finished();; (double ;).
  2. 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)

comment:11 Changed 21 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

comment:12 in reply to:  10 Changed 21 months ago by isis

Milestone: Tor: unspecifiedTor: 0.3.4.x-final

Replying to ahf:

I think this looks good!

I only have some minor thing related to the tt_finished() macro:

  1. The ; in the macro definition is not needed, since tt_finished(); will then get expanded to tt_finished();; (double ;).


Ah right, good catch.

  1. 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_squashed2 branch (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

comment:13 in reply to:  11 Changed 21 months ago by isis

Replying to nickm:

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.


What tag do I use to put this back in for the next triage?

comment:14 Changed 21 months ago by isis

Status: needs_revisionmerge_ready

comment:15 Changed 20 months ago by nickm

Keywords: 034-triage-20180328 034-removed-20180328 removed

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.

comment:16 Changed 20 months ago by nickm

Status: merge_readyneeds_revision

Looks okay to me; please add a changes file and I'll merge it?

comment:17 in reply to:  16 Changed 20 months ago by isis

Status: needs_revisionmerge_ready

Replying to nickm:

Looks okay to me; please add a changes file and I'll merge it?


Done!

comment:18 Changed 20 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.