Opened 4 years ago

Closed 3 years ago

#16792 closed enhancement (implemented)

Have a way to mark lines as "unreachable by unit tests"

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, 028-triaged, TorCoreTeam201604, tor-tests-coverage, tor-tests-unit
Cc: Actual Points: small
Parent ID: Points: small
Reviewer: isis Sponsor: SponsorS-can

Description

It would be great to have a way to tell the test coverage code "this line won't be reached by tests, so don't worry about it." This would let us have a prayer of reaching 100%.

Child Tickets

Change History (21)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-final
Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 years ago by nickm

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

comment:3 Changed 4 years ago by tdruiva

Hi,

We can use the lcov exclusion markers like: LCOV_EXCL_LINE, LCOV_EXCL_START, LCOV_EXCL_STOP,
LCOV_EXCL_BR_LINE, LCOV_EXCL_BR_START and, LCOV_EXCL_BR_STOP.

The usage will be something like this:
/* LCOV_EXCL_START */

if (!options->Tor2webMode) {

log_err(LD_CONFIG, "This copy of Tor was compiled to run in "

"'tor2web mode'. It can only be run with the Tor2webMode torrc "
"option enabled.");

return -1;

}

/* LCOV_EXCL_STOP */

What do you think if we use macros to have this markers only if the coverage is enable?

Something like:
switch(some_value){

case 1:

something
break;

case 0:

something else
break;

case -1:

TOR_COV_UNREACHABLE_BEGIN
unreachable code
TOR_COV_UNREACHABLE_ENF

}

comment:4 Changed 4 years ago by nickm

I don't see the point in using macros here; they don't seem any cleaner than LCOV stuff, right?

(I wonder if any other coverage tools have macros like this that they would expect us to use, and whether we're about to be littered with them...)

comment:5 Changed 4 years ago by rjunior

The only reason was to abstract from the tool we are using - since Tor source code names the existing macros as COVERAGE_ENABLED (and TOR_COVERAGE) rather than LCOV_ENABLED.

comment:6 Changed 4 years ago by tdruiva

I don't see the point in using macros here; they don't seem any cleaner than LCOV stuff, right?

Yeah, you're right.

Other point about these lcov exclude markers, we can use in a macro style instead of comment, like

#define LCOV_EXCL_START
#define LCOV_EXCL_STOP

LCOV_EXCL_START
code
.
.
.
LCOV_EXCL_STOP

(I wonder if any other coverage tools have macros like this that they would expect us to use, and whether we're about to be littered with them...)

I'll keep looking

Last edited 4 years ago by tdruiva (previous) (diff)

comment:7 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:8 Changed 4 years ago by nickm

Keywords: SponsorS removed
Sponsor: SponsorS

Bulk-replace SponsorS keyword with SponsorS sponsor field in Tor component.

comment:9 Changed 4 years ago by nickm

Points: small
Priority: normalmajor

comment:10 Changed 3 years ago by nickm

Priority: HighMedium

comment:11 Changed 3 years ago by nickm

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

These tickets, though owned by me, are not deliverables I can realistically deliver by the 0.2.8 freeze window.

comment:12 Changed 3 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:13 Changed 3 years ago by nickm

Priority: MediumHigh

comment:14 Changed 3 years ago by nickm

Keywords: TorCoreTeam201604 added

These are infrastructure that will help out other projects, and should go in early. Marking them for April.

comment:15 Changed 3 years ago by nickm

Severity: Normal

I think that the approach from comment:6 above is the best we're going to be able to do. We'll need to teach our own coverage-parsers about them, though.

comment:16 Changed 3 years ago by nickm

Actual Points: small
Status: acceptedneeds_review

Actually, comments are more readable here.

Added an lcov_excl branch to handle the tooling, and to mark some lines in crypto.c

comment:17 Changed 3 years ago by nickm

Note: my approach to when we should use these LCOV_EXCL lines differs from the approach taken in tortls.c and config.c so far. I believe that we should only mark a line as excluded when it is truly unreachable. The annotations in tortls.c think that "maybe unreachable" can be a good enough reason.

comment:18 Changed 3 years ago by nickm

Keywords: tor-tests-coverage tor-tests-unit added

comment:19 Changed 3 years ago by isis

Reviewer: isis

comment:20 Changed 3 years ago by isis

Status: needs_reviewmerge_ready

LGTM. Tiny complaint that the scripts/test/cov-exclude script is Perl, but there's already Perl things floating around in that directory so I guess I shouldn't complain.

comment:21 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Thanks for the review; merged to master!

Note: See TracTickets for help on using tickets.