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/fix: incorrect loop variable aliasing in TestRewrite #35632

Closed
mewmew opened this issue Nov 16, 2019 · 4 comments
Closed

cmd/fix: incorrect loop variable aliasing in TestRewrite #35632

mewmew opened this issue Nov 16, 2019 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@mewmew
Copy link
Contributor

mewmew commented Nov 16, 2019

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

$ go version
go version devel +c20b71eb37 Sat Nov 16 02:06:39 2019 +0000 linux/amd6

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/u/.cache/go-build"
GOENV="/home/u/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/u/goget:/home/u/Desktop/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/u/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/u/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/u/go/src/cmd/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build745246669=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. clone the go repository: git clone https://go.googlesource.com/go.git
  2. build go as usual: cd src; ./all.bash
  3. edit cmd/fix/context_test.go to ensure that the test case should FAIL.
diff --git a/src/cmd/fix/context_test.go b/src/cmd/fix/context_test.go
index 935d0d7235..f666be2fb1 100644
--- a/src/cmd/fix/context_test.go
+++ b/src/cmd/fix/context_test.go
@@ -19,7 +19,7 @@ var _ = "golang.org/x/net/context"
 `,
                Out: `package main
 
-import "context"
+import "context"x
 
 var _ = "golang.org/x/net/context"
 `,
  1. run go test from the src/cmd/fix directory.
$ go test
PASS
ok  	cmd/fix	0.032s
  1. notice that the test case passes, even though it should fail, given that we edited the expected output of the context rewrite rule.

  2. remove call to t.Parallel in cmd/fix/main_test.go

diff --git a/src/cmd/fix/main_test.go b/src/cmd/fix/main_test.go
index ee74f24c6e..5214d67af1 100644
--- a/src/cmd/fix/main_test.go
+++ b/src/cmd/fix/main_test.go
@@ -77,7 +77,7 @@ func parseFixPrint(t *testing.T, fn func(*ast.File) bool, desc, in string, mustB
 func TestRewrite(t *testing.T) {
        for _, tt := range testCases {
                t.Run(tt.Name, func(t *testing.T) {
-                       t.Parallel()
+                       //t.Parallel()
                        // Apply fix: should get tt.Out.
                        out, fixed, ok := parseFixPrint(t, tt.Fn, tt.Name, tt.In, true)
                        if !ok {
  1. re-run test case using go test from the src/cmd/fix directory.
--- FAIL: TestRewrite (2.86s)
    --- FAIL: TestRewrite/context.0 (0.00s)
        main_test.go:94: incorrect output.
        main_test.go:96: --- have
            package main
            
            import "context"
            
            var _ = "golang.org/x/net/context"
            
            --- want
            package main
            
            import "context"x
            
            var _ = "golang.org/x/net/context"
        main_test.go:133: --- /tmp/go-fix-test929058818	2019-11-15 21:39:47.643666701 -0600
            +++ /tmp/go-fix-test221839737	2019-11-15 21:39:47.643666701 -0600
            @@ -1,5 +1,5 @@
             package main
             
            -import "context"
            +import "context"x
             
             var _ = "golang.org/x/net/context"
            
FAIL
exit status 1
FAIL	cmd/fix	2.866s
  1. notice that the test case now fails, as it should do, given that we edited the expected output of the context rewrite rule.

What did you expect to see?

A failed test case (after having modified the expected output of the context_test.go test case).

What did you see instead?

Test cases passing without error.

@mewmew mewmew changed the title cmd/fix: go test always reports PASS when using t.Parallel, once when it should FAIL cmd/fix: go test always reports PASS when using t.Parallel, even when it should FAIL Nov 16, 2019
@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2019

Run the test with -race. You seem to be running afoul of #16520.

@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2019

Hrm, even with -race the race is undetected. That's unfortunate.

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Nov 18, 2019
@bcmills bcmills added this to the Go1.14 milestone Nov 18, 2019
@bcmills bcmills changed the title cmd/fix: go test always reports PASS when using t.Parallel, even when it should FAIL cmd/fix: incorrect loop variable aliasing in TestRewrite Nov 18, 2019
@bcmills bcmills self-assigned this Nov 18, 2019
@gopherbot
Copy link

Change https://golang.org/cl/207603 mentions this issue: cmd/fix: eliminate data races in TestRewrite and gofmt

@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2019

Filed #35670 for the missing -race-mode check.

@golang golang locked and limited conversation to collaborators Nov 17, 2020
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants