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

TestErrorIs/TestNotErrorIs: check error message contents #1435

Merged
merged 4 commits into from Oct 31, 2023

Conversation

craig65535
Copy link
Contributor

This extends the TestErrorIs/TestNotErrorIs tests to check not only the result, but the error message that is logged.

@dolmen requested that I split this out from #1345, which improves the error message returned by ErrorAs.

@craig65535 craig65535 changed the title TestErrorIs/TestNotErrorIs: check error contents TestErrorIs/TestNotErrorIs: check error message contents Jul 27, 2023
assert/assertions_test.go Show resolved Hide resolved
assert/assertions_test.go Show resolved Hide resolved
assert/assertions_test.go Outdated Show resolved Hide resolved
assert/assertions_test.go Outdated Show resolved Hide resolved
@craig65535 craig65535 force-pushed the checkResultAndErrMsg branch 2 times, most recently from b870369 to 4773e1b Compare August 1, 2023 19:27
@dolmen dolmen added pkg-assert Change related to package testify/assert internal/refactor Refactor internals with no external visible changes internal/testing Changes purely related to the testify testsuites and removed internal/refactor Refactor internals with no external visible changes labels Aug 9, 2023
@craig65535
Copy link
Contributor Author

@dolmen Is there more that needs to be done on this PR?

@craig65535
Copy link
Contributor Author

@dolmen Should I just close this, and either start fresh or give up? I don't understand what the hold up is here as I've done everything you asked.

For context, this started as a simple change 8 months ago to make the misleading assert.ErrorAs failure messages more useful. That isn't even in this PR; this one is just the test changes you requested to enable adding tests to the actual ErrorAs PR.

I need to know what kind of change is acceptable to you to improve assert.ErrorAs. I think we can both agree that that is at least a useful and attainable goal, and that leaves the question of how to make it happen.

@dolmen
Copy link
Collaborator

dolmen commented Oct 30, 2023

@craig65535 The hold up is that there are currently 152 waiting pull requests, I'm the only active maintainer and I'm not even allowed to merge my own PRs. I also have a day job and 4 kids.

err: io.EOF,
target: io.EOF,
result: false,
resultErrMsg: "Target error should not be in err chain:\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

For improved readability, split the line:

Suggested change
resultErrMsg: "Target error should not be in err chain:\n" +
resultErrMsg: ""+
"Target error should not be in err chain:\n" +

@craig65535
Copy link
Contributor Author

The hold up is that there are currently 152 waiting pull requests, I'm the only active maintainer and I'm not even allowed to merge my own PRs. I also have a day job and 4 kids.

@dolmen I understand and I do appreciate the time you put into this project. But at the same time I can see a pattern where I think I've addressed everything, and then a few weeks later find out there's one more small change to make, and rinse and repeat.

I made the readability change you suggested. PTAL

@dolmen dolmen merged commit 331c520 into stretchr:master Oct 31, 2023
7 checks passed
@dolmen
Copy link
Collaborator

dolmen commented Oct 31, 2023

@craig65535 Could you review my own PRs, in hope one other co-maintainer might see them?

Just be aware that as a non-maintainer you are privileged: you just need me to approve and merge. While myself I need another maintainer to approve my MRs, but nobody else is active.

@craig65535 craig65535 deleted the checkResultAndErrMsg branch October 31, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal/testing Changes purely related to the testify testsuites pkg-assert Change related to package testify/assert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants