Skip to content
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

fix: enable nil-compare rule from testifylint #18689

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Oct 7, 2024

This fixes nil-compare rule from testifylint.

It also replaces testutil.AssertNil and testutil.AssertNotNil by assert.Nil and require.NotNil

The require package is used for NotNil replacement because of the Fatal method being used in testutil version of the method.

@k8s-ci-robot
Copy link

Hi @mmorel-35. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@mmorel-35
Copy link
Contributor Author

/assign @spzala

@mmorel-35
Copy link
Contributor Author

/cc @ivanvc
/cc @jmhbnz

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.78%. Comparing base (133918e) to head (29ae8ac).

Current head 29ae8ac differs from pull request most recent head de2c95d

Please upload reports for the commit de2c95d to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

see 23 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18689      +/-   ##
==========================================
+ Coverage   68.75%   68.78%   +0.02%     
==========================================
  Files         420      420              
  Lines       35494    35494              
==========================================
+ Hits        24404    24413       +9     
- Misses       9655     9660       +5     
+ Partials     1435     1421      -14     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 133918e...de2c95d. Read the comment docs.

@henrybear327
Copy link
Contributor

/ok-to-test

@ahrtr
Copy link
Member

ahrtr commented Oct 7, 2024

Thank you!

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, Matthieu. Can you delete client/pkg/testutil/assert.go in a follow-up pull request?

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, mmorel-35

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Oct 7, 2024

Can you delete client/pkg/testutil/assert.go in a follow-up pull request?

Are you sure that's not used by any project that depends on etcd.
Maybe having it deprecated with a message inviting to migrate to testify would be more appropriate ?

@ivanvc
Copy link
Member

ivanvc commented Oct 7, 2024

Are you sure that's not used by any project that depends on etcd.

Good point. Let's deprecate.

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Oct 8, 2024

Good point. Let's deprecate.

OK, I updated the comments on the client/pkg/testutil/assert.go functions

@mmorel-35 mmorel-35 force-pushed the testifylint/nil-compare branch 2 times, most recently from 2d64a72 to 827654b Compare October 8, 2024 04:36
@mmorel-35
Copy link
Contributor Author

/retest linux-arm64-integration-4-cpu

@k8s-ci-robot
Copy link

@mmorel-35: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pull-etcd-build
  • /test pull-etcd-e2e-386
  • /test pull-etcd-e2e-amd64
  • /test pull-etcd-e2e-arm64
  • /test pull-etcd-govulncheck
  • /test pull-etcd-integration-1-cpu-amd64
  • /test pull-etcd-integration-2-cpu-amd64
  • /test pull-etcd-integration-4-cpu-amd64
  • /test pull-etcd-robustness-amd64
  • /test pull-etcd-robustness-arm64
  • /test pull-etcd-unit-test-386
  • /test pull-etcd-unit-test-amd64
  • /test pull-etcd-unit-test-arm64
  • /test pull-etcd-verify

The following commands are available to trigger optional jobs:

  • /test pull-etcd-integration-1-cpu-arm64
  • /test pull-etcd-integration-2-cpu-arm64
  • /test pull-etcd-integration-4-cpu-arm64

Use /test all to run all jobs.

In response to this:

/retest linux-arm64-integration-4-cpu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ivanvc
Copy link
Member

ivanvc commented Oct 8, 2024

OK, I updated the comments on the client/pkg/testutil/assert.go functions

Sorry, @mmorel-35. I think it'd be better in another PR, to open it up for discussion.

@mmorel-35
Copy link
Contributor Author

Alright, it's reverted

@mmorel-35
Copy link
Contributor Author

/test pull-etcd-integration-1-cpu-amd64

@ivanvc
Copy link
Member

ivanvc commented Oct 8, 2024

Alright, it's reverted

Thank you so much.

/retest

@ivanvc
Copy link
Member

ivanvc commented Oct 8, 2024

@jmhbnz, Matthieu removed the deprecation notice from client/pkg/testutil/assert.go. The pull request is now back in the state it was in when Benjamin and I approved it. Could you PTAL at it? Thanks.

@mmorel-35
Copy link
Contributor Author

@ahrtr ,
I rebased, that shall be ready for merging after the tests are passed

@mmorel-35
Copy link
Contributor Author

/assign @jmhbnz

@mmorel-35
Copy link
Contributor Author

/test pull-etcd-integration-1-cpu-amd64

@mmorel-35
Copy link
Contributor Author

/test pull-etcd-robustness-arm64

@ahrtr ahrtr merged commit f0187c3 into etcd-io:main Oct 11, 2024
37 checks passed
@mmorel-35 mmorel-35 deleted the testifylint/nil-compare branch October 11, 2024 08:54
@ivanvc
Copy link
Member

ivanvc commented Oct 11, 2024

Link to #18719

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants