Opened 4 years ago

Last modified 21 months ago

#17020 needs_revision enhancement

Add test for Config options_act function

Reported by: tcz001 Owned by: tcz001
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-testing, SponsorS-deferred
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

Increase test coverage for Config.c function options_act.
All changes are in following branch:

https://github.com/twstrike/tor/commit/eedfd46cb24f874b5608ee0c6ae49cabe832f97f

Child Tickets

Change History (29)

comment:1 Changed 4 years ago by tcz001

Status: newneeds_review

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

Whoa, big patch! We'd better review it!

comment:3 Changed 4 years ago by teor

Thank you for this branch. We are trying to increase our test coverage at the moment.

Please find my comments from a read-through of the code below:

Code

  • I think we want all our unit tests to succeed in tor2web mode. (I might br wrong, do we run unit tests in tor2web mode?) Can you make running test_config_options_act_Tor2webMode_err conditional on #ifndef ENABLE_TOR2WEB_MODE?
  • Should the Tor2webRendezvousPoints part of test_config_options_act_circuit_change_by_Nodes_update be conditional on #ifdef ENABLE_TOR2WEB_MODE? Or does it work regardless?

Coverage / lcov

  • I'm not sure if we've using the LCOV_EXCL_ directives elsewhere in the tor codebase. We have tended to avoid tool-secific directives unless there are compelling reasons.

Splitting into commits

  • In future, it will be easier for us to review large branches if they are split into different commits. For example, this branch could have been spit into:
    • MOCK_* and other declaration changes
    • STATIC/static changes
    • LCOV_EXCL_* changes
    • Whitespace-only or comment-only changes
    • Tor code changes (such as geoip.c lines 1210-1213 - are there any others?)
    • Test code changes

Nitpicks

  • There are some unnecessary whitespace changes in this branch, such as config.c lines 1829-1834, and some of the changes in connection_or.h.

comment:4 Changed 4 years ago by teor

I haven't run this on either my Linux or OS X boxes yet.
Please remind me in a week if I haven't done so.

comment:5 Changed 4 years ago by nickm_mobile

Actually I'm okay with the lcov business: there isn't a portable crosstool solution there yet afaik. If we find one, we can switch to it easily enough.

comment:6 Changed 4 years ago by teor

I see the following warning when running the tests on OS X and Linux:
"config/options_act_inform_testing_reachability: [forking] [warn] event_add: event has no event_base set.
OK"

Can you please fix it?

comment:7 Changed 4 years ago by tcz001

Thanks for feedbacks

Code

  • I think we want all our unit tests to succeed in tor2web mode. (I might br wrong, do we run unit tests in tor2web mode?) Can you make running test_config_options_act_Tor2webMode_err conditional on #ifndef ENABLE_TOR2WEB_MODE?

Not quite understand this, does it mean we need to set every test case with --enable-tor2web-mode?


  • Should the Tor2webRendezvousPoints part of test_config_options_act_circuit_change_by_Nodes_update be conditional on #ifdef ENABLE_TOR2WEB_MODE? Or does it work regardless?

Yeah agree, this should be only in ENABLE_TOR2WEB_MODE, but should it also have a #ifdef ENABLE_TOR2WEB_MODEfor where Tor2webRendezvousPoints exists?

Coverage / lcov

  • I'm not sure if we've using the LCOV_EXCL_ directives elsewhere in the tor codebase. We have tended to avoid tool-secific directives unless there are compelling reasons.

LCOV_EXCL is related to #16792, if we don't really enforce the coverage, I think it's fine to remove it.

Splitting into commits

  • In future, it will be easier for us to review large branches if they are split into different commits. For example, this branch could have been spit into:
    • MOCK_* and other declaration changes
    • STATIC/static changes
    • LCOV_EXCL_* changes
    • Whitespace-only or comment-only changes
    • Tor code changes (such as geoip.c lines 1210-1213 - are there any others?)
    • Test code changes

Totally agree. Will do this in future patches.

Nitpicks

  • There are some unnecessary whitespace changes in this branch, such as config.c lines 1829-1834, and some of the changes in connection_or.h.

It seems my editor's fault, will double check these problems.

I see the following warning when running the tests on OS X and Linux:
"config/options_act_inform_testing_reachability: [forking] [warn] event_add: event has no event_base set.
OK"

Ah this is not good, I'm looking into this now.

comment:8 in reply to:  7 Changed 4 years ago by teor

Replying to tcz001:

  • I think we want all our unit tests to succeed in tor2web mode. (I might br wrong, do we run unit tests in tor2web mode?) Can you make running test_config_options_act_Tor2webMode_err conditional on #ifndef ENABLE_TOR2WEB_MODE?

Not quite understand this, does it mean we need to set every test case with --enable-tor2web-mode?

I don't think so. That would mean we weren't running the unit tests under the standard mode (tor2web mode and standard mode are different options at compile time, you can't choose both).

I don't know what we do with unit tests and tor2web mode at the moment. I need to ask nickm or virgil:

  • Do we just ignore tor2web mode when unit testing?
  • Or do we expect unit tests to pass in both modes?


  • Should the Tor2webRendezvousPoints part of test_config_options_act_circuit_change_by_Nodes_update be conditional on #ifdef ENABLE_TOR2WEB_MODE? Or does it work regardless?

Yeah agree, this should be only in ENABLE_TOR2WEB_MODE, but should it also have a #ifdef ENABLE_TOR2WEB_MODEfor where Tor2webRendezvousPoints exists?

If the tests work in normal mode, please leave them in for now.

Coverage / lcov

  • I'm not sure if we've using the LCOV_EXCL_ directives elsewhere in the tor codebase. We have tended to avoid tool-secific directives unless there are compelling reasons.

LCOV_EXCL is related to #16792, if we don't really enforce the coverage, I think it's fine to remove it.

I'm happy to go with nickm's comment on this, please leave it in: "Actually I'm okay with the lcov business: there isn't a portable crosstool solution there yet afaik. If we find one, we can switch to it easily enough."

Nitpicks

  • There are some unnecessary whitespace changes in this branch, such as config.c lines 1829-1834, and some of the changes in connection_or.h.

It seems my editor's fault, will double check these problems.

Some editors will show or automatically trim trailing whitespace.
Once you've done that, run make check-spaces on your code. It will find some of the most important whitespace issues.

I see the following warning when running the tests on OS X and Linux:
"config/options_act_inform_testing_reachability: [forking] [warn] event_add: event has no event_base set.
OK"

Ah this is not good, I'm looking into this now.

Thanks.

comment:9 Changed 4 years ago by tcz001

I have fixed the warning and whitespace and splited commits up into (did a force overwrite on git, sorry for it)

  • MOCK_* and other declaration changes
  • STATIC/static changes
  • LCOV_EXCL_* changes
  • test cases

Please check following link:

https://github.com/twstrike/tor/commits/test_config_options_act

comment:10 in reply to:  9 Changed 4 years ago by teor

Keywords: TorCoreTeam201509 added

Replying to tcz001:

I have fixed the warning and whitespace and splited commits up into (did a force overwrite on git, sorry for it)

In future, please feel free to create new branches. Branches are cheap in git and easy to merge. And overwriting a branch breaks everything based on that branch.

I'll have a look at this next week, this week is full of the 027 release.

comment:11 Changed 3 years ago by nickm

Keywords: 028-triaged added

comment:12 Changed 3 years ago by nickm

Sponsor: SponsorS

comment:13 Changed 3 years ago by nickm

Points: small

comment:14 Changed 3 years ago by nickm

Severity: Normal
Status: needs_reviewneeds_revision

Quick notes:

  • it doesn't build when Tor is configured with --enable-gcc-warnings.
  • Try running it under valgrind (see instructions in doc/HACKING/HelpfulTools.txt) to see whether there are memory leaks.

comment:15 Changed 3 years ago by nickm

Keywords: pre028-patch added

comment:17 Changed 3 years ago by nickm

Owner: set to tcz001
Status: needs_revisionassigned

Setting tcz001 as the owner of this needs_revision ticket.

comment:18 Changed 3 years ago by nickm

Status: assignedneeds_revision

comment:19 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:20 Changed 3 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:21 Changed 3 years ago by nickm

Keywords: TorCoreTeam201509 removed

Removing TorCoreTeam201509 from these tickets, since we do not own a time machine.

comment:22 Changed 3 years ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:23 Changed 3 years ago by nickm

Keywords: SponsorS-deferred added
Sponsor: SponsorS-can

Remove the SponsorS status from these items, which we already decided to defer from 0.2.9. add the SponsorS-deferred tag instead in case we ever want to remember which ones these were.

comment:24 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:25 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:26 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:27 Changed 22 months ago by nickm

Keywords: 028-triaged removed

comment:28 Changed 22 months ago by nickm

Keywords: pre028-patch removed

comment:29 Changed 21 months ago by nickm

Keywords: tor-testing added; testing removed
Note: See TracTickets for help on using tickets.