-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
testifylint: enable go-require, float-compare and require-error rules #8027
base: main
Are you sure you want to change the base?
Conversation
/kind changelog-not-required |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8027 +/- ##
=======================================
Coverage 59.06% 59.06%
=======================================
Files 364 364
Lines 30322 30322
=======================================
Hits 17909 17909
Misses 10970 10970
Partials 1443 1443 ☔ View full report in Codecov by Sentry. |
db63cc0
to
a14de0f
Compare
a14de0f
to
70d1eeb
Compare
@@ -125,9 +126,9 @@ func TestWaitPVCBound(t *testing.T) { | |||
pv, err := WaitPVCBound(context.Background(), kubeClient.CoreV1(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, time.Millisecond) | |||
|
|||
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.
@blackpiglet,
This condition seems odd, should it be this instead?
if err != nil { | |
if test.err != "" { |
That's out of the scope of this PR but might worth a review. It's as if the assertions are done depending on the result and not there expected result.
And this happens in several places in this file.
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.
Thanks. I agree with you. Although both should work, the code should check the test.err
instead.
b0f8cfb
to
3766c1c
Compare
@Lyndon-Li , |
64fbb21
to
ba3d05a
Compare
3431f73
to
1d672e4
Compare
144cd7f
to
5f16c8a
Compare
I have squashed and rebased all my commit now. Please review this when you find the time. |
Signed-off-by: Matthieu MOREL <[email protected]>
5f16c8a
to
da90147
Compare
if test.expectedError != nil { | ||
assert.EqualError(t, err, test.expectedError.Error()) | ||
return | ||
require.EqualError(t, err, test.expectedError.Error()) | ||
} else { | ||
require.NoError(t, err) |
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 feel like require.EqualError should be sufficient here without if/else as long as you don't call .Error()
Comparing error type to error type should be fine IMO.
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 believe it’s out of scope of this PR. This only apply best practices while keeping actual tests working. Maybe you can do a PR following your suggestion in the same time ?
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.
ok.
Thank you for contributing to Velero!
Please add a summary of your change
Enable and fixes go-require and require-error rules of testifylint linter.
Also update golangci-lint version
Does your change fix a particular issue?
Fixes #(issue)
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.