code review 42790044: errors: add Equal

errors: add Equal

This is a helper method for a situation which crops up fairly
frequently in tests.

Fixes issue 6965.

[hdm, 2013-12-16 14:51:30]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-12-16 15:54:22]

NOT LGTM

This can be defined elsewhere if this is the definition of equality your
code wants.
 On Dec 16, 2013 7:12 AM, <hdm@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> errors: add Equal
>
> This is a helper method for a situation which crops up fairly
> frequently in tests.
>
> Fixes issue 6965.
>
> Please review this at https://codereview.appspot.com/42790044/
>
> Affected files (+26, -0 lines):
>   M src/pkg/errors/errors.go
>   M src/pkg/errors/errors_test.go
>
>
> Index: src/pkg/errors/errors.go
> ===================================================================
> --- a/src/pkg/errors/errors.go
> +++ b/src/pkg/errors/errors.go
> @@ -18,3 +18,11 @@
>  func (e *errorString) Error() string {
>         return e.s
>  }
> +
> +// Equal returns true, iff the two errors are equal or both are nil.
> +func Equal(a, b error) bool {
> +       if a == nil {
> +               return b == nil
> +       }
> +       return b != nil && a.Error() == b.Error()
> +}
> Index: src/pkg/errors/errors_test.go
> ===================================================================
> --- a/src/pkg/errors/errors_test.go
> +++ b/src/pkg/errors/errors_test.go
> @@ -51,3 +51,21 @@
>         }
>         // Output: user "bimmler" (id 17) not found
>  }
> +
> +func TestEqual(t *testing.T) {
> +       e := errors.New("boo")
> +       for i, et := range []struct {
> +               a, b  error
> +               equal bool
> +       }{
> +               {nil, nil, true},
> +               {nil, e, false},
> +               {e, nil, false},
> +               {e, e, true},
> +               {e, errors.New("foo"), false},
> +       } {
> +               if equal := errors.Equal(et.a, et.b); equal != et.equal {
> +                       t.Errorf("%d. Equal(%q, %q) = %v; want = %v", i,
> et.a, et.b, equal, et.equal)
> +               }
> +       }
> +}
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[hdm, 2013-12-16 15:59:58]

On Mon, Dec 16, 2013 at 3:54 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> NOT LGTM
>
> This can be defined elsewhere if this is the definition of equality your
> code wants.
>
We have defined it elsewhere.  It seemed useful enough to be generic.

Given that it's only cropped up during testing, would it make more sense as
part of the testing package?

[iant, 2013-12-16 16:55:21]

If your API benefits from testing errors for equality, then it is generally
better to export the errors, along the lines of io.EOF and io.ErrShortWrite.

So what you are after here is something that helps you test whether you get a
specific error from some test case.  It's a reasonable case but it's not the
kind of thing we've historically added to the testing package.

Also, you can always use reflect.DeepEqual, so I'm not sure a new function is
really required.

Thanks for sending the CL, though.

[hdm, 2013-12-16 17:18:33]

On Mon, Dec 16, 2013 at 4:55 PM, <iant@golang.org> wrote:

> If your API benefits from testing errors for equality, then it is
> generally better to export the errors, along the lines of io.EOF and
> io.ErrShortWrite.
>

We do this where possible.  But in some cases, we are returning errors
which are specific to a processed file, and it's useful to include the
filename in the error.


> So what you are after here is something that helps you test whether you
> get a specific error from some test case.  It's a reasonable case but
> it's not the kind of thing we've historically added to the testing
> package.
>

The background is that we frequently have a loop of structs in our tests
(similar to the one in this change).  We're trying to come up with one
expression that tests all the cases of returning nil vs returning errors.


> Also, you can always use reflect.DeepEqual, so I'm not sure a new
> function is really required.
>

Thanks — that sounds sensible.

-Dom

[hdm, 2013-12-16 18:11:42]

On Mon, Dec 16, 2013 at 5:18 PM, Dominic Mitchell <hdm@google.com> wrote:

> On Mon, Dec 16, 2013 at 4:55 PM, <iant@golang.org> wrote:
>
>> If your API benefits from testing errors for equality, then it is
>> generally better to export the errors, along the lines of io.EOF and
>> io.ErrShortWrite.
>>
>
> We do this where possible.  But in some cases, we are returning errors
> which are specific to a processed file, and it's useful to include the
> filename in the error.
>
>
>> So what you are after here is something that helps you test whether you
>> get a specific error from some test case.  It's a reasonable case but
>> it's not the kind of thing we've historically added to the testing
>> package.
>>
>
> The background is that we frequently have a loop of structs in our tests
> (similar to the one in this change).  We're trying to come up with one
> expression that tests all the cases of returning nil vs returning errors.
>
>
>> Also, you can always use reflect.DeepEqual, so I'm not sure a new
>> function is really required.
>>
>
> Thanks — that sounds sensible.
>

Hmm.  reflect.DeepEqual() works in most cases, except where we use
errors.New() in the test, and fmt.Errorf() in the production code.  I'll
fall back to locally comparing a.Error() == b.Error() (after nil checks).

-Dom

[rsc, 2013-12-17 17:18:21]

R=close