What problem are you facing?

Long ago we adopted a convention of defining all of our error strings as constants at the top of the source file where they're used. For example:

// Error strings.
const (
  errFmtFetchFailed = "cannot fetch %q"
  errFmtStoreFailed = "cannot store %q"
)

// many lines later...

func UpdateThing(t Thing) error {
  if err := remote.Fetch(t); err != nil {
    return errors.Wrapf(err, errFmtFetchFailed, t)
  }
  return errors.Wrapf(store.Update(t), errFmtStoreFailed, t)
}

This approach has a couple of issues:

• It adds a layer of indirection when searching the codebase for an error string.
• Our linter doesn't seem to notice unused error string constants, so we end up with unused ones. (Actually, it does)
• As code moves from one file to another it's easy to forget to move the error string constants accordingly.
• It's awkward to use errors.Errorf or errors.Wrapf because the format string is defined far away from the format arguments.

I believe we adopted this pattern to make it easier to (unit) test that we return the expected error in exceptional cases. We do this using https://pkg.go.dev/github.com/crossplane/crossplane-runtime@v1.13.0/pkg/test#EquateErrors, which compares two errors by ensuring they are of the same type, and produce the same error string.

How could Crossplane help solve your problem?

We could agree to unwind this constant error pattern, and just write error strings "inline" wherever we wrap or produce new errors. It may be possible to do a sweep through the codebase and inline all the error constants using find and replace. We should probably also record this decision in our contributing guide.

If we do this, I'd like to understand how we'll compare errors in our unit tests. I can think of a couple of options:

1. Keep comparing mostly by string, and update error strings in N places whenever we want to change them.
2. Compare only that an error is or is not nil.
3. Use https://pkg.go.dev/github.com/google/go-cmp@v0.5.9/cmp/cmpopts#EquateErrors to compare errors using errors.Is

My preference is option 3.

In my experience testing error strings has been less fragile than you might expect - we're typically only testing the "wrapper" string, which we control. I don't think it's very important to test though. What I do think is important is to test that we return the right (i.e. expected) error. I don't think testing for only a non-nil error is enough.

Often we'll be testing a pretty branchy bit of code that could return errors in several different places. We tend to test error paths by injecting dependencies that will return an error when called. When you're only testing for a non-nil error it's easy to write a unit test that appears to test the error case you intend it to, but is actually never reaching that error case because it's hitting another one.

I believe using errors.Is should allow us to ensure we're returning the error we want without needing to test on string. We'd do this by injecting a dependency that returned a particular sentinel error (defined only in the unit test function), then test that the returned error wrapped that sentinel error.