os: "panic: Fail in goroutine after TestKillStartProcess has completed"

[zx2c4]

Another flaky test:

=== RUN   TestKillFindProcess
panic: Fail in goroutine after TestKillStartProcess has completed

goroutine 7417 [running]:
testing.(*common).Fail(0xc0007e4600)
        C:/Go/src/testing/testing.go:698 +0x127
testing.(*common).FailNow(0xc0007e4600)
        C:/Go/src/testing/testing.go:720 +0x32
testing.(*common).Fatalf(0xc0007e4600, 0xa47d8a, 0x1f, 0xc0006d9f98, 0x1, 0x1)
        C:/Go/src/testing/testing.go:816 +0x97
os_test.TestKillStartProcess.func1(0xc0003ce8d0)
        C:/Go/src/os/os_test.go:2331 +0xc7
os_test.testKillProcess.func1(0xc00028a0d0, 0xc000298000)
        C:/Go/src/os/os_test.go:2310 +0x4b
created by os_test.testKillProcess
        C:/Go/src/os/os_test.go:2308 +0x157
FAIL    os      67.523s
FAIL
go: failed to remove work dir: remove C:\Go\tmp\go-build3763818576\b001\os.test.exe: Access is denied.

cc @bcmills

go test -v  os -count 10000

[bcmills]

This turns out not to be Windows-specific. There are at least two defects in the test.

1. The test does not wait for the processKiller callback to return, so it may fail after the test function has already returned (as observed here).
   

   go/src/os/os_test.go

   Lines 2308 to 2311 in 0c5afc4

   	go func() {
   	time.Sleep(100 * time.Millisecond)
   	processKiller(cmd.Process)
   	}()

2. The callback invokes t.Fatalf from a non-main goroutine — which may invoke runtime.Goexit — but does not propagate the Goexit back to the caller. It should probably use t.Errorf instead.
   

   go/src/os/os_test.go

   Lines 2328 to 2333 in 0c5afc4

   	testKillProcess(t, func(p *Process) {
   	err := p.Kill()
   	if err != nil {
   	t.Fatalf("Failed to kill test process: %v", err)
   	}
   	})

[gopherbot]

Change https://golang.org/cl/284153 mentions this issue: os: invoke processKiller synchronously in testKillProcess