Avoid asserting on an error message, just require an error to be present

[samuong]

Fixes #40

[samuong]

I've never used errors.Is before. It sounds like a good idea, but I have a few questions:

It looks like Is (as well as As and Unwrap) was introduced in Go 1.13. I'm thinking of just using the errors package directly, and making Alpaca require 1.13. Or do you think it's better to use the golang.org/x/exp/errors package for now and force the upgrade later? (and if so, when?)

The error that I'd be asserting on comes from a call to exec#Cmd.Output() (see nomad_darwin.go:35). So I think the "base" error would be that error (which I don't control).

I could do something like create an error that wraps two errors:

// 1. create a global error var
var CommandError = errors.New("command failed")
// 2. when running the command, create an error that wraps both the global error and the error from Output()
if out, err := cmd.Output(); err != nil {
    return "", fmt.Errorf("%w %w", err, CommandError)
}
// 3. assert using errors.Is in the test
assert.True(t, errors.Is(err, CommandError)

It feels a little strange to be creating one error to wrap two other errors though. Another way to do it would be to use errors.As:

var exitErr *exec.ExitError // cmd.Output() returns an error of type exec.ExitError on failure
assert.True(t, errors.As(err, exitErr))

I'm leaning towards the second one, what do you think? Also, is it better to use my own custom error types, or the ones from the standard library (exec.ExitError)?

[juliaogris]

Sorry for the late reply.

Tldr: agreed and agreed

All of this is really only if you enjoy it, what you have is just fine I'd say.

But yeah, I'd agree: use std ilb and mandate go 1.13 rather than a 3p import - that's one of the things that we really love about alpaca: how it does it all without 3p.

I also think that method 2 with error.As and type comparison is clearer. I think wrap and Is and As only work linearly, ie wrapping ONE error at a time, not with a trees - does your method 1 even work/compile (curious)?

On Fri, Dec 20, 2019, 8:21 AM samuong notifications@github.com wrote:

I've never used errors.Is before. It sounds like a good idea, but I have a few questions:

It looks like Is (as well as As and Unwrap) was introduced in Go 1.13 https://blog.golang.org/go1.13-errors. I'm thinking of just using the errors package https://godoc.org/errors directly, and making Alpaca require 1.13. Or do you think it's better to use the golang.org/x/exp/errors package https://godoc.org/golang.org/x/exp/errors for now and force the upgrade later? (and if so, when?)

The error that I'd be asserting on comes from a call to exec#Cmd.Output() https://godoc.org/os/exec#Cmd.Output (see nomad_darwin.go:35 https://github.com/samuong/alpaca/blob/66788201fc1aa350e4744ef54811e88521f90707/nomad_darwin.go#L35). So I think the "base" error would be that error (which I don't control).

I could do something like create an error that wraps two errors:

// 1. create a global error var var CommandError = errors.New("command failed") // 2. when running the command, create an error that wraps both the global error and the error from Output() if out, err := cmd.Output(); err != nil { return "", fmt.Errorf("%w %w", err, CommandError) } // 3. assert using errors.Is in the test assert.True(t, errors.Is(err, CommandError)

It feels a little strange to be creating one error to wrap two other errors though. Another way to do it would be to use errors.As:

var exitErr *exec.ExitError // cmd.Output() returns an error of type exec.ExitError on failure assert.True(t, errors.As(err, exitErr))

I'm leaning towards the second one, what do you think? Also, is it better to use my own custom error types, or the ones from the standard library (exec.ExitError)?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/samuong/alpaca/pull/41?email_source=notifications&email_token=AAMF3R3SJEZVN326FNWC5B3QZPQV5A5CNFSM4J5BZNWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHLCFRY#issuecomment-567681735, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMF3RZPGYFPQANPZU4ZFWLQZPQV5ANCNFSM4J5BZNWA .

[samuong]

Ok. I think it's worth doing, but I'm going to do it later (unless someone beats me to it). I've filed issue #43 so that I don't lose track of it.

The reason is that I don't have access to a Mac right now (will be getting a new Macbook in a few weeks) which makes it hard to make further changes to this PR. I don't want to leave the Mac build broken any longer.