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]

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.

[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.

[brackendawson]

@dolmen's most recent request to move the test refactor into another PR has been done and merged. So I think I can in good faith dismiss @dolmen's requested changes on this PR. I still approve.

As it has been so long there is now a merge conflict. @craig65535 or anyone else, if you want to re-base and @ me then I will see it.

[craig65535]

I'll resolve these conflicts and push an update.

[craig65535]

@slonopotamus @brackendawson PTAL

[brackendawson]

Thanks for the diligent review @ccoVeille, it's helpful. And thanks @craig65535 for your continued effort. Let me know what you think about my comment and we'll get this merged.

[brackendawson]

LGTM

@dolmen can you dismiss your review or approve now? It appears I don't have that permission. Your comments were addressed.

[craig65535]

@brackendawson Thanks for the review. If we can't get the old review dismissed in the next week or so, I suppose I could just open a new PR and close this one?

[brackendawson]

@brackendawson Thanks for the review. If we can't get the old review dismissed in the next week or so, I suppose I could just open a new PR and close this one?

Technically yes, but I think it's poor form. My plan is to give @dolmen some time to reply and if we're still stuck I'll ask @matryer if he can give me permissions to dismiss reviews.

[brackendawson]

I'm still pulling at this thread, bear with us.