Opened 4 months ago

Closed 4 months ago

#30893 closed defect (fixed)

Refactor and improve test coverage in confparse.c

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: dgoulet-merge
Cc: Actual Points: 1
Parent ID: #29211 Points: 1
Reviewer: catalyst Sponsor: Sponsor31-can

Description

confparse.c is at around 81% coverage, which is not too bad, but since we're refactoring it, we should probably improve its coverage.

As we do so, I'd like to remove unused configuration types, and rename the dubiously so called "UINT", which is really an int restricted to nonnegative values.

Child Tickets

Change History (12)

comment:1 Changed 4 months ago by nickm

Status: assignedneeds_review

See branch ticket30893 with PR at https://github.com/torproject/tor/pull/1113 . It gets us to 100% coverage on confparse.c.

comment:2 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

Whoops, memory leaks.

comment:3 Changed 4 months ago by nickm

Status: needs_revisionneeds_review

Force-pushed new branch to fix memory leaks.

One of the memory leaks was a real bug in Tor; see #30894. I have merged that branch into this one, so that we'll get the fix and CI will pass.

comment:4 Changed 4 months ago by dgoulet

Reviewer: catalyst

comment:5 in reply to:  1 Changed 4 months ago by catalyst

Status: needs_reviewneeds_information

Replying to nickm:

See branch ticket30893 with PR at https://github.com/torproject/tor/pull/1113 . It gets us to 100% coverage on confparse.c.

Thanks! Overall, this looks good.

https://coveralls.io/builds/24017667 shows confparse.c coverage as 97.64%. The uncovered lines seem to be error handling code. Did you have a previous Coveralls run on that branch that showed that file at 100% coverage?

I made some minor comments on the pull request.

comment:6 Changed 4 months ago by nickm

Hm, odd. I had gotten this result with gcov and the cov-exclude script, not from coveralls. I can add tests for those lines though.

I've answered the questions on the ticket. If it's okay with you, I'd like to fix the comment later on in the branch.

comment:7 Changed 4 months ago by nickm

Status: needs_informationneeds_review

comment:8 Changed 4 months ago by nickm

Ah, I see what the problem was with the coverage. I wrote those tests _after_ I had moved some functions while working on #30864, and then backported them. I'll write a few more tests for that code.

comment:9 in reply to:  8 Changed 4 months ago by nickm

Replying to nickm:

Ah, I see what the problem was with the coverage. I wrote those tests _after_ I had moved some functions while working on #30864, and then backported them. I'll write a few more tests for that code.

If it's okay with you, though, I'll also add those tests as part of #30864, since otherwise I'm going to run into conflicts with marking these functions STATIC.

comment:10 Changed 4 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks. I agree with your proposal to do the comment change and the coverage improvement in #30864. I would like it if you would update #30864 to describe these planned changes as carried over from this ticket, though.

comment:11 Changed 4 months ago by nickm

Keywords: dgoulet-merge added

comment:12 Changed 4 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.