assert.ErrorAs: log target type

[craig65535]

Summary

When an assertion fails in assert.ErrorAs, log the target error type rather than its value.

Changes

Passes the target error type, rather than the target itself, to the Fail() call in assert.ErrorAs. Also, modifies buildErrorChainString to support go 1.20 error wrapping, and to include the error type in its output when called by assert.ErrorAs.

Motivation

The existing error message was confusing - see #1343

Related issues

Closes #1343

[brackendawson]

I think as well as printing the type of target after "expected:", we should print the type of every error in chain after "in chain:" otherwise it's still confusing.

Since Go 1.20 this is how you have to get every error in the chain:

func unwrapAll(err error) (errs []error) {
	errs = append(errs, err)
	for {
		switch x := err.(type) {
		case interface{ Unwrap() error }:
			err = x.Unwrap()
			if err == nil {
				return
			}
			errs = append(errs, err)
		case interface{ Unwrap() []error }:
			for _, err := range x.Unwrap() {
				errs = append(errs, unwrapAll(err)...)
			}
			return
		default:
			return
		}
	}
}

[craig65535]

@brackendawson Updated as suggested. PTAL

[craig65535]

@brackendawson Thank you for the review. It looks like I still need an approval before this can be merged.

[brackendawson]

@brackendawson Thank you for the review. It looks like I still need an approval before this can be merged.

I wish mine counted for a ✅ but it doesn't. Maintainers drop by from time to time, when one does I think this PR stands a reasonable chance of a merge now.

[dolmen]

Missing test case.

[craig65535]

@dolmen This modifies the error string, not behaviour, and I'm not sure there are any existing test cases for error strings. Do you want me to add a test case anyway?

[craig65535]

@dolmen Actually it should be straightforward for me to add tests for unwrapAll and buildErrorChainString. I can do that now.

[dolmen]

We don't want tests that test details of the actual implementation. Good tests must only use the public API of testify.

[craig65535]

We don't want tests that test details of the actual implementation. Good tests must only use the public API of testify.

To be clear, you do want these new tests to check the format and contents of the error string returned for ErrorAs? I don't think other tests in this repo are doing anything like that.

[dolmen]

@craig65535 testify is now heavily used in the Go ecosystem. We have reached a point where we must stabilize things: slow down API growth, improve test coverage, add more regression tests, ease maintenance work, reduce load for maintainers.

So, yes, I think that because this PR is about the log output, it is important to have a regression test to ensure further changes will not break the output you designed.

[craig65535]

@dolmen PTAL

[craig65535]

@dolmen Wanted your thoughts on the tests - am I on the right track? Thank you

[dolmen]

Please do not merge master into your branch.

Instead:

git remote update
git rebase origin/master
git push -f

[dolmen]

I think that your changes to improve testing are very good.

However those changes are quite intrusive in the context of this PR.

Could you post the testing refactor as a separate MR that I will merge first? Then you will rebase this PR that will have smaller changes only related to ErrorAs.

[craig65535]

@dolmen I added the tests in #1435. I'll also split out a new PR for the go 1.20 error wrapping handling.

[craig65535]

@dolmen Updated - PTAL

[MovieStoreGuy]

Do you mind explaining this one? I believe this should only be the default cause since you're going to have recursive definitions of error if you explore each value in the slice.

[craig65535]

This basically "flattens" the provided error (err) into a slice so we can access the type and Error() string of each constituent error. We start by appending err to the slice because it itself has a type and Error() string.

errors.As failing for a given error means that the target error type is not present in any of these constituent errors. So, we need to exhaustively log all of the types in the error message.

I'm not 100% sure if this answers your question.

[MovieStoreGuy]

Aaa, so the usecase I am thinking of is the following:

errs := unwrap(errors.Join(errors.New("a"), errors.New("b"))

This would mean the resulting values would be:

[]error(error([]{"a", "b"}, "a", "b")

Which I don't believe you want to repeat those values

[craig65535]

In this case, I do want to repeat those values - I want 3 lines in the error message, and 3 types.

[MovieStoreGuy]

Sorry for back and forth on this, I just have a question for you regarding an implementation detail but seems okay thus far.

It is also middle of the night for me so I will be checking back in 8hrs or so.

[brackendawson]

I still approve.

[brackendawson]

This can be simpler if all the switch cases recurse, but the above implementation is fine.

Suggested change

	for {
	switch x := err.(type) {
	case interface{ Unwrap() error }:
	err = x.Unwrap()
	if err == nil {
	return
	}
	errs = append(errs, err)
	case interface{ Unwrap() []error }:
	for _, err := range x.Unwrap() {
	errs = append(errs, unwrapAll(err)...)
	}
	return
	default:
	return
	}
	}
	switch x := err.(type) {
	case interface{ Unwrap() error }:
	err = x.Unwrap()
	if err == nil {
	return
	}
	errs = append(errs, unwrapAll(err)...)
	case interface{ Unwrap() []error }:
	for _, err := range x.Unwrap() {
	errs = append(errs, unwrapAll(err)...)
	}
	}
	return

[slonopotamus]

So, what is preventing this from being merged? I've just come across a test failure in assert.ErrorAs and discovered that error message is very far from being helpful because it prints expected error type, but doesn't print actual error types in chain.