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

cleanup: fix vet and staticcheck failures #435

Merged
merged 3 commits into from
Jul 22, 2021

Conversation

iand
Copy link
Contributor

@iand iand commented Jul 19, 2021

Fixes the safe staticcheck failures. The following remain and need more thought or explicit ignores.

May indicate a bug, not clear what the intention is here:

gossipsub.go:904:4: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)

Needs a decision on the alternative:

gossipsub.go:1300:8: log.EventBegin is deprecated: Stop using go-log for event logging  (SA1019)

Changing these may break clients who rely on error string text:

validation.go:146:15: error strings should not be capitalized (ST1005)
validation.go:173:15: error strings should not be capitalized (ST1005)
validation.go:209:15: error strings should not be capitalized (ST1005)
validation.go:387:9: error strings should not be capitalized (ST1005)

@vyzo
Copy link
Collaborator

vyzo commented Jul 19, 2021

gossipsub.go:904:4: ineffective break statement

That thing with the break is really innocuous, we can just remove it.

gossipsub.go:1300:8: log.EventBegin is deprecated: Stop using go-log for event logging (SA1019)

We can just replace it with a log.Infow that logs the timing

Changing these may break clients who rely on error string text:

That's just silly/stupid; just go ahead and change them.

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

maybe add some short-circuit code for breaking the test when we have goroutines failing? Or wait for them. Not sure what's prudent.

@iand
Copy link
Contributor Author

iand commented Jul 19, 2021

breaking

Is this a reference to the replacement of t.Fatal with t.Error+return in test goroutines? In these cases the test still fails.

@iand iand changed the title cleanup: fix vet failures and most staticcheck failures cleanup: fix vet and staticcheck failures Jul 19, 2021
@vyzo
Copy link
Collaborator

vyzo commented Jul 19, 2021 via email

@iand
Copy link
Contributor Author

iand commented Jul 19, 2021

yeah, just worried it might get blocked somewhere.

OK. In TestGossipsubAttackSpamIHAVE returning from the goroutine cancels the context. I could use that to ensure that the other goroutines exit too (we should do that anyway to avoid other sources of blocking). Will look at the other tests as well.

@iand
Copy link
Contributor Author

iand commented Jul 19, 2021

yeah, just worried it might get blocked somewhere.

OK. In TestGossipsubAttackSpamIHAVE returning from the goroutine cancels the context. I could use that to ensure that the other goroutines exit too (we should do that anyway to avoid other sources of blocking). Will look at the other tests as well.

Done this in 4238ca7

@Stebalien Stebalien merged commit 2efd313 into libp2p:master Jul 22, 2021
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants