Opened 3 months ago

Closed 5 weeks ago

#25787 closed defect (fixed)

geoip_load_file() tests don't work on all window build environments

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-must regression tests win32
Cc: juga Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

These tests assume that when the test binary is run, it can see the contents of the builddir at the same location as the original build process. But that isn't always true for windows builds.

Child Tickets

Change History (8)

comment:1 Changed 3 months ago by nickm

I've added a temporary fix as 8eb4a32a4dc4e9ecccb162a70b5212a9a2d3f80c , but we should come up with a better one.

comment:2 Changed 7 weeks ago by nickm

Keywords: 034-must added; 043-must removed

comment:3 Changed 7 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 6 weeks ago by nickm

Cc: juga added
Status: acceptedneeds_review

Please see branch bug25787 with PR at https://github.com/torproject/tor/pull/143 .

The fix here IMO is to just cut the Gordian Knot and generate the files we need ourself rather than trying at build-time to generate paths that Windows tests at run-time will accept.

cc'ing juga because juga has seen these tests since the beginning, and ought to see it when we admit failure :)

comment:5 Changed 5 weeks ago by dgoulet

Status: needs_reviewneeds_revision

clang (from travis) doesn't seem very happy with the PR :S ... https://travis-ci.org/torproject/tor/jobs/390995401

src/test/test_geoip.c:386:12: error: no previous extern declaration for
      non-static variable 'GEOIP_CONTENT'
      [-Werror,-Wmissing-variable-declarations]
const char GEOIP_CONTENT[] =

comment:6 Changed 5 weeks ago by nickm

Status: needs_revisionneeds_review

I think I fixed that with the fixup commit, though?

comment:7 Changed 5 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

lgtm;

comment:8 Changed 5 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to 0.3.4 and forward!

Note: See TracTickets for help on using tickets.