Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[RateLimiter] Fix delete method of the cache storage#38661
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you. This is correct.
Could you add a small unit test for this too?
GregOriol commentedOct 22, 2020
I'm not sure how to write such a test, I'd rather not try to make something up here. |
Nyholm commentedOct 22, 2020
Do you mind if I push to your branch? |
GregOriol commentedOct 22, 2020
Not at all, I'd be happy to take your suggestion on this :-) |
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a test.
Im happy with this PR.
| publicfunctiontestDelete() | ||
| { | ||
| $this->pool->expects($this->once())->method('deleteItem')->with(sha1('test'))->willReturn(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line uses$this->pool which is a mock created insetUp().
We say that we "expect" that "once" the "methoddeleteItem" is called withsha1('test'). We also return true.
If what we expect does not happen, then the test will fail
| { | ||
| $this->pool->expects($this->once())->method('deleteItem')->with(sha1('test'))->willReturn(true); | ||
| $this->storage->delete('test'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is just calling the storage
Nyholm commentedOct 22, 2020
Thank you@GregOriol. |
This PR fixes a small issue with RateLimiter's cache storage and the delete method: all getItems are called with a sha1 of the id, but not the one for delete, which makes it miss the deletion.