Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net: TestLookupLocalPTR fails on Windows #46882

Closed
bcmills opened this issue Jun 23, 2021 · 4 comments
Closed

net: TestLookupLocalPTR fails on Windows #46882

bcmills opened this issue Jun 23, 2021 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 23, 2021 

PS C:\Users\bryan\src\go\src> hostname
zarlino
PS C:\Users\bryan\src\go\src> go version
go version devel go1.17-0ebd5a8de0 Tue Jun 22 16:59:10 2021 +0000 windows/amd64
PS C:\Users\bryan\src\go\src> go test net
…
--- FAIL: TestLookupLocalPTR (0.05s)
    lookup_windows_test.go:150: different results 2601:547:1302:1713:21d6:a925:d7a5:3198:exp:["zarlino."] got:["zarlino"]
FAIL
FAIL    net     61.050s
go env 
PS C:\Users\bryan\src\go\src> go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\bryan\AppData\Local\go-build
set GOENV=C:\Users\bryan\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\bryan\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\bryan\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\bryan\src\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Users\bryan\src\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=devel go1.17-0ebd5a8de0 Tue Jun 22 16:59:10 2021 +0000
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\bryan\src\go\src\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\bryan\AppData\Local\Temp\go-build84688908=/tmp/go-build -gno-record-gcc-switches

The test (added for #29600) is comparing net.LookupAddr to the output of the unexported lookupPTR function, which shells out to the built-in ping command.

The output from ping in this case lacks a trailing dot:

PS C:\Users\bryan\src\go\src> ping -n 1 -a localhost

Pinging zarlino [::1] with 32 bytes of data:
Reply from ::1: time<1ms

Ping statistics for ::1:
    Packets: Sent = 1, Received = 1, Lost = 0 (0% loss),
Approximate round trip times in milli-seconds:
    Minimum = 0ms, Maximum = 0ms, Average = 0ms

This looks to me like a bug in the test. The Windows lookupPTR helper function adds a trailing dot unconditionally:

go/src/net/lookup_windows_test.go

Line 302 in 2ebe77a

ptr = append(ptr, ans[1]+".")

However, the real absDomainName helper function does not:

go/src/net/dnsclient.go

Lines 147 to 149 in 2ebe77a

if hasDots && b[len(b)-1] != '.' {
b = append(b, '.')
}

This behavior has been tweaked many times (see #12189, #12193, #12240, #13564), so it seems problematic for the A/B test not to apply the same transformation function to both names.

CC @alexbrainman @tdabasinskas @neild @ianlancetaylor
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure. labels Jun 23, 2021
@bcmills bcmills added this to the Backlog milestone Jun 23, 2021
@gopherbot
Copy link

Change https://golang.org/cl/330249 mentions this issue: net: use absDomainName in the Windows lookupPTR test helper

@alexbrainman
Copy link
Member

@bcmills you did not say how you discovered the failure.

I don't see any of our builders fail with this issue.

On the other hand, the long test fails on my PC:

c:\Users\Alex\dev\go\src\net>go test -short
PASS
ok      net     12.545s

c:\Users\Alex\dev\go\src\net>go test
--- FAIL: TestDialParallel (13.32s)
    dial_test.go:188: dialClosedPort: measured delay 2.0668458s
    dial_test.go:203: got 2.0668458s; want <= 1.5s
--- FAIL: TestLookupLocalPTR (0.04s)
    lookup_windows_test.go:150: different results 192.168.1.24: exp:["sharik."] got:["sharik"]
FAIL
exit status 1
FAIL    net     65.390s

c:\Users\Alex\dev\go\src\net>

Can we make builders run net.TestLookupLocalPTR somehow?

Thank you.

Alex

@bcmills
Copy link
Contributor Author

bcmills commented Jun 23, 2021 •
edited

you did not say how you discovered the failure.

I ran go test std on a personal Windows device.

I don't see any of our builders fail with this issue.
…
Can we make builders run net.TestLookupLocalPTR somehow?

I think the test is already running on the builders — it just doesn't reproduce the failures there because the behavior of the test depends on the machine's fully-qualified hostname.

I don't think it is viable to set up builders with specific hostnames just to exercise the net tests. (Ideally, the tests should be written so that they are not sensitive to the local hostname, but I'm not sure exactly how to do that in this case.)

@alexbrainman
Copy link
Member

I think the test is already running on the builders — it just doesn't reproduce the failures there because the behavior of the test depends on the machine's fully-qualified hostname.

Thanks for explaining.

I don't think it is viable to set up builders with specific hostnames just to exercise the net tests.

Agree.

but I'm not sure exactly how to do that in this case.

Same.

Hopefully CL 330249 will make this test break less.

Alex

@golang golang locked and limited conversation to collaborators Jun 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@bcmills @gopherbot @alexbrainman