Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix update of expired items #124

Open
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

Snawoot
Copy link

@Snawoot Snawoot commented Feb 23, 2024

Fix #123

Extends get method to return expired items to set method in order to avoid second insertion into expiration queue.

Copy link
Contributor

@davseby davseby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

Providing some feedback, however hadn't had the time to investigate this issue thoroughly, will need to come back and re-check this.

cache_test.go Outdated
@@ -1212,3 +1212,29 @@ func addToCache(c *Cache[string, string], ttl time.Duration, keys ...string) {
c.items.expQueue.push(elem)
}
}

func TestUpdateExpired(t *testing.T) {
Copy link
Contributor

@davseby davseby Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how we write unit tests. The test cases should be incorporated into the functions named after the methods that were changed.

Copy link
Author

@Snawoot Snawoot Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't see much value in this test at all, it's too specific. But this test was crucial for me to reproduce issue I was suspecting and validate solution. Should I relocate it or should I just drop it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test_Cache_set should be expanded to include a test for overwriting an expired item.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

cache.go Outdated Show resolved Hide resolved
Signed-off-by: Vladislav Yarmak <vladislav-ex-src@vm-0.com>
Signed-off-by: Vladislav Yarmak <vladislav-ex-src@vm-0.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange issue with OnEviction/OnInsertion hooks
2 participants