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 testifylint lint errors #1321

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jasikpark
Copy link
Collaborator

https://github.com/Antonboom/testifylint contains lints for validating that you avoid pitfalls of using testify.

I fixed all of the rules except for require-error which asks you to use require.Error instead of assert.Error because it calls t.FailNow() and makes it less likely that you accidentally swallow the resulting error.

Mostly it's syntactical style and better error messages, plus fixing cases where "expected" and "actual" were switched, making error messages more confusing.

There was one case where we were testing whether an error was an error, rather than whether it matched the error in question, caught by error-is-as

@jasikpark jasikpark force-pushed the fixing-expected-actual-errors branch from ca0b825 to 1ea72b2 Compare January 29, 2025 18:22
@@ -352,7 +352,7 @@ func TestFirewall_Drop2(t *testing.T) {
cp := cert.NewCAPool()

// h1/c1 lacks the proper groups
assert.Error(t, fw.Drop(p, true, &h1, cp, nil), ErrNoMatchingRule)
assert.ErrorIs(t, fw.Drop(p, true, &h1, cp, nil), ErrInvalidRemoteIP)
Copy link
Collaborator Author

@jasikpark jasikpark Jan 29, 2025

Choose a reason for hiding this comment

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

fw.Drop catches the fact that h1 is initialized w/o a valid vpnIp before it actually checks the firewall rule later.

Caught by this block:

nebula/firewall.go

Lines 435 to 439 in 2b427a7

// Simple case: Certificate has one IP and no subnets
if fp.RemoteIP != h.vpnIp {
f.metrics(incoming).droppedRemoteIP.Inc(1)
return ErrInvalidRemoteIP
}

@jasikpark jasikpark marked this pull request as ready for review January 29, 2025 18:31
@jasikpark jasikpark requested review from nbrownus, johnmaguire and wadey and removed request for nbrownus and johnmaguire January 29, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant