-
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: introducing no field is empty assertion function #1591
base: master
Are you sure you want to change the base?
assert: introducing no field is empty assertion function #1591
Conversation
Please let me know if I should also create an issue alongside this PR as it is for proposed functionality. EDIT: I created an issue for this as well #1601. |
assert/assertions.go
Outdated
@@ -2107,3 +2107,38 @@ func buildErrorChainString(err error) string { | |||
} | |||
return chain | |||
} | |||
|
|||
// NoFieldIsEmpty asserts that object, which must be a struct or eventually reference to one, has no empty exported fields. |
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.
What does "empty" mean? It's not in the language spec. From reading the code it seems to be "zero values for most types or with length of zero for some of the container types".
This assertion's behaviour needs to be clearly documented. I can't review it as it is because I can't tell what empty is supposed to mean. I think it's supposed to mean unpopulated struct fields? If that is the case then it doesn't work, see discussion in #1601.
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 am happy to have more explicit documentation.
One option would be to provide a godoc link to the current assert.Empty function e.g.
NoFieldIsEmpty asserts that object, which must be a struct or eventually reference to one, has no exported field with a value that is empty (following the definition of empty used in [Empty]).
Another option is to include the same definition used in assert.Empty in the documentation for this function e.g.
NoFieldIsEmpty asserts that object, which must be a struct or eventually reference to one, has no exported field with a value that is empty I.e. nil, "", false, 0 or either a slice or a channel with len == 0.
Do find either these clear enough?
P.S. I am aware that "empty" is not part of the go spec. My intent was to follow a definition already used by this library (that I have found useful) rather than invent something myself.
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 godoc link would be sufficient.
621db53
to
43a8b0a
Compare
Summary
Proposal to introduce an assertion that checks that no (exported) fields are empty in a struct.
Changes
NoFieldIsEmpty
is introduced into into theassert
package. This function fails the test and returns false if any of the fields in the inputed struct (or reference to struct) are empty, aligning with pre-existing definition of empty.NoFieldIsEmpty
function were added.go generate ./...
was run.Motivation
I found myself repeatedly rewriting similar code in tests across different repositories in an effort to ensure that tests do not become out-of-date and no longer align with their original intention.
An example of this is writing tests that check that all fields in a struct can be stored and then loaded. Consider the test:
This test is claiming to check that all fields can be stored and loaded but it not enforcing it. If the entity was not correctly populated initially or if new fields where added to the Entity and the test was not updated then the test would not be align with its stated purpose.
In this particular case the assertion could be used as a pre-condition to ensure we always start with all fields containing data
or as check at the end of test to ensure all fields where populated
I have found this assertion useful for when using tests that require populated structs including:
Related issues
None.
Additional comments