Opened 4 years ago

Closed 3 years ago

#16801 closed enhancement (implemented)

Increase torgzip coverage as high as possible

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, 028-triage, tor-tests-coverage, tor-tests-unit, TorCoreTeam201606, review-group-3
Cc: Actual Points: .2
Parent ID: #17289 Points: 1
Reviewer: andrea Sponsor: SponsorS-can

Description

We've actually had bugs in this one that caused needless bandwidth wastage. (eg #11824 and #11787.) Let's see if there are any more!

x/torgzip.c.gcov 88 136 60.71

Child Tickets

Change History (19)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 4 years ago by nickm

Keywords: 028-triage added

comment:3 Changed 4 years ago by nickm

Keywords: SponsorS removed
Sponsor: SponsorS

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

comment:4 Changed 4 years ago by nickm

Points: small

comment:5 Changed 3 years ago by nickm

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

Throw most 0.2.8 "NEW" tickets into 0.2.9. I expect that many of them will subsequently get triaged out.

comment:6 Changed 3 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:7 Changed 3 years ago by nickm

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

comment:8 Changed 3 years ago by isabela

Points: small1

comment:9 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-final
Parent ID: #16791#17289

comment:10 Changed 3 years ago by nickm

Owner: set to nickm
Reviewer: nick
Severity: Normal
Status: newaccepted

I've made a start here with my branch zlib_coverage. It focuses on stuff that's not covered by unit or integration tests. It gets coverage from "make check" on torgzip.c up over 80%

comment:11 Changed 3 years ago by nickm

Actual Points: .2
Keywords: TorCoreTeam201606 added
Milestone: Tor: 0.2.9.x-final
Status: acceptedneeds_review

comment:12 Changed 3 years ago by cypherpunks

Commit cdb8f57a3138f6f9a59183094e89ee72df43d356

+#if defined ZLIB_VERNUM && ZLIB_VERNUM < 0x1200
+#define "We require zlib version 1.2 or later."
+#endif

Shouldn't this be #error? And if so, wouldn't it be better to test this when setting up the build environment (aka ./configure)?

-  if (is_gzip_supported()) {
+
+  if (1) {

Why not simply remove the if statement?

comment:13 Changed 3 years ago by nickm

Shouldn't this be #error?

yup, #error.

We could check it in configure (if somebody wants to write a patch) but personally I don't expect that there are enough zlib 1.1 users remaining for this to be worth much time.

Why not simply remove the if statement?

IMO best to do that as a separate patch, so that the diff doesn't get spuriously large.

comment:14 Changed 3 years ago by nickm

Keywords: review-group-3 added

Move some tickets into review-group-3: they are in 0.2.9, and they are needs_review.

comment:15 Changed 3 years ago by nickm

Reviewer: nick

comment:16 Changed 3 years ago by andrea

Reviewer: andrea

comment:17 Changed 3 years ago by nickm

If you're keen to try gitlab for reviewing this, I've made a merge request there as https://gitlab.com/nickm_tor/tor/merge_requests/2 .

comment:18 Changed 3 years ago by andrea

Status: needs_reviewneeds_revision

See comments on gitlab merge request

comment:19 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed
Type: defectenhancement

fixed and squashed and applied. Thanks!

Note: See TracTickets for help on using tickets.