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.

Sign up for GitHub

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

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

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

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

btracey opened this issue Jun 27, 2017 · 7 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.9

Comments

@btracey
Copy link
Contributor

btracey commented Jun 27, 2017 •
edited

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
Copy link
Contributor Author

btracey 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.

@btracey btracey mentioned this issue Jun 27, 2017
Closed
@bradfitz bradfitz added this to the Go1.9 milestone Jun 27, 2017
@bradfitz bradfitz added Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 27, 2017
@bradfitz
Copy link
Contributor

@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
Copy link
Contributor

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

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

@btracey
Copy link
Contributor Author

btracey commented Jun 27, 2017

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

@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 27, 2017
@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

Sending a CL.

@gopherbot
Copy link

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

@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.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
4 participants
@bradfitz @ianlancetaylor @btracey @gopherbot