Sometimes, add a -/+ diff to DiffErrString output

[drevell]

This helps users see at a glance where two errors are different, when errors are large.

To avoid cluttering the output with -got/+want output when unneeded, we guard this logic to only run when either:

• the message sizes are both greater than 20 bytes OR
• the messages are both multiple lines

[sethvargo]

I wonder if we would be better off just using cmp on the string? https://pkg.go.dev/github.com/google/go-cmp/cmp

[drevell]

I wonder if we would be better off just using cmp on the string? https://pkg.go.dev/github.com/google/go-cmp/cmp

I can't quite understand what the suggestion is here.

If the suggestion is to use that library to generate the diff, that's what this code already does.

If the suggestion is to remove DiffErrString() in favor of using cmp.Diff directly, that seems like a step backward. DiffErrString is really clean to use because it handles

• empty/nil
• substring contains
• calling error.Error() for you

[sethvargo]

I wonder if we would be better off just using cmp on the string? https://pkg.go.dev/github.com/google/go-cmp/cmp

I can't quite understand what the suggestion is here.

If the suggestion is to use that library to generate the diff, that's what this code already does.

If the suggestion is to remove DiffErrString() in favor of using cmp.Diff directly, that seems like a step backward. DiffErrString is really clean to use because it handles

• empty/nil
• substring contains
• calling error.Error() for you

Sorry I wasn't super clear. I wasn't suggesting that we replace this with cmp; I was suggesting that we ask cmp to do the diff (after we've done nil-checks). So it'd look like:

func DiffErrString(got error, want string) string {
	if want == "" {
		if got == nil {
			return ""
		}
		return fmt.Sprintf("got error %q but want <nil>", got.Error())
	}
	if got == nil {
		return fmt.Sprintf("got error <nil> but want an error containing %q", want)
	}
	return cmp.Diff(got.Error(), want)
}

[drevell]

I wonder if we would be better off just using cmp on the string? https://pkg.go.dev/github.com/google/go-cmp/cmp

I don't think that would be as good. Consider the case where we're just matching a short substring of a large error, like DiffErrString(aGiganticErr, "does not exist"). In that case, you'd probably rather see a message like got error %q but want an error containing %q. A diff would be more confusing to read when comparing small thing to a large thing and they don 't have any similarity.. The check for "both got and want are large" is a decent heuristic for when a diff would be helpful and readable.