Discussion: cmp.ErrorIs is not a full replacement for cmp.ErrorType, but is now deprecated

[thaJeztah]

• relates to Deprecate assert.ErrorType #254

I noticed the ErrorType compare was deprecated, and when updating gotest.tools, I found quite some checks in our codebase that depend on it. Looking at ErrorIs() I feel like that's not a full replacement.

While ErrorIs() works to match an exact error (or an error wrapped by another error), it does not provide ways to check for an error type. (I guess some equivalent of errors.As). That said I found the ability to provide a check function also to be very convenient (in Moby, we have the errdefs package, which has functions to match each of the type).

I'm probably able to craft my own way around this, but wondering if there's something missing in gotest's comparers.

For some quick comparison / example:

type customError struct {
	Name  string
	Value []string
}

func (e *customError) Error() string {
	return fmt.Sprintf("invalid name: %s (%s)", e.Name, e.Value)
}

func (e *customError) InvalidParameter() {}

type invalidParameter interface {
	InvalidParameter()
}

func isInvalidParameter(err error) bool {
	_, ok := err.(invalidParameter)
	return ok
}

func TestErrorIsVsErrorType(t *testing.T) {
	var expectedType *customError
	expectedErr := &customError{Name: "hello", Value: []string{"world"}}

	actualErr := &customError{Name: "hello", Value: []string{"world"}}
	wrapped := fmt.Errorf("something went wrong: %w", expectedErr)

	if result := ErrorType(actualErr, expectedErr)(); !result.Success() {
		t.Error(result.(StringResult).FailureMessage())
	}
	if result := ErrorType(actualErr, isInvalidParameter)(); !result.Success() {
		t.Error(result.(StringResult).FailureMessage())
	}

	if !errors.As(actualErr, &expectedType) {
		t.Error("type compare failed")
	}

	if result := ErrorIs(wrapped, expectedErr)(); !result.Success() {
		t.Error(result.(templatedResult).FailureMessage(nil))
	}
	if result := ErrorIs(actualErr, expectedErr)(); !result.Success() {
		t.Error(result.(templatedResult).FailureMessage(nil))
	}
}

[thaJeztah]

👋 @dnephin happy to hear your thoughts (or perhaps I overlooked an approach 😂)

[dnephin]

Hi @thaJeztah ! Thanks for raising this issue. The way I would generally check error types would be:

// using func(err) bool
assert.Assert(t, IsNotFound(err), "err=%v", err

// using errors.As
typedErr, ok := errors.As(t, err)
assert.Assert(t, ok, "err (%T) = %v", err, err)

But I do realize these forms are a bit difficult to discover right now because the examples are only in the package godoc, and not in the godoc for assert.Assert. I think we can keep ErrorType until we have a better way to do this. If we do make breaking changes in a v4 I think a command to migrate source to the new interface would be pretty important. That command could take care of switching ErrorType to one of these forms.

I've been thinking a bit about how to reduce the interface of the assert package even further, and make usage like the examples above easier and more obvious. One idea that I've been playing with would be something like this:

// True fails the test with [t.FailNow] if the value of expr is false.
// True uses the Go source and comments in the test function to print a
// helpful failure message. Extra values can be passed to provide
// the full context for the message.
// If True fails and there are no extra values, it will rewrite the test
// Go source to add any variables that were found in the Go source that
// created expr.
//
// Error is a type (error.As)
//
//	err := ScheduleWork()
//	errV := &CustomErrorType{}
//	assert.True(t, errors.As(err, &errV), err)
//	// main_test.go:17: We wanted err := DoSomeWork() to be &CustomErrorType,
//	// but it was fs.PathError with a value of: file not found.
//
// Error is a value
//
//	fi, err := os.Stat("./output")
//	assert.True(t, errors.Is(err, fs.ErrNotfound), err)
//	// main_test.go:23: We wanted err := os.Stat("./output") to be fs.ErrNotFound,
//	// but it was nil.
//
// Error contains a message
//
//	err := GetAccount()
//	contains := "account id"
//	assert.True(t, err != nil && strings.Contains(err, contains), err)
//	// main_test.go:28: We wanted err := GetAccount() to contain
//	// "account id", but it was "connection failed".
//
// Map contains a value
//
//	v, ok := settings["max"]
//	assert.True(t, ok, settings)
//	// main_test.go:32: We wanted settings to have the key "max", but it did not.
//	// settings=map[min: 34].
//
// Fail with t.Fail instead of t.FailNow
//
//	assert.True(assert.AndContinue(t), false)
func True(t TestingT, expr bool, values ...any) bool { ... }

If the function is able to rewrite the test to add the extra values arguments, I think it becomes pretty
easy to write these. Write the part you care about first:

assert.True(t, IsNotFound(err))
// main_test.go:45: We wanted IsNotFound(err) to be true, but it was false.
//     Successfully updated the test to include the value of err! Run the test again
//     for a better error message.

Run the test once to show it is testing what you expect and fails when the behaviour isn't implement (generally good practice anyway), and then end up with:

assert.True(t, IsNotFound(err), err)
// main_test.go:45: We wanted IsNotFound(err) to be true, but err was *errors.errorString with value: "something else"

I'm really hoping to be able to lean into a much smaller interface (maybe even just 4 functions True, Nil, Empty, and Equal) while being able to provide even better failure messages for 90% of the most common assertions.

[thaJeztah]

Oh boy, for some reason I missed your reply 🙈 🙈 🙈, thanks!!

The way I would generally check error types would be:

Yes, I was considering variations thereof. I guess we were lucky that the is.ErrorType was a great fit for our errdefs package, which allowed us to use a nice one-liner to check the type, AND produce a nice output on failures (without any effort other than using is.ErrorType).

To be really honest; errors.As() is quite awkward to use in many situations (the "pointer" vs "non-pointer" vs "pointer-to-pointer" always makes my head spin, and it's really easy to pick the wrong variant).

But I do realize these forms are a bit difficult to discover right now because the examples are only in the package godoc, and not in the godoc for assert.Assert.

Yes, big nod there, and also something I ("we") should look at contributing. gotest.tools really became an indispensable tool for us. It for sure is less known than Testify, but I like the simplicity of many things.

I've been thinking a bit about how to reduce the interface of the assert package even further

Need to take a deeper look at your proposal. I definitely like simplifying things further. That said; discoverability is important, and yes, documentation may be key there, so more examples would be a good thing to have.

Some of those examples could be basic principles, such as "when to use assert.Check vs other options, such as assert.NilError". assert.NilError is easier to discover, but also the "wrong" option to choose in many cases.