uber-go / ratelimit

A Go blocking leaky-bucket rate limit implementation
MIT License
4.32k stars 300 forks source link

Add a test verifying initial startup sequence #97

Closed rabbbit closed 2 years ago

rabbbit commented 2 years ago

See https://github.com/uber-go/ratelimit/pull/95#discussion_r915251700

From that discussion I wasn't sure whether the proposed the initial startup sequence of the limiter - i.e. whether at startup we always block, or always allow.

Since we didn't seem to have that codified (perhaps apart from the example_test.go) this PR adds a test to verify this.

This is still slightly (2/1000) flaky, but I think that's good enough to add this in - should be valuable anyway.

codecov[bot] commented 2 years ago

Codecov Report

Merging #97 (a0a8d91) into main (29ac3a2) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #97   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           97        97           
=========================================
  Hits            97        97           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 29ac3a2...a0a8d91. Read the comment docs.

rabbbit commented 2 years ago

Well this one clearly needs more work.

Works fine 998/1000 on my machine, but github runner is consistently unhappy.

rabbbit commented 2 years ago

Well this one clearly needs more work.

Works fine 998/1000 on my machine, but github runner is consistently unhappy.

Turns out I just wasn't running -race locally. This is interesting though - because we have RL the code should not be racy, but the race detector (I think) has no way of knowing this.

rabbbit commented 2 years ago

@storozhukBM Could you take a look?

Irrespective of the implementation changes I think we have a gap WRT behavior on the initial few requests. This seems useful to formalize in tests.