-
Notifications
You must be signed in to change notification settings - Fork 1.6k
assert: add NotZeroThenSetZero
and NotNilThenSetNil
#1440
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
Conversation
|
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 do not plan to merge this.
But anyway I choose to give you feedback that might be valuable for another contribution
Also you didn't run "go generate ./.. ".
@@ -24,7 +24,6 @@ jobs: | |||
strategy: | |||
matrix: | |||
go_version: | |||
- "1.17" |
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.
Don't change this. This is out of scope.
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 changed the minimum version to be >=1.18 in discussion with Boyan Soubachov in this PR: #1379 (comment). Maybe that was a bad change?
NotZeroThenSetZero
and NotNilThenSetNil
I'm sorry you feel you don't want to merge this.
Yes, I noticed that the cannot run now. Seems to be because of generics as well. Should I close the PR? |
From your description of the use case: clientResponse := someApiRequest(request)
assert.NotZeroThenSetZero(t, &clientResponse.CreateTime)
assert.NotZeroThenSetZero(t, &clientResponse.UpdateTime)
assert.Equal(t, expected, clientResponse) This can be rewritten as: clientResponse := someApiRequest(request)
assert.NotZero(t, clientResponse.CreateTime)
assert.NotZero(t, clientResponse.UpdateTime)
var zeroTime time.Time
clientResponse.CreateTime = zeroTime
clientResponse.UpdateTime = zeroTime
assert.Equal(t, expected, clientResponse) And I think that is would be more clear that the proposed API. I think that if you want than feature it is best to copy your implementation in the test package where you use it. That way, the source is immediately available to the reader who may not be familiar with the full |
About the code generation not working with generics code, this is a problem that we must fix before introducing any generics-based API. |
I have used similar versions in my own code for many years. It helps to do assertions of API responses, like this:
Could resolve the issue brought up in #1432.