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/go, testing: TB.Failed does not report race failures #19851

Closed
tamird opened this issue Apr 5, 2017 · 5 comments

Comments

Projects
None yet
6 participants
@tamird
Copy link
Contributor

commented Apr 5, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tamird/src/go"
GORACE=""
GOROOT="/Users/tamird/src/go1.8"
GOTOOLDIR="/Users/tamird/src/go1.8/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/lf/1_qqs3815tzgkhtrbrjs913m0000gn/T/go-build904117806=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

main_test.go:

package main

import "testing"

func TestRace(t *testing.T) {
	t.Run("", func(t *testing.T) {
		defer func() {
			if !t.Failed() {
				t.Errorf("subtest did not fail")
			}
		}()
		for i := 0; i < 10; i++ {
			c := make(chan int)
			x := 1
			go func() {
				x = 2
				c <- 1
			}()
			x = 3
			<-c
		}
	})
	if !t.Failed() {
		t.Errorf("test did not fail")
	}
}

What did you expect to see?

==================
WARNING: DATA RACE
Write at 0x00c420070df8 by goroutine 8:
  github.com/cockroachdb/cockroach/foo.TestRace.func1.2()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:16 +0x3b

Previous write at 0x00c420070df8 by goroutine 7:
  github.com/cockroachdb/cockroach/foo.TestRace.func1()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:19 +0xf6
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107

Goroutine 8 (running) created at:
  github.com/cockroachdb/cockroach/foo.TestRace.func1()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:18 +0xe5
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /Users/tamird/src/go1.8/src/testing/testing.go:697 +0x543
  github.com/cockroachdb/cockroach/foo.TestRace()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:22 +0x5a
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107
==================
--- FAIL: TestRace (0.00s)
    --- FAIL: TestRace/#00 (0.00s)
    	testing.go:610: race detected during execution of test
	testing.go:610: race detected during execution of test
FAIL
FAIL	github.com/cockroachdb/cockroach/foo	0.021s

What did you see instead?

==================
WARNING: DATA RACE
Write at 0x00c42006edf8 by goroutine 8:
  github.com/cockroachdb/cockroach/foo.TestRace.func1.2()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:16 +0x3b

Previous write at 0x00c42006edf8 by goroutine 7:
  github.com/cockroachdb/cockroach/foo.TestRace.func1()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:19 +0xf6
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107

Goroutine 8 (running) created at:
  github.com/cockroachdb/cockroach/foo.TestRace.func1()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:18 +0xe5
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /Users/tamird/src/go1.8/src/testing/testing.go:697 +0x543
  github.com/cockroachdb/cockroach/foo.TestRace()
      /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/foo/main_test.go:22 +0x5a
  testing.tRunner()
      /Users/tamird/src/go1.8/src/testing/testing.go:657 +0x107
==================
--- FAIL: TestRace (0.00s)
    --- FAIL: TestRace/#00 (0.00s)
    	main_test.go:9: subtest did not fail
    	testing.go:610: race detected during execution of test
	testing.go:610: race detected during execution of test
FAIL
FAIL	github.com/cockroachdb/cockroach/foo	0.015s

Note the presence of main_test.go:9: subtest did not fail. We're running into this issue in CockroachDB, where we have logging redirection taking place for the duration of the test; if the test is not observed to have failed at the end of the run, the logs are cleaned up. As the snippet above demonstrates, it's not currently possible for a test to observe its own race-related failure, so we end up losing our logs. The CockroachDB issue is cockroachdb/cockroach#14563.

Related to #15972.

cc @dvyukov @rsc

@bradfitz bradfitz added this to the Unplanned milestone Apr 5, 2017

@dvyukov dvyukov added the RaceDetector label Apr 6, 2017

@dvyukov

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

Probably need to make t.Failed look at race.Errors().

@josharian josharian added the Suggested label Apr 6, 2017

@cespare cespare self-assigned this Apr 7, 2017

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

I have a WIP CL. I think the fix is easy but writing a test requires a certain amount of cleverness I'm not sure I possess. I'll take another look later (but testing ideas on the CL are appreciated).

@gopherbot

This comment has been minimized.

Copy link

commented Apr 8, 2017

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

@gopherbot gopherbot closed this in 221541e Apr 10, 2017

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

testing: consider a test failed after race errors
Fixes golang#19851.

Change-Id: I5ee9533406542be7d5418df154f6134139e75892
Reviewed-on: https://go-review.googlesource.com/39890
Run-TryBot: Caleb Spare <cespare@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Aug 8, 2017

Change https://golang.org/cl/54050 mentions this issue: testing: don't fail all tests after racy test failure

gopherbot pushed a commit that referenced this issue Aug 15, 2017

testing: don't fail all tests after racy test failure
The code was adding race.Errors to t.raceErrors before checking
Failed, but Failed was using t.raceErrors+race.Errors. We don't want
to change Failed, since that would affect tests themselves, so modify
the harness to not unnecessarily change t.raceErrors.

Updates #19851
Fixes #21338
Change-Id: I7bfdf281f90e045146c92444f1370d55c45221d4
Reviewed-on: https://go-review.googlesource.com/54050
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Aug 18, 2017

Change https://golang.org/cl/57151 mentions this issue: [release-branch.go1.9] testing: don't fail all tests after racy test failure

gopherbot pushed a commit that referenced this issue Aug 18, 2017

[release-branch.go1.9] testing: don't fail all tests after racy test …
…failure

The code was adding race.Errors to t.raceErrors before checking
Failed, but Failed was using t.raceErrors+race.Errors. We don't want
to change Failed, since that would affect tests themselves, so modify
the harness to not unnecessarily change t.raceErrors.

Updates #19851
Fixes #21338

Change-Id: I483f27c68c340928f1cbdef160abc0a5716efb5d
Reviewed-on: https://go-review.googlesource.com/57151
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Aug 18, 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.