fix testifylint issues - #3204
Conversation
a70b8b1 to
2b6c35a
Compare
ccb8ee5 to
611b2a6
Compare
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
left a comment
There was a problem hiding this comment.
I did a rebase, and fixed up some issues, but looking at some of the changes, I'm a bit on the fence; some of the changes are really mechanical, and should be fine to take (for example using require where continuing would only lead to secondary failures). But other bits are ... too opinionated, and from failures I just saw, cause subtle failures; e.g. as require.Greater rejects int64 vs int.
e.g. this, while a bit more verbose, at least is clear on exact semantics (Go standard behavior);
if got := atomic.LoadInt64(&fakeCAServer.nodeStatusCalled); got <= 1 {
t.Fatalf("expected NodeCertificateStatus to have been polled more than once; got %d", got)
}But this failed; because require.Greater does reflection, then rejects the comparison; that's not obvious, and can easily hide issues, and require being familiar with the subtleties how testify handles these;
require.Greater(t, atomic.LoadInt64(&fakeCAServer.nodeStatusCalled), 1, ...)I think we need to hold off merging this as-is; we could still do smaller, incremental changes (like some of the assert -> require that could otherwise result in a panic); perhaps even consider switching to gotest.tools (as used in moby), but that'd be a large rewrite.
|
I'll drop this PR and replace it with several with more acceptable sizes and let's hope less opiniated modifications. |
|
Thanks! Yeah, sorry for that (and for the long delay); I was going through old PRs on this repository and thought I'd do a rebase on this one, and then ran into some of the corner-cases, which made me a bit hesitant. Smaller commits probably also help with review |
| suite.T().Helper() | ||
|
|
||
| na, err := suite.np.NewAllocator(nil) | ||
| suite.NoError(err) | ||
| suite.Require().NoError(err) |
There was a problem hiding this comment.
For these kind of changes, I was also wondering if something like require := suite.Require() would make the asserts more convenient, although I'm also not a huge fan of potentially shadowing an import 🤔
I'm less familiar with testify than with gotest.tools, so that also doesn't help (for me 😂) to pick the most idiomatic patterns.
There was a problem hiding this comment.
If you want to read more about this, you'll find it here https://github.com/Antonboom/testifylint#suite-extra-assert-call
gotest.tools seems to have converter from testify so once testifylint best practices are applied a migration can be done with thid converter.
- What I did
Use testifylint to identify issues and fix them. The linter is not enabled until golangci-lint is in v2
- How I did it
- How to test it
- Description for the changelog