Allow version-tagged filepath in tests

thsnr commented 6 years ago

Go 1.11 will introduce support for versioned modules. When downloading module dependencies, go get will add a version tag to the directory where the package is checked out (e.g., github.com/pkg/errors@v0.8.0).

It is also recommended as a best practice to run go test all before release, which will run tests for the package being released and all dependencies to check compatibility. Currently this fails because the tests are not happy to be checked out in a version-tagged directory.

$ cat go.mod 
module github.com/thsnr/gomod

require github.com/pkg/errors v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded

$ go1.11rc1 test all
?       github.com/thsnr/gomod  [no test files]
...
--- FAIL: TestFrameFormat (0.00s)
    format_test.go:379: test 2: line 2: fmt.Sprintf("%+s", err):
         got: "github.com/pkg/errors.init\n\t/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go"
        want: "github.com/pkg/errors.init\n\t.+/github.com/pkg/errors/stack_test.go"
    format_test.go:379: test 12: line 2: fmt.Sprintf("%+v", err):
         got: "github.com/pkg/errors.init\n\t/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go:9"
        want: "github.com/pkg/errors.init\n\t.+/github.com/pkg/errors/stack_test.go:9"
...

This pull request simply changes the tests to allow a version tag. I purposefully left the version regular expression vague (@v[^/]+) to keep it short – these tests should not concern themselves with correctness of the tag – and inlined it to avoid changing any line numbers.

davecheney commented 6 years ago

Thanks for your PR. I want to think about why this is breaking in the first place before I apply this fix. I'm sure its correct, but I wonder if it's papering over a broader problem with assuming the prefix is stable.

On 17 August 2018 at 20:18, Tiit Pikma notifications@github.com wrote:

Go 1.11 will introduce support for versioned modules https://github.com/golang/go/wiki/Modules. When downloading module dependencies, go get will add a version tag to the directory where the package is checked out (e.g., github.com/pkg/errors@v0.8.0).

It is also recommended as a best practice to run go test all before release, which will run tests for the package being released and all dependencies to check compatibility. Currently this fails because the tests are not happy to be checked out in a version-tagged directory.

$ cat go.mod module github.com/thsnr/gomod

require github.com/pkg/errors v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded

$ go1.11rc1 test all ? github.com/thsnr/gomod [no test files] ... --- FAIL: TestFrameFormat (0.00s) format_test.go:379: test 2: line 2: fmt.Sprintf("%+s", err): got: "github.com/pkg/errors.init\n\t/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go http://github.com/pkg/errors.init%5Cn%5Ct/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go" want: "github.com/pkg/errors.init\n\t.+/github.com/pkg/errors/stack_test.go http://github.com/pkg/errors.init%5Cn%5Ct.+/github.com/pkg/errors/stack_test.go" format_test.go:379: test 12: line 2: fmt.Sprintf("%+v", err): got: "github.com/pkg/errors.init\n\t/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go:9 http://github.com/pkg/errors.init%5Cn%5Ct/home/tiit/go/pkg/mod/github.com/pkg/errors@v0.0.0-00000000000000-816c9085562cd7ee03e7f8188a1cfd942858cded/stack_test.go:9" want: "github.com/pkg/errors.init\n\t.+/github.com/pkg/errors/stack_test.go:9 http://github.com/pkg/errors.init%5Cn%5Ct.+/github.com/pkg/errors/stack_test.go:9" ...

This pull request simply changes the tests to allow a version tag. I purposefully left the version regular expression vague (@v[^/]+) to keep it short – these tests should not concern themselves with correctness of the tag – and inlined it to avoid changing any line numbers.

You can view, comment on, or merge this pull request online at:

https://github.com/pkg/errors/pull/165 Commit Summary

Allow version-tagged filepath in tests

File Changes

M format_test.go https://github.com/pkg/errors/pull/165/files#diff-0 (56)

M stack_test.go https://github.com/pkg/errors/pull/165/files#diff-1 (24)

Patch Links:

https://github.com/pkg/errors/pull/165.patch

https://github.com/pkg/errors/pull/165.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pkg/errors/pull/165, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcA970b6TO410Yxo4OH2lyCGOar2YIks5uRph_gaJpZM4WBUvD .

ChrisHines commented 6 years ago

FWIW, I was just playing with adding module support to github.com/go-stack/stack and encountered the same problem with the tests in that package. In particular, if I work in module mode outside GOPATH with go1.11rc1 then the logic to trim the compile time GOPATH from source file paths no longer works because there is no compile time GOPATH any more. That breaks the %+s and %+v formats.

I don't have a great solution yet.

davecheney commented 6 years ago

Thanks Chris, I had a suspicion that this was a symptom of a larger problem.

@thsnr I am going to close this PR as I don't think it is the right approach. I would appreciate it if someone would open an issue about the path trimming not working outside GOPATH.

Thank you.

thsnr commented 6 years ago

Ah yes, I forgot to mention that the tests fail only when operating in "module-aware mode" as opposed to "GOPATH mode".

@davecheney I do not think I fully understand the GOPATH trimming problem. While yes, github.com/go-stack/stack does trim the GOPATH from filenames before printing, I see no such behavior here in github.com/pkg/errors (although promised in https://github.com/pkg/errors/blob/master/stack.go#L49).

I even ran a small test-case:

package main

import (
        "fmt"

        "github.com/pkg/errors"
)

type stackTracer interface {
        StackTrace() errors.StackTrace
}

func main() {
        err := errors.New("stack")
        fmt.Printf("%+s\n\n", err.(stackTracer).StackTrace()[0])
        fmt.Printf("%+v\n", errors.New("stack"))
}

Result in "module-aware mode":

main.main
        /tmp/gomod/main.go

stack
main.main
        /tmp/gomod/main.go:16
runtime.main
        /home/tiit/sdk/go1.11rc1/src/runtime/proc.go:201
runtime.goexit
        /home/tiit/sdk/go1.11rc1/src/runtime/asm_amd64.s:1333

Result in "GOPATH mode":

main.main
        /tmp/gopath/src/github.com/thsnr/stack/main.go

stack
main.main
        /tmp/gopath/src/github.com/thsnr/stack/main.go:16
runtime.main
        /home/tiit/sdk/go1.11rc1/src/runtime/proc.go:201
runtime.goexit
        /home/tiit/sdk/go1.11rc1/src/runtime/asm_amd64.s:1333

(Output when using Go 1.10 was same as GOPATH mode, just with a different GOROOT.)

No GOPATH trimming is happening, it simply uses the output of Func.FileLine.

If I could understand the situation better, I would be happy to open a new issue.

davecheney commented 6 years ago

@thsnr sorry I forgot to close the pull request. I am doing so now.

Please lets continue this question on an issue. We don't do design and bug traige in pull requests. Thank you.

pkg / errors

Allow version-tagged filepath in tests #165

This pull request simply changes the tests to allow a version tag. I purposefully left the version regular expression vague (@v[^/]+) to keep it short – these tests should not concern themselves with correctness of the tag – and inlined it to avoid changing any line numbers.