-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: deprecate and move exported internal comparison functions #1431
Conversation
These functions should not have been exported, but we must maintain the compatibility promise until v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are valid use cases for using ObjectsAreEqual
as shown by the usage in the mock
package. So the deprecation message should provide alternatives for that use case too.
assert/assertions.go
Outdated
@@ -19,6 +19,7 @@ import ( | |||
|
|||
"github.com/davecgh/go-spew/spew" | |||
"github.com/pmezard/go-difflib/difflib" | |||
"github.com/stretchr/testify/internal/check" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the import in a separate block below to separate internal dependencies and external ones.
assert/assertions.go
Outdated
@@ -54,116 +55,28 @@ type Comparison func() (success bool) | |||
|
|||
// ObjectsAreEqual determines if two objects are considered equal. | |||
// | |||
// This function does no assertion of any kind. | |||
// Deprecated: This function does no assertion of any kind. Use Equal instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectsAreEqual
might have been used not as a mistake (vs assertions) but as a conveniency (to avoid reflect.Equal
or a dependency on goolgle/go-cmp). (I admit to have done that myself) so the deprecation message should also handle that case.
Also, use godoc links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in #1279 we established that testify is not a utility library, I think that includes our docs.
Godoc links added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testify should not have been used as a utility library, but the fact is it has been used like that, at least in testing frameworks/libraries.
} | ||
|
||
// ObjectsExportedFieldsAreEqual determines if the exported (public) fields of two objects are | ||
// considered equal. This comparison of only exported fields is applied recursively to nested data | ||
// structures. | ||
// | ||
// This function does no assertion of any kind. | ||
// Deprecated: This function does no assertion of any kind. Use | ||
// EqualExportedValues instead. | ||
func ObjectsExportedFieldsAreEqual(expected, actual interface{}) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deprecation message must suggest a full alternative to ease migrating away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature shares the same arguments, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #1279 you hinted that people should use go-cmp instead. However the testify functions have a particular definition of equality as nil
values in the expected value mean ignore this field.
I think that we have the responsibility of helping users to find that path if we are deprecating the functions.
So, do we have a path forward for all use cases? I think we must provide that detailed information in the documentation. If not, we will get loads of support questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a specific recommendation, such as go-cmp, is something we'd have to maintain though. What if go-cmp declares itself EOL tomorrow? I'd be happy to clarify that one should use EqualExportedValues if you intend to make an assertion and you should use another module if you intend to make a comparison. But I'm not happy to make any specific recommendation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have no safe migration to suggest we must not deprecate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing functions which make no assertion from Assert, so that people using testify to test don't have hidden bugs, is more important than providing a forward path for people using testify as a utility library. The 4th paragraph of Philosophy supports this.
We're deprecaing the function, not removing it. It will exist in testify v1 forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some development teams have policies which disallow using deprecated functions/methods. If we don't provide a path forward some users might be tempted to stop upgrading Testify to avoid the deprecation warning instead of dropping the use of ObjectsAreEquals
.
I don't think that this change is worth a fight with our users.
assert/assertions_test.go
Outdated
@@ -2919,7 +2702,7 @@ func TestErrorAs(t *testing.T) { | |||
|
|||
func TestIsNil(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should also move to internal/check
.
It's the existing convention in this project as seen in mock.go
Because it tests a function in this package.
1d937a8
to
1fffab0
Compare
@brackendawson I would like to merge #1436 first and wait for the next minor release to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think CallerInfo is another function that should receive this treatment.
OK, I concede, this cannot progress. The refactor is not beneficial unless it leads to the eventual removal of those functions. |
Summary
These functions should not have been exported, but we must maintain the compatibility promise until v2.
Changes
Motivation
These functions look like assertions but are not, they can never cause any test to fail.
Related issues
Closes #1180