code review 6525052: runtime/race: add unit tests for race detector

runtime/race: add unit tests for race detector

[dvyukov, 2012-10-07 18:15:33]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[dvyukov, 2012-10-07 18:25:45]

What do you think about high-level organisation?

All test files except race_test.go are +build ignore.
race_test.go executes 'go test -race -v all_other_test_files', and parses the
output.
For tests starting with func TestRaceXXX it expects a race to be detected. For
tests starting with func TestNoRaceXXX it expects no race to be detected.
Then prints summary along the lines of:
NoRaceFin                                .
RaceFin                                  FAILED
NoRaceCgoSync                            .
NoRaceIOFile                             .
NoRaceIOHttp                             .
Passed 209 of 229 tests (91.27%, 0+, 20-).

0+ means 0 false positives.
20- means 20 false negatives.

I think I need to add "expected failures" to be able to run it on buildbot.

[remyoudompheng, 2012-11-01 19:51:29]

A test case I stumbled upon today:

package main

func Encode() ([]byte, error) {
	t := []byte("hello world")
	return t, nil
}

func main() {
	var s []byte
	ch := make(chan bool, 1)
	go func() {
		var err error
		s, err = Encode()
		ch <- true
		_ = err
	}()
	str := string(s)
	<-ch
	_ = str
}

[dvyukov, 2012-11-01 19:59:01]

On 2012/11/01 19:51:29, remyoudompheng wrote:
> A test case I stumbled upon today:
> 
> package main
> 
> func Encode() ([]byte, error) {
> 	t := []byte("hello world")
> 	return t, nil
> }
> 
> func main() {
> 	var s []byte
> 	ch := make(chan bool, 1)
> 	go func() {
> 		var err error
> 		s, err = Encode()
> 		ch <- true
> 		_ = err
> 	}()
> 	str := string(s)
> 	<-ch
> 	_ = str
> }

:)
Did race detector help you?

[remyoudompheng, 2012-11-01 20:07:28]

It sort of didn't. I wanted to write a trivial example for stack split, so I
used "s, err = json.Marshal(blah)" since encoding/json is usually stack hungry.

And was surprised to see it was not reporting anything for a concurrent read of
s. See http://codereview.appspot.com/6821068

[dvyukov, 2012-11-02 06:39:00]

On 2012/11/01 20:07:28, remyoudompheng wrote:
> It sort of didn't. I wanted to write a trivial example for stack split, so I
> used "s, err = json.Marshal(blah)" since encoding/json is usually stack
hungry.
> 
> And was surprised to see it was not reporting anything for a concurrent read
of
> s. See http://codereview.appspot.com/6821068

Ah, so it's a false negative case... just figured that's because of OBLOCK...
and then looked at your patch :)

[dvyukov, 2012-11-02 08:05:33]

On 2012/11/02 06:39:00, dvyukov wrote:
> On 2012/11/01 20:07:28, remyoudompheng wrote:
> > It sort of didn't. I wanted to write a trivial example for stack split, so I
> > used "s, err = json.Marshal(blah)" since encoding/json is usually stack
> hungry.
> > 
> > And was surprised to see it was not reporting anything for a concurrent read
> of
> > s. See http://codereview.appspot.com/6821068
> 
> Ah, so it's a false negative case... just figured that's because of OBLOCK...
> and then looked at your patch :)


Added the tests for OBLOCK:
https://codereview.appspot.com/6525052/diff2/12019:16019/src/pkg/runtime/race...

[dvyukov, 2012-11-08 16:04:53]

On 2012/10/07 18:25:45, dvyukov wrote:
> What do you think about high-level organisation?
> 
> All test files except race_test.go are +build ignore.
> race_test.go executes 'go test -race -v all_other_test_files', and parses the
> output.
> For tests starting with func TestRaceXXX it expects a race to be detected. For
> tests starting with func TestNoRaceXXX it expects no race to be detected.
> Then prints summary along the lines of:
> NoRaceFin                                .
> RaceFin                                  FAILED
> NoRaceCgoSync                            .
> NoRaceIOFile                             .
> NoRaceIOHttp                             .
> Passed 209 of 229 tests (91.27%, 0+, 20-).
> 
> 0+ means 0 false positives.
> 20- means 20 false negatives.
> 
> I think I need to add "expected failures" to be able to run it on buildbot.

I've added the ability to mark "expected to fail" tests. Now this test can pass
even with some not-yet-implemented race detector features. I.e. it does not fail
go test -race std.
PTAL

[rsc, 2012-11-12 21:49:35]

I suggest moving the excluded files into a testdata subdirectory. The go command
knows to ignore files in testdata directories, even *.go files.

[dvyukov, 2012-11-14 14:15:58]

On 2012/11/12 21:49:35, rsc wrote:
> I suggest moving the excluded files into a testdata subdirectory. The go
command
> knows to ignore files in testdata directories, even *.go files.

Done. PTAL.

[dvyukov, 2012-11-23 09:52:39]

Hello golang-dev@googlegroups.com, remyoudompheng@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-11-26 17:11:56]

LGTM

https://codereview.appspot.com/6525052/diff/37019/src/pkg/runtime/race/race_t...
File src/pkg/runtime/race/race_test.go (right):

https://codereview.appspot.com/6525052/diff/37019/src/pkg/runtime/race/race_t...
src/pkg/runtime/race/race_test.go:28: passedTests, totalTests, falsePos,
falseNeg, failingPos, failingNeg int
please split these into one per line.
it will scale better and avoid the comical next line

[dvyukov, 2012-11-27 11:04:03]

https://codereview.appspot.com/6525052/diff/37019/src/pkg/runtime/race/race_t...
File src/pkg/runtime/race/race_test.go (right):

https://codereview.appspot.com/6525052/diff/37019/src/pkg/runtime/race/race_t...
src/pkg/runtime/race/race_test.go:28: passedTests, totalTests, falsePos,
falseNeg, failingPos, failingNeg int
On 2012/11/26 17:11:56, rsc wrote:
> please split these into one per line.
> it will scale better and avoid the comical next line

Done.

[dvyukov, 2012-11-27 11:05:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=e03ad98f9165 ***

runtime/race: add unit tests for race detector

R=golang-dev, remyoudompheng, rsc
CC=golang-dev
http://codereview.appspot.com/6525052