Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Add support for Go 1.13 error chains #206

Merged
merged 4 commits into from
Nov 9, 2019
Merged

Add support for Go 1.13 error chains #206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7427754
Add support for Go 1.13 error chains
jayschwa Jul 3, 2019
3958db8
Rename go1.13_compat_test.go to go113_test.go
jayschwa Nov 1, 2019
567a96c
Merge branch 'master' into go1.13-compat
jayschwa Nov 9, 2019
afdfa07
Move go113_test back into errors package
jayschwa Nov 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ type withStack struct {

func (w *withStack) Cause() error { return w.error }

// Unwrap provides compatibility for Go 1.13 error chains.
func (w *withStack) Unwrap() error { return w.error }

func (w *withStack) Format(s fmt.State, verb rune) {
switch verb {
case 'v':
Expand Down Expand Up @@ -241,6 +244,9 @@ type withMessage struct {
func (w *withMessage) Error() string { return w.msg + ": " + w.cause.Error() }
func (w *withMessage) Cause() error { return w.cause }

// Unwrap provides compatibility for Go 1.13 error chains.
func (w *withMessage) Unwrap() error { return w.cause }

func (w *withMessage) Format(s fmt.State, verb rune) {
switch verb {
case 'v':
Expand Down
18 changes: 18 additions & 0 deletions go1.13_compat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// +build go1.13

package errors_test
aperezg marked this conversation as resolved.
Show resolved Hide resolved

import (
stdlib_errors "errors"
aperezg marked this conversation as resolved.
Show resolved Hide resolved
"testing"

"github.com/pkg/errors"
aperezg marked this conversation as resolved.
Show resolved Hide resolved
)

func TestErrorChainCompat(t *testing.T) {
err := stdlib_errors.New("error that gets wrapped")
aperezg marked this conversation as resolved.
Show resolved Hide resolved
wrapped := errors.Wrap(err, "wrapped up")
aperezg marked this conversation as resolved.
Show resolved Hide resolved
if !stdlib_errors.Is(wrapped, err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !stdlib_errors.Is(wrapped, err) {
if !errors.Is(wrapped, err) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is not necessary use the package _test in this case because you can use directly the std library and check against the package itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted it to be a black box test, and that is why I put it in a separate package.

Copy link
Member

@aperezg aperezg Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so is a good argument to me, we need to merge this #212 before that we can merge your PR, because in otherwise the travis ci will never pass. But it's look great, and I would like to merge this ASAP :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once #212 is merged, I will rebase and squash this branch.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see the utility here in actually pushing it into a separate package. You can still do blackbox testing even if technically you could look inside the box.

The design is in the design, not in the arbitrarily forcing a semantic. Consider as well all the people forking the project, and now that test is going to pull this package for their tests, and not their own forked repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point and is very reasonable, but the package itself can't be tested normally from a fork, because have the repository path hardcode in some tests, for example:

https://github.com/pkg/errors/blob/master/format_test.go#L29

So if anyone fork this repository, he/she must be moved under $GOPATH/src/github.com/pkg/errors or they also could use docker instead. Otherwise the test will never pass on local.

Furthermore this can also be resolved using go modules, but yes for now the package haven't this functionality.

But, I agree that in this case one way on another to do this black box test is similar to the other. So maybe would be ok, if we try to avoid more accoplated code with the package itself.

What do you think @puellanivis?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm… fun test. But a critical difference is that the format_test would break loudly, while this would not even break, because a forking author may not even realize that this import is here, and thus get a false sense of security that their tests are passing, even though they are not even running against their own code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I totally agree that are two different case, this case could produce as you said false sense of security. My point was that with the current state of the package tests, the package couldn't be tested from another directory. But, I see your point.

@jayschwa could you change the test and use the package errors instead the package errors_test? Is truth that Go provide with the package _test to realize the black box test on secure way, but we can mantain this test as black box even without that.

Thanks 😁

PS. the PR #212 is already merged, so your pipeline should be pass this time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the test back into the errors package and CI is green.

t.Errorf("Wrap does not support Go 1.13 error chains")
}
}