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

Update documentation for the Error function in assert or require package #1675

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

architagr
Copy link

Summary

Incorrect assert.Error() docs

Changes

Removing the if statement and the Equal call from the example.

Motivation

Since we already have EqualError, the documentation is trying to use assert.Equal to assert the error object's content.

Related issues

Closes #1609

@architagr architagr changed the title Update documentation for the Error function in assert or require package #1609 Update documentation for the Error function in assert or require package Nov 2, 2024
Copy link
Collaborator

@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

The change now hides that Error returns a boolean that can be used for further checks.

I think that we must keep the if statement around in the usage example.

@architagr
Copy link
Author

The change now hides that Error returns a boolean that can be used for further checks.

I think that we must keep the if statement around in the usage example.

This change is in accordance with the below conversation by @brackendawson

#1609 (comment)

Comment on lines +1594 to +1595
// actualObj, err := SomeFunction()
// assert.Error(t, err)
Copy link
Contributor

@alexandear alexandear Dec 16, 2024

Choose a reason for hiding this comment

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

Maybe better would be the following:

Suggested change
// actualObj, err := SomeFunction()
// assert.Error(t, err)
// actualObj, err := SomeFunction()
// if assert.Error(t, err) {
// assert.Equal(t, expectedObj, actualObj)
// }

In that case, we don't hide that Equals returns boolean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not good practice IMO, a good test will check the returned actualObj even if the returned error is non-nil.

Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

I approve as is.

This was a poor place to document the assert assertions returning a booleans. It's just one assertion out of many. Perhaps one of the examples at the top of the package should be used.

It's important to remove this from the assert.Error doc because it is also used as the require.Error doc, which does not return a boolean! The only alternative is writing some code in the generator to handle it.

I also think the example is a bad example because the inner assert.Equal can stand in place of the entire example.

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.

Incorrect require.Error() docs
4 participants