proposal: errors: add Like for testing that error values appear as expected

[sfllaw]

Problem

Currently, there is no general way to test whether a returned error value is what the developer expected:

• The obvious == operator doesn’t work, since *errors.stringError is designed to be unique: proposal: Add constant error #17226 (comment).
• Using errors.Is is also unsatisfactory because the developer must go through extra effort to test both the wrapped error and the Error string, as demonstrated by fmt_test.TestErrorf.
• reflect.DeepEqual is sometimes correct for testing errors, but makes these tests particularly brittle when it comes to wrapped errors.
• Finally, cmp.Equal is a popular third-party package for comparisons, but even its cmpopts.EquateErrors option doesn’t do the right thing here.

Ideally, a developer could write the following test:

func TestFunc(t *testing.T) {
	wantErr := fmt.Errorf("my: %w", fs.ErrNotExist)
	err := my.Func()
	if !errors.Like(err, wantErr) {
		t.Fatalf("err %v, want %v", err, wantErr)
	}
}

Workarounds

Often, we see a helper function inside the test suite that helps compare Error text by dealing with nil errors:

func Error(err error) string {
	if err == nil {
		return ""
	}
	return err.Error()
}

func TestFunc(t *testing.T) {
	wantErr := fmt.Errorf("my: %w", fs.ErrNotExist)
	err := my.Func()
	if Error(err) != wantErr {
		t.Fatalf("err %v, want %v", err, wantErr)
	}
}

This is unsatisfying now that we have wrapped errors, because even if the Error strings are equal, that doesn’t mean that any wrapped errors match.

Concerns

• The name of errors.Like may not be obvious. I considered errors.Match, but the errors documentation already uses the word “match” with a slightly different meaning.
• Since the primary use-case for this function is to test errors, and not to handle them, it may not belong in the errors package. We don’t want developers to choose Like when they mean Is. Perhaps it should go in a new errors/errorstest package?

Proposed implementation

The following is a proposed implementation of errors.Like:

// Like reports whether err is equivalent to target.
//
// An error is considered to be equivalent if it is equal to the target.
// It is also equivalent if its Error string is equal to the target’s Error
// and its wrapped error is equivalent to the target’s wrapped error.
func Like(err, target error) bool {
	if err == target {
		return true
	}
	if err == nil || target == nil {
		return false
	}
	if utarget := Unwrap(target); utarget != nil {
		if err.Error() != target.Error() {
			return false
		}
		return Like(errors.Unwrap(err), utarget)
	}
	return false
}

You can also find an implementation with test cases in the Go Playground: https://play.golang.org/p/qnBbkSbMlLO

See also

• Working with Errors in Go 1.13 - go.dev
• proposal: Go 2 error values #29934
• errors: feature request: nil-safe compares #6965
• proposal: cmpopts: add convenience functions for errors google/go-cmp#89

[ianlancetaylor]

CC @neild @jba

[robpike]

See https://pkg.go.dev/upspin.io/errors and https://commandcenter.blogspot.com/2017/12/error-handling-in-upspin.html.

As explained in the article, to work well this functionality tends to be application-specific. Your proposal puts too much weight on equality to be widely useful. Things like user names and file names must be factored out.

[jba]

Comparing error strings should be a last resort, when other methods of classification fail. And even then, it would be better to look for stable substrings in the message instead of comparing it whole, for the reason Rob mentioned above.

[D1CED]

It is best to only test what you promise your library users. Here you either over promise or over test. Testing for presence or absence of a specific error with some checks on information carrying errors is good enough in my book.

[sfllaw]

@robpike I definitely appreciate your advocacy of error values and upspin.io/errors is an interesting package. It’s been my experience that very few applications and libraries have a custom errors package:

1. Is it not more common to define sentinel errors with errors.New and use fmt.Errorf to wrap them with additional context?
2. When considering a package that relies solely on the standard library, surely we can design something that isn’t application-specific, much like we did with errors.Is and errors.As?
3. You bring up a good point around matching a subset of the error, as with upspin.io/errors.Match, which the standard library provides with errors.Is. Are you arguing that tests will be cluttered when you write that “user names and file names must be factored out”?

@jba The proposed errors.Like is meant to make it easier for tests to check the wrapped error, in addition to the error string:

1. When you say that comparing error strings is a last resort, are you suggesting that the common idiom of fmt.Errorf("additional context: %w", err) should be tested in a different way? Even looking at Rob’s upspin.io/errors, it seems like the error strings are not left to chance.
2. I acknowledge that there are special cases where it might be impossible for the test author to control randomness in the test setup, but errors.Like doesn’t preclude a custom test. Are you worried that errors.Like will discourage custom tests?
3. Are you concerned that people will use errors.Like outside of tests? I am also concerned about that, which is why I suggested that this might belong in an errors/errorstest package.

@D1CED I agree that libraries must test what they promise to users:

1. If a library’s tests use the proposed errors.Like, wouldn’t it be promising that its error strings won’t change as part of its API? Searching for .Error() == "...", it seems like plenty of existing code would break if error strings were not stable.
2. Why would we consider this over-promising?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[neild]

If a library’s tests use the proposed errors.Like, wouldn’t it be promising that its error strings won’t change as part of its API?

This seems to me to be an excellent argument against adding a function like this. We should not encourage treating error strings as part of a package's API.

[sfllaw]

If a library’s tests use the proposed errors.Like, wouldn’t it be promising that its error strings won’t change as part of its API?

This seems to me to be an excellent argument against adding a function like this. We should not encourage treating error strings as part of a package's API.

@neild Assuming this isn’t a snarky comment, why is it a bad thing for error strings to be part of a package’s API?

For better or for worse, .Error() == "" proves that people are relying on it. If a library author changes their error string, they will be making a backwards-incompatible change. If they are making a backwards-incompatible change, do they not want their tests to warn them?

I have a few of arguments that I can come up with, in no particular order, along with some responses:

1. “The Error method on the error interface exists for humans, not code.”¹
   @davecheney’s article is a good one, and I generally agree with the sentiment, but he also admits that “this advice doesn’t apply to writing tests.” However, the article is a bit dated, since the standard library now encourages the use of sentinel and wrapped errors. I know people argued against proposal: Go 2 error values #29934, but we now live in this world.

2. Error strings might be misspelled or capitalized or contain bad punctuation and library authors should be able to fix these without a major version bump.
   This is the classic HTTP Referer problem where we are stuck with this misspelling forever. If dependent code relies on a misspelled field or method, surely they can expect to rely on a misspelled error.

3. Error strings should be localizable, to better support an international audience, so their values won’t be stable under different locales.
   Although Go’s standard library doesn’t offer localized error strings, there is a convincing argument that they are part of the user interface and should respect the user’s locale. Unfortunately, the proposal in x/text: localization support #12750 doesn’t mention how errors will get localized, especially when it comes to sentinel errors. In addition, library authors will want to test if these errors strings were localized correctly, so I’m not sure if there isn’t an argument to test a subset of error strings for every supported locale.

4. We explicitly want to define error strings as opaque, and developers should be strongly encouraged to use errors.Is and errors.As.
   I guess I can’t argue against that. This implies that developers using only standard errors should be encouraged to write tests of the form:

   func TestFunc(t *testing.T) {
   	wantErr := fs.ErrNotExist
   	err := my.Func()
   	if !errors.Is(err, wantErr) {
   		t.Fatalf("err %v is not %v", err, wantErr)
   	}
   }

[jba]

@sflaw:

@jba The proposed errors.Like is meant to make it easier for tests to check the wrapped error, in addition to the error string:

1. When you say that comparing error strings is a last resort, are you suggesting that the common idiom of fmt.Errorf("additional context: %w", err) should be tested in a different way?

Yes, it should be tested with errors.Is or errors.As.

If you want to test that the error string has the text "additional context", then you would use strings.Contains, not ==.

2. I acknowledge that there are special cases where it might be impossible for the test author to control randomness in the test setup, but errors.Like doesn’t preclude a custom test. Are you worried that errors.Like will discourage custom tests?

Almost nothing in the standard library precludes alternatives, but everything in it is supposed to embody best practices. Comparing errors strings for equality is not a best practice.

3. Are you concerned that people will use errors.Like outside of tests? I am also concerned about that, which is why I suggested that this might belong in an errors/errorstest package.

I missed that. That part of the design I agree with.

[neild]

why is it a bad thing for error strings to be part of a package’s API?

• It should be possible to improve error strings (by correcting typos, clarifying, etc.) without breaking programs.
• Error strings are outside the Go type system, and tests against them are inherently fragile. The compiler cannot tell you that strings.Contains(err, "permision denied") contains a typo, but it can for errors.Is(err, ErrPermissionDenied).
• There is no good way to expose to the user what error strings are intended as a stable API surface and which ones are not. In contrast, providing an exported error API (e.g., an ErrPermissionDenied sentinel) makes the API surface clear.
• Treating all error strings as a stable API is infeasible; changes to the internals of a package can frequently change what error text makes sense.

Ad hoc error text matching is simply not a good API. There are much better alternatives.

If a library author changes their error string, they will be making a backwards-incompatible change.

This is not the position taken for the Go standard library. We do not consider changes to error text to be backwards-incompatible changes, and therefore can improve errors without violating the Go compatibility promise. (We have on occasion reverted error text changes when the practical impact was large, although arguably doing so sets a bad precedent.)

[sfllaw]

@jba The proposed errors.Like is meant to make it easier for tests to check the wrapped error, in addition to the error string:

1. When you say that comparing error strings is a last resort, are you suggesting that the common idiom of fmt.Errorf("additional context: %w", err) should be tested in a different way?

Yes, it should be tested with errors.Is or errors.As.

If you want to test that the error string has the text "additional context", then you would use strings.Contains, not ==.

@jba I think what you’re saying is that we’d be encouraging people to write brittle tests? Given this example:

func TestFunc(t *testing.T) {
	wantErr := fs.ErrNotExist
	wantErrText := "additional context: "
	err := my.Func()
	if !errors.Is(err, wantErr) {
		t.Fatalf("err %v is not %v", err, wantErr)
	}
	if err != nil && !strings.Contains(err.Error(), wantErrText) {
		t.Fatalf("err %v does not contain %v", err, wantErrText)
	}
}

It seems like you’re arguing that err might have other context introduced and the test shouldn’t fail?

I think that’s a legitimate concern that I haven’t encountered in our tests, but I definitely acknowledge that this could happen. Would you have similar objections if errorstest.Like were less strict about wrapped errors?

[sfllaw]

@neild I’m fine with anything I haven’t quoted below:

• It should be possible to improve error strings (by correcting typos, clarifying, etc.) without breaking programs.
  …
• Treating all error strings as a stable API is infeasible; changes to the internals of a package can frequently change what error text makes sense.

… (We have on occasion reverted error text changes when the practical impact was large, although arguably doing so sets a bad precedent.)

Accidentally breaking programs when changing error strings is the motivation for this proposal! We have been burned by an innocent change to error strings because we had downstream systems relying on that text.

We noticed that many of our tests had poor coverage over error values because testing errors is clumsy and unpleasant. The proposed errorstest.Like, or some better alternative, is meant to encourage assertions on error values and discourage arbitrary changes.

I suppose there is this fundamental disconnect between:

• the current state of errors, which is that downstreams currently rely on error string comparisons
• the world we’d prefer, which is that downstreams rely on error sentinels or error structs.

[sfllaw]

Perhaps this proposal is solving the wrong problem? Perhaps the goal should be to encourage an idiomatic way of using errors? If that’s the case, I would be happy to submit an alternate proposal where we do one or more of the following:

1. Add sections to CodeReviewComments that recommend:
   1. For exporting error wrapping with sensible idioms for testing them using errors.Is and errors.As.
   2. Against matching or parsing errors with string comparisons.
2. Augment go vet such that it warns against:
   1. error.Error() == "…"
   2. strings.Contains(error.Error(), "…")
   3. strings.HasPrefix(error.Error(), "…")
   4. strings.HasSuffix(error.Error(), "…")
   5. regexp.MatchString(pat, error.Error())
   6. regexp.Regexp.MatchString(error.Error())
   7. regexp.Regexp.Find*String(error.Error())
3. Change errors.New and fmt.Errorf such that they introduce random word joiners when:
   1. running go test -errfuzz
   2. running go test
   3. all the time

Obviously, some of these recommendations are outlandish, but I’ve included them for completeness.

Each of these changes would shift the burden of error string compatibility from upstream library authors to downstream library consumers. Are people more sympathetic to this potential future?

[neild]

Add sections to CodeReviewComments

This seems reasonable to me.

Augment go vet

Checks in go vet should have few to no false positives: A reported error should almost always indicate a real bug. I don't think these checks would meet the bar.

Change errors.New and fmt.Errorf

If we were designing the standard library from scratch today, I would argue for introducing randomness in error text when running tests. Alas, it is much too late now; that change would be a clear violation of the Go compatibility guarantee. It might be feasible to have a build tag that randomizes the text of every error for testing, but that would rely on users knowing about and testing with it set.

Nothing stops you from doing something in your own packages, of course. For example, the google.golang.org/protobuf module generates error text with deliberately-introduced randomness.

[sfllaw]

@neild:

Augment go vet

Checks in go vet should have few to no false positives: A reported error should almost always indicate a real bug. I don't think these checks would meet the bar.

By our definition of error strings, are we sure that comparing error.Error() to a string isn’t actually a subtle bug? If Go itself can change the error strings between minor releases, then this is a potential landmine for any developer.

For an even weaker suggestion, perhaps we add stringcompareerrors to x/tools/go/analysis/passes, which defines an Analyzer that checks for the use of comparisons with error strings? There’s a similar package, deepequalerrors that isn’t shipped with go vet, but is used by metalinters.

Change errors.New and fmt.Errorf

If we were designing the standard library from scratch today, I would argue for introducing randomness in error text when running tests. Alas, it is much too late now; that change would be a clear violation of the Go compatibility guarantee. It might be feasible to have a build tag that randomizes the text of every error for testing, but that would rely on users knowing about and testing with it set.

I’m being a little contrarian here, but here’s a thought experiment:

If we “do not consider changes to error text to be backwards-incompatible changes, and therefore can improve errors without violating the Go compatibility promise”, then surely it follows that we could break error string comparisons as long as the output of errors.New and fmt.Errorf look the same?

Build flags like -race or -shuffle are being added to CI, so it seems reasonable to suggest an -errfuzz flag? If we used a build flag, we could change errors.New to read:

// New returns an error that formats as the given text.
// Each call to New returns a distinct error value even if the text is identical.
func New(text string) error {
	return &errorString{fuzz(text)}
}

const wordJoiner = "\u2060"
var fuzzer = strings.NewReplacer("", wordJoiner) // TODO: add randomness

func fuzz(text string) string {
	if runtime.errfuzzenabled {
		return fuzzer.Replace(text)
	}
	return text
}

[neild]

For an even weaker suggestion, perhaps we add stringcompareerrors to x/tools/go/analysis/passes, which defines an Analyzer that checks for the use of comparisons with error strings?

Seems reasonable to me, although I'm not the decider for that package.

If we “do not consider changes to error text to be backwards-incompatible changes, and therefore can improve errors without violating the Go compatibility promise”, then surely it follows that we could break error string comparisons as long as the output of errors.New and fmt.Errorf look the same?

We could technically break string comparisons against errors generated by the standard library. However, errors.New and fmt.Errorf are specified in a way that precludes changing the text of the errors they return.

Additionally, while changing the text of every error in the stdlib is technically within the bounds of the compatibility promise, I think it would be difficult to justify the cost.

[sfllaw]

If we “do not consider changes to error text to be backwards-incompatible changes, and therefore can improve errors without violating the Go compatibility promise”, then surely it follows that we could break error string comparisons as long as the output of errors.New and fmt.Errorf look the same?

We could technically break string comparisons against errors generated by the standard library. However, errors.New and fmt.Errorf are specified in a way that precludes changing the text of the errors they return.

If this change only applied to go test, while protected behind an -errfuzz flag, is the cost really that high? It seems unlikely that anyone would want to build this into their actual binaries, so I would explicitly avoid having this flag for build or run commands.

The advantage to fuzzing error strings, instead of writing some kind of analyzer, is that there would be no false positives.

As an aside, I tried to think of a way to add error string fuzzing outside the standard library, but I don’t think that would work reliably.

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[sfllaw]

@rsc Agreed. It looks like we will take another path.

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group