Conversation
f6e33a1 to
0d687e7
Compare
matthinz
left a comment
There was a problem hiding this comment.
Am I correct that the goal here is to make it possible to time travel in specs to affect rate limiters (which was not previously possible?)
If so, can you add a test to rate_limiter_spec.rb that fails when expire is used but passes when expireat is used?
|
Similar to @matthinz's question, does this help situations like the test that initially led to trying the changes here? |
|
Added a spec that fails if the redis calls use |
|
The basic problem stemmed from a mismatch between the Redis and Ruby timestamps. Our tests allow for a one-second bobble, but between normal timing variations and the fact that we were sometimes using our clock, and sometimes Redis's, the bobble came to more than we tolerate sometimes, resulting in a few flaky specs. |
Pushed for CI run.
Spec depended on rate limiter timeouts being in Redis-time, rather than Ruby-time changelog: Internal,Rate Limiting,Use absolute timestamps in Redis for improved consistency.
Added spec which fails if we use relative timestamps.
2155612 to
91a5492
Compare
🎫 Ticket
Link to the relevant ticket:
LG-12134
🛠 Summary of changes
Changed Redis calls in RateLimiter to always use absolute time values, instead of relative ones.
This change is internal to the timestamp class, and not visible outside it.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.