Enable Client.RateLimits to bypass the rate limit check#1907
Conversation
|
Whups, sorry, it looks like the unit tests are not passing: |
gmlewis
left a comment
There was a problem hiding this comment.
Changing approval until unit tests pass.
Credits to @ViBiOh for the
Whups indeed, my bad. Got a little too confident, it is now fixed. |
Codecov Report
@@ Coverage Diff @@
## master #1907 +/- ##
=======================================
Coverage 97.65% 97.65%
=======================================
Files 105 105
Lines 6786 6788 +2
=======================================
+ Hits 6627 6629 +2
Misses 86 86
Partials 73 73
Continue to review full report at Codecov.
|
| client.rateLimits[0].Reset.Time = time.Now().Add(10 * time.Minute) | ||
| resp, err = f() | ||
| if bypass := resp.Request.Context().Value(bypassRateLimitCheck); bypass != nil { | ||
| return |
There was a problem hiding this comment.
I'm concerned that we are not checking the value of err here... might there be a case where we get an error that will be ignored here?
There was a problem hiding this comment.
This helper explicitly sets the client up to trigger errors, and at this stage a rate limit error. As is, the test would fail if we check for err != nil for methodName == "RateLimits" as the request would result in a HTTP 404, since no handler has been created. We can set one up on TestRateLimits_coverage but I think it makes the test less straightforward. Client.RateLimits rate limit check bypass behavior is already covered by the newly introduced TestRateLimits_overQuota.
gmlewis
left a comment
There was a problem hiding this comment.
LGTM.
Awaiting second LGTM before merging.
|
Friendly ping @wesleimp |
Fixes #1899.