Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: ErrorIs #208

Merged
merged 4 commits into from
Sep 18, 2021
Merged

assert: ErrorIs #208

merged 4 commits into from
Sep 18, 2021

Conversation

dnephin
Copy link
Member

@dnephin dnephin commented Sep 8, 2020

No description provided.

@dnephin dnephin changed the base branch from master to main October 31, 2020 21:59
@glenjamin
Copy link

Was there a reason this got abandoned? Is it something I could pick up and finish off?

I found myself wanting to do an .Is assertion today, and although it works with .Check()/.Assert() the default failure message isn't great as it doesn't tell you what error you got instead

@dnephin
Copy link
Member Author

dnephin commented Aug 26, 2021

There is https://pkg.go.dev/gotest.tools/v3/assert#ErrorType, but erorrs.Is does compare a bit differently.

I'll run CI again to see what state this is in. From the previous runs it looks like I probably put this on hold because I wasn't too sure what the failure messages should look like. I guess that would probably be the last thing to figure out before getting this merged.

@glenjamin
Copy link

Ah, I've checked the branch out and I see what you mean.

Potentially for Is the useful information is?

  • expected: The message and type (or maybe not the type if it's only an errorString?)
  • source: The message and its whole chain of wrapped types?

For comparing errors using errors.Is.
@dnephin dnephin marked this pull request as ready for review September 4, 2021 21:05
Expected strings can be pretty long sometimes.
@dnephin
Copy link
Member Author

dnephin commented Sep 4, 2021

I rebased this again and fixed the tests.

Removing errors.errorString might be nice, but probably ok to keep it for now. Since that type isn't exported the name could change, so excluding it by name might not be reliable.

I guess we could also print all the wrapped types. Right now it's only printing the top-level type along with the name of the symbol and error message.

I marked this as ready for review. If you don't have a need for the full chain of wrapped types I'd be fine to merge this as-is and we could look at adding it later. But if you are interested in adding that to the message I can keep this open for a bit.

Copy link

@glenjamin glenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have a need for the full chain of wrapped types I'd be fine to merge this as-is

Yeah, this was more of a "it'd be cool if..." than a concrete need

assert/cmp/compare_go113.go Outdated Show resolved Hide resolved
@dnephin
Copy link
Member Author

dnephin commented Sep 18, 2021

Thanks for the nudge to get this included, and for giving it a review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants