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

cmd/compile: intermittent test failure on gonum suite with testing/quick #20809

Closed
btracey opened this issue Jun 27, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@btracey
Copy link
Contributor

commented Jun 27, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.9beta2 (go version devel +eab99a8 Mon Jun 26 21:12:22 2017 +0000 darwin/amd64)

I confirmed this bug does not exist in go1.8.3

What operating system and processor architecture are you using (go env)?

brendan:~$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/brendan/Documents/othergo"
GORACE=""
GOROOT="/Users/brendan/gover/go"
GOTOOLDIR="/Users/brendan/gover/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l6/mhj4qjrj4437b_lgfz9pm1rw0000gn/T/go-build536430216=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

go get -u -t gonum.org/v1/gonum/blas/gonum
cd $GOPATH/src/gonum.org/v1/gonum/blas/gonum/internal/math32
go test ./...

Approximately 1 in every 5 runs, this returns

brendan:~/Documents/othergo/src/gonum.org/v1/gonum/blas/gonum/internal/math32$ go test
--- FAIL: TestHypot (0.00s)
	math_test.go:46: #91: failed on input struct { X float32; Y float32 }{X:2.214908e+38, Y:-1.091827e+38}
FAIL
exit status 1

I ran this ~20 times with go1.8.3, and never saw this test failure (plus, we've been running travis on all of our commits with go1.8.3 and have not seen this failure).

The function in question, math32.Hypot calls math32.Sqrt, which is implemented in assembly. This can be disabled with the "noasm" tag, and the failure still occurs.

brendan:~/Documents/othergo/src/gonum.org/v1/gonum/blas/gonum/internal/math32$ go test -tags=noasm
--- FAIL: TestHypot (0.00s)
	math_test.go:46: #16: failed on input struct { X float32; Y float32 }{X:-1.917559e+38, Y:-3.838481e+36}
FAIL
exit status 1

The release notes say there is a change to the int64 and uint64 random number generation, but as best as I can tell the source has not changed for float32 generation.

@btracey btracey changed the title cmd/compile: intermittent test failure on gonum suite cmd/compile: intermittent test failure on gonum suite with testing/quick Jun 27, 2017

@btracey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

Sorry, I should have ran this before submission. The test is flaky, for instance the specific test

func TestHypot2(t *testing.T) {
	var p float32 = -3.6557444e+37
	var q float32 = 7.9378095e+37
	y := Hypot(p, q)
	if !floats.EqualWithinRel(float64(y), math.Hypot(float64(p), float64(q)), tol) {
		t.Errorf("hypot mismatch")
	}
}

fails under both go1.8.3 and go1.9beta2.

I still don't understand why we never see the failure under go1.8.3 with testing quick. If I misread the code and the int64 generation does affect the int32, it would be nice to have that mentioned in the release notes.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

@btracey, perhaps the actual change affecting you is:

https://go-review.googlesource.com/c/39152/9/src/testing/quick/quick.go

at the bottom, note the:

@@ -193,7 +200,7 @@ var defaultConfig Config
 // getRand returns the *rand.Rand to use for a given Config.
 func (c *Config) getRand() *rand.Rand {
        if c.Rand == nil {
-               return rand.New(rand.NewSource(0))
+               return rand.New(rand.NewSource(time.Now().UnixNano()))
        }
        return c.Rand
 }

If you revert that part back to always using NewSource(0), do your tests pass again?

It's probably worth noting that change as well, assuming we want to keep it.

@rsc, @ianlancetaylor?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

From https://golang.org/cl/39152:

While we are here, also make the default source of randomness not completely deterministic.

@btracey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

Ah, missed that. Yes, reverting that change means the test does not fail.

@bradfitz bradfitz added NeedsFix and removed NeedsDecision labels Jun 27, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

Seems like a valid change to me, but, yes, it needs to be in the release notes.

@bradfitz bradfitz self-assigned this Jun 27, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Sending a CL.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 27, 2017

CL https://golang.org/cl/46910 mentions this issue.

@gopherbot gopherbot closed this in e25fdb9 Jun 27, 2017

@golang golang locked and limited conversation to collaborators Jun 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.