Opened 5 months ago

Closed 5 months ago

#25993 closed defect (fixed)

Improve deliberate test coverage for addressmap_get_virtual_address()

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, tor-tests-coverage, tor-tests-unit
Cc: Actual Points:
Parent ID: #25908 Points:
Reviewer: catalyst Sponsor: Sponsor3-can

Description

This function is currently tested indirectly from test_entryconn.c, but this results in unreliable coverage for a couple of the features in this function.

Specifically, we don't test whether duplicate addresses are rejected, or whether IPv4 addresses ending with .0 or .255 are rejected.

This is one source of nondeterminism in our test coverage.

Child Tickets

Change History (6)

comment:1 Changed 5 months ago by nickm

I have a branch ticket25993. I'm going to wait to see what coveralls says about it before I make a PR though.

comment:2 Changed 5 months ago by nickm

Status: assignedneeds_review

Yes, this does seem to cover the lines that were flapping on and off before, plus a few more lines as well. https://github.com/torproject/tor/pull/72 is a pull request.

comment:3 Changed 5 months ago by nickm

Sponsor: Sponsor3-can

comment:4 Changed 5 months ago by dgoulet

Reviewer: catalyst

comment:5 Changed 5 months ago by catalyst

Status: needs_reviewmerge_ready

Code change look good to me! I left a comment on GitHub with some suggestions about the wording of a comment.

I haven't fully confirmed that every new test does what it claims to do, but it looks likely that the coverage flapping for the .0 and .255 case is gone.

comment:6 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks for the review! I've added the comment you suggested in a new fixup commit on the branch, and then squashed it as ticket25993_squashed and merged the branch.

Note: See TracTickets for help on using tickets.