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

Enable testifylint linter rules #18719

Open
16 of 19 tasks
ivanvc opened this issue Oct 11, 2024 · 8 comments · May be fixed by #18827
Open
16 of 19 tasks

Enable testifylint linter rules #18719

ivanvc opened this issue Oct 11, 2024 · 8 comments · May be fixed by #18827
Assignees
Labels
priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. type/cleanup type/feature

Comments

@ivanvc
Copy link
Member

ivanvc commented Oct 11, 2024

@ghouscht
Copy link
Contributor

Hey @ivanvc, do you need some help here? I could take on a few items on the list if you want me to. Just let me know which are the preferred ones.
Thank you

@ivanvc
Copy link
Member Author

ivanvc commented Oct 15, 2024

Hi @ghouscht, thanks for offering your help with this task. Although it's unassigned (because I can't assign it to people outside of the etcd-io organization if they haven't participated in the issue), @mmorel-35 is doing it.

Therefore, I'm going to redirect the question. @mmorel-35, is there anything that @ghouscht could do to help with this task? Thanks.

@mmorel-35
Copy link
Contributor

mmorel-35 commented Oct 15, 2024

We actually have two open PR for this linter so until they are merged you can start with the error-is-as or formatter with formatter.require-f-funcs option enabled. Keep your PR as draft until one of them is merged so the number of opened PR stay narrowed concerning testifylint.

You can configure it with https://golangci-lint.run/usage/linters/#testifylint

Golangci lint can not fix it with --fix. You can use testifylint with -fix to start fixing it then review the changes to see if everthing works correctly.

@ghouscht
Copy link
Contributor

ghouscht commented Oct 15, 2024

Thank you for the response @mmorel-35, I found some time today and opened two PRs for the error-is-as and formatter rules:

I also tagged you on the PRs (sorry for the spam 🙂).

@ivanvc
Copy link
Member Author

ivanvc commented Oct 15, 2024

/assign @mmorel-35 @ghouscht

@jmhbnz
Copy link
Member

jmhbnz commented Oct 17, 2024

Just curious - we should compare the make verify runtime before enabling these checks and now to make sure we aren't increasing test runtimes too significantly.

@mmorel-35 - As you have enabled these linter rules have you noticed any meaningful increase in test runtimes in CI or locally?

@mmorel-35
Copy link
Contributor

I haven't noticed any significant increase. What make you think there could be regarding the nature of changes that has been done yet ?

@ghouscht
Copy link
Contributor

Running make verify-lint yields the following runtime on my machine

with a clean golangci-lint cache:

  • with testifylint 21.941s
  • without 20.632s

with golangci-lint cache present:

  • with testifylint 4.926s
  • without 4.947s

I'd say that is neglible 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. type/cleanup type/feature
Development

Successfully merging a pull request may close this issue.

4 participants