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

testing: tests after a data race are marked as failed (1.9rc2) #21338

Closed
karalabe opened this issue Aug 8, 2017 · 13 comments

Comments

Projects
None yet
8 participants
@karalabe
Copy link
Contributor

commented Aug 8, 2017

Maybe I missed this behavior until now, still it seems a bug. Opposed to Go 1.8.3 (#21338 (comment)), running a racey test suite with Go 1.9rc2 results in all tests after the racey one being marked as failed:

package main

import (
	"testing"
)

func TestPreRace(t *testing.T) {
	// Nothing to do, must succeed
}

func TestRace(t *testing.T) {
	// Blow it up
	var n int

	wait := make(chan struct{})
	go func() {
		defer close(wait)
		n++
	}()

	n++
	<-wait
}

func TestPostRace(t *testing.T) {
	// Nothing to do, must succeed
}
$ go test -v --race

=== RUN   TestPreRace
--- PASS: TestPreRace (0.00s)
=== RUN   TestRace
==================
WARNING: DATA RACE
[...]
==================
--- FAIL: TestRace (1.00s)
	testing.go:700: race detected during execution of test
=== RUN   TestPostRace
--- FAIL: TestPostRace (0.00s)

Note, TestPostRace is marked as failed.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

Failing tests due to races is new in 1.9 – that it flags all following tests as failed might be a bug.

@karalabe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2017

@dominikh Tests always failed if a data race was detected.

@davecheney Go 1.8.3 only marks the racey test failed, not the others.

$ GOROOT=/tmp/go1.8.3 /tmp/go1.8.3/bin/go test -v --race

=== RUN   TestPreRace
--- PASS: TestPreRace (0.00s)
=== RUN   TestRace
==================
WARNING: DATA RACE
[...]
==================
--- FAIL: TestRace (0.00s)
	testing.go:610: race detected during execution of test
=== RUN   TestPostRace
--- PASS: TestPostRace (0.00s)
@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

@karalabe The commit I was thinking of is 221541e

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

This may be a regression /cc @ianlancetaylor @broady

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

I also saw this recently and was puzzled, but I assumed it was somehow on purpose to highlight the data race. If it was on purpose, this should be documented in the release notes. Either way, something to do for 1.9.

@mvdan mvdan added this to the Go1.9 milestone Aug 8, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

@cespare

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

Yeah, unintended consequence of 221541e.

We could probably just revert for 1.9, since #19851 doesn't seem too serious, and fix it properly later. But I'll check right now to see whether the real fix is trivial.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

I'm testing a fix.

@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 gopherbot closed this in a137175 Aug 15, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

Reopening for 1.9 backport.

@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>
@aclements

This comment has been minimized.

Copy link
Member

commented Aug 19, 2017

Fixed on 1.9 by fbf7e1f

@aclements aclements closed this Aug 19, 2017

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