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: Go1.7 sub-test with t.Parallel() using data from loop out of its scope #16586

Closed
VojtechVitek opened this issue Aug 3, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@VojtechVitek
Copy link

commented Aug 3, 2016

Hi there,
I was playing with go1.7's sub-tests today and I ran into a not really obvious side-effect of marking sub-test with t.Parallel().

func TestSubtests(t *testing.T) {
    routes := []struct {
        url  string
        path string
    }{
        {"http://example.com/1", "/1"},
        {"http://example.com/2", "/2"},
        {"http://example.com/3", "/3"},
        {"http://example.com/4", "/4"},
        {"http://example.com/5", "/5"},
    }

    t.Run("sequential", func(t *testing.T) {
        for _, tt := range routes {
            t.Run(tt.url, func(t *testing.T) {
                u, _ := url.Parse(tt.url)
                if u.Path != tt.path {
                    t.Errorf("expected %v, got %v", tt.path, u.Path)
                }
            })
        }
    })

    t.Run("parallel", func(t *testing.T) {
        for _, tt := range routes {
            t.Run(tt.url, func(t *testing.T) {

                t.Parallel() // <== trying to set Parallel(), while using tt from the range loop

                u, _ := url.Parse(tt.url)
                if u.Path != tt.path {
                    t.Errorf("expected %v, got %v", tt.path, u.Path)
                }
            })
        }
    })
}
$ go test -v
--- PASS: TestSubtests (0.00s)
    --- PASS: TestSubtests/sequential (0.00s)
        --- PASS: TestSubtests/sequential/http://example.com/1 (0.00s)
            main_test.go:28: tested /1
        --- PASS: TestSubtests/sequential/http://example.com/2 (0.00s)
            main_test.go:28: tested /2
        --- PASS: TestSubtests/sequential/http://example.com/3 (0.00s)
            main_test.go:28: tested /3
        --- PASS: TestSubtests/sequential/http://example.com/4 (0.00s)
            main_test.go:28: tested /4
        --- PASS: TestSubtests/sequential/http://example.com/5 (0.00s)
            main_test.go:28: tested /5

    --- PASS: TestSubtests/parallel (0.00s)
        --- PASS: TestSubtests/parallel/http://example.com/1 (0.00s)
            main_test.go:48: tested /5
        --- PASS: TestSubtests/parallel/http://example.com/4 (0.00s)
            main_test.go:48: tested /5
        --- PASS: TestSubtests/parallel/http://example.com/5 (0.00s)
            main_test.go:48: tested /5
        --- PASS: TestSubtests/parallel/http://example.com/3 (0.00s)
            main_test.go:48: tested /5
        --- PASS: TestSubtests/parallel/http://example.com/2 (0.00s)
            main_test.go:48: tested /5

                                ^ always "tested /5"

This bahavior was not obvious to me from the beginning, since all of my tests still passed :) But after a while I figured there was a concurrency issue similar to this:

    for _, tt := range routes {
        go func() {
            fmt.Println(tt.url) // Not guaranteed which item will be stored in tt during the goroutine execution.
        }()
    }

which is easy to solve by passing the value onto goroutine's stack:

    for _, tt := range routes {
        go func(url string) {
            fmt.Println(url)
        }(tt.url)
    }

Question

I'm trying to figure out a fix (similar to passing data onto goroutine's stack) for the above Parallel sub-test. Any suggestions?

Documentation suggestion

Imho, the t.Parallel() behavior should be documented better, especially in the context of sub-tests + table driven tests.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2016

This is a common pattern in any kind of parallel loop. Nothing is specific to t.Parallel here. Effective Go recommends making a local copy of the variable inside the loop:

    t.Run("parallel", func(t *testing.T) {
        for _, tt := range routes {
            tt := tt
            t.Run(tt.url, func(t *testing.T) {

                t.Parallel() // <== trying to set Parallel(), while using tt from the range loop

                u, _ := url.Parse(tt.url)
                if u.Path != tt.path {
                    t.Errorf("expected %v, got %v", tt.path, u.Path)
                }
            })
        }
    })

As Effective Go says, "It may seem odd to write req := req but it's legal and idiomatic in Go to do this. You get a fresh version of the variable with the same name, deliberately shadowing the loop variable locally but unique to each goroutine."

@quentinmit quentinmit closed this Aug 3, 2016

@VojtechVitek

This comment has been minimized.

Copy link
Author

commented Aug 3, 2016

@qeedquan nice find about req := req.

But should we improve t.Parallel() godoc rather than closing this issue right away? I bet I won't be the only one wasting time on this.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2016

I think you mean @quentinmit

This problem can happen with any API that takes a func pointer. t.Run is but one of many in that standard libary, should we really document all of them with "beware of calling this from a for loop"? If it's not obvious something will execute in parallel, I agree that should be documented, but given that the function is named "Parallel" I think that part is already documented pretty well.

@VojtechVitek

This comment has been minimized.

Copy link
Author

commented Aug 3, 2016

Sorry about the wrong mention, it came first after typing @q.

I agree with you, t.Parallel() documents itself pretty well. But at the same time, I feel like we won't prevent anyone from doing the same mistake, which is very hard to debug - especially when the only executed test actually passes.

@golang golang locked and limited conversation to collaborators Aug 3, 2017

@mikioh mikioh changed the title Go1.7 sub-test with t.Parallel() using data from loop out of its scope testing: Go1.7 sub-test with t.Parallel() using data from loop out of its scope Aug 3, 2017

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.