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

add go linter - "unused" #11235

Merged
merged 15 commits into from
Sep 5, 2023
61 changes: 61 additions & 0 deletions chain/sub/ratelimit/queue_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package ratelimit

import (
"testing"
)

func TestQueue(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No objection to this test, obviously, but I'm surprised it was needed to make the queue lok "used" -- it should already look used because of its use in rate limiting.

Do you understand why this was the case?

Copy link
Contributor Author

@snissn snissn Sep 5, 2023

Choose a reason for hiding this comment

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

Yes! I sort of understand, for some reason pop is unused:

chain/sub/ratelimit/queue.go:38:17: func `(*queue).pop` is unused (unused)
func (q *queue) pop() int64 {

it's pretty common to write a little datastructure library, write out a bunch of api/funcs for it, and then not use a few when you have your final implementation. I think there's some combination of peeking and truncating that happens in ./chain/sub/ratelimit/window.go instead of pop, but i haven't done a thorough dive into it. Given my expectation of this pattern of building a general tool and then just using some of it, the test made more sense than deleting pop in case in future we wanted to reorganize this code. also just a simple parallel example, might make sense to write a queue library that lets you take out values from the top or bottom, but so far only use one of them

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation! Makes total sense.

const size = 3
q := &queue{buf: make([]int64, size)}

if q.len() != 0 {
t.Fatalf("q.len() = %d, expect 0", q.len())
}

if q.cap() != size {
t.Fatalf("q.cap() = %d, expect %d", q.cap(), size)
}

for i := int64(0); i < int64(size); i++ {
err := q.push(i)
if err != nil {
t.Fatalf("cannot push element %d", i)
}
}

if q.len() != size {
t.Fatalf("q.len() = %d, expect %d", q.len(), size)
}

err := q.push(int64(size))
if err != ErrRateLimitExceeded {
t.Fatalf("pushing element beyond capacity should have failed with err: %s, got %s", ErrRateLimitExceeded, err)
}

if q.front() != 0 {
t.Fatalf("q.front() = %d, expect 0", q.front())
}

if q.back() != int64(size-1) {
t.Fatalf("q.back() = %d, expect %d", q.back(), size-1)
}

popVal := q.pop()
if popVal != 0 {
t.Fatalf("q.pop() = %d, expect 0", popVal)
}

if q.len() != size-1 {
t.Fatalf("q.len() = %d, expect %d", q.len(), size-1)
}

// Testing truncation.
threshold := int64(1)
q.truncate(threshold)
if q.len() != 1 {
t.Fatalf("q.len() after truncate = %d, expect 1", q.len())
}
if q.front() != 2 {
t.Fatalf("q.front() after truncate = %d, expect 2", q.front())
}
}