Jest and TS Config with initial Token Bucket tests #12
Conversation
jondeweydev
left a comment
jondeweydev
left a comment
There was a problem hiding this comment.
Submitted comments, mainly suggestions.
updated timestamps to use UNIX format
Summary
Type of ChangePlease delete options that are not relevant.
Issues
Evidence
|
| const tokenCountFull = await getBucketFromClient(client, user1); | ||
| expect(tokenCountFull).toBe(CAPACITY - withdraw5); | ||
|
|
||
| // Bucket partially full but enough time has elapsed to fill the bucket since the last request and |
There was a problem hiding this comment.
Break these out into separate tests instead of having one test for all of these variations on situations with allowed requests
| timestamp + 1000 * (CAPACITY - initial), | ||
| initial + partialWithdraw | ||
| ) | ||
| ).toBe(CAPACITY - (initial + partialWithdraw)); |
There was a problem hiding this comment.
These case reads to me like:
line 58 - set token bucket to have 6 tokens
line 60 - process request withdrawing 7 tokens (6+1)
line 65 - expect the 3 tokens in the bucket (10 - (6 + 1)) when I think there will be -1 tokens in the bucket
I'm having a hard time following all this redis stuff so I might be wrong. Need more time.
There was a problem hiding this comment.
This also needs to be processRequest().tokens to access the tokens property on the returned object from processRequest method. This goes for a lot of the tests in this area. Maybe also assert that processRequest().sucess is true/false as well.
There was a problem hiding this comment.
line 62 is saying that enough time has elapsed to refill the bucket since the last request (4000 ms). So when we withdraw 7 tokens there should be 3 remaining. Was trying to get after a case where the bucket fills enough between requests.
evanmcneely
left a comment
evanmcneely
left a comment
There was a problem hiding this comment.
Approved. I made one small followup comment on the optional parameters in typescript.
Initial jest and tsconfig.
Closes #4 with token bucket testing
Closes #9 with a class spec for RateLimiters