-
Notifications
You must be signed in to change notification settings - Fork 820
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
Added missing FailNow calls to sdkserver unit tests #1659
Conversation
Build Succeeded 👏 Build Id: 81373876-db63-4eee-bce6-0fdc735e4d0c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
pkg/sdkserver/sdkserver_test.go
Outdated
@@ -333,7 +333,9 @@ func TestSidecarUpdateState(t *testing.T) { | |||
t.Run(k, func(t *testing.T) { | |||
m := agtesting.NewMocks() | |||
sc, err := defaultSidecar(m) | |||
assert.Nil(t, err) | |||
if err != nil { |
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 this should be wrapped in a helper function.
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.
It's just a matter of taste. I prefer the current style because if we introduce a function we need to copy it in every test file and when you read code you need to dig into that additional function to check what happens there. I prefer to keep things as simple as possible. I believe that function will become more and more complex in time with additional logic, logging parameters, and so on. I would like to avoid that.
Other opinions?
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.
As another option, you can use the package require
from testify library.
Package require
implements the same assertions as the assert
package but stops test execution when a test fails.
I found function NoError very useful for such cases when we want all preconditions to be completed fully or fail the test.
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.
@oleg-balunenko good suggestion. Actually we can either continue following if err != nil { FailNow() }
pattern or use another package to avoid these checks. I find assert methods descriptions a bit confusing, honestly.
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.
No issues with pulling in require
on my end - a little less noise in the code when wanting to fail fast.
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.
Yes, let us use require
, I also have found it, while investigating alternatives, and by the way it would make code more compact.
Fixing the link mentioned above
https://pkg.go.dev/github.com/stretchr/testify/require?tab=doc
7f88046
to
4b4dbb1
Compare
Build Failed 😱 Build Id: b905a57d-bdda-4b08-b738-39f42b60d898 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
4b4dbb1
to
78a41e3
Compare
Build Failed 😱 Build Id: bfbeea48-a04d-4fbc-a5a1-f564d21995a4 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
78a41e3
to
eb4301f
Compare
Build Failed 😱 Build Id: 228a3078-dd3c-4404-9820-c567c5c97d11 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
use NoError fix added require to vendor
eb4301f
to
6146d8d
Compare
Build Succeeded 👏 Build Id: e6271cf8-dc94-401c-bcbc-dfc469d3c8f3 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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 like this version it is more readable now. 3 times more readable to be accurate.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akremsa, aLekSer The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build Succeeded 👏 Build Id: c4a8060c-5b51-4831-9a05-4c3cacd38a64 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
) * added missing FailNow calls to sdkserver unit tests * use require package to abort a test use NoError fix added require to vendor Co-authored-by: Alexander Apalikov <[email protected]>
What type of PR is this?
What this PR does / Why we need it:
There are places where in case of an error nil pointer exception will happen because assert.Fail() do not abort test execution. I followed the approach that we have discussed previously.
Special notes for your reviewer:
I've noticed that NewSDKServer function has return parameters
*SDKServer, error
, but actually it never returns an error. Why? It producesif err != nil
checks where an error won't happen.