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

Address failing tests #59

Merged
merged 4 commits into from
Oct 20, 2024
Merged

Address failing tests #59

merged 4 commits into from
Oct 20, 2024

Conversation

gillchristian
Copy link
Collaborator

@gillchristian gillchristian commented Oct 20, 2024

TTL race condition?

Setting expiration on a key and checking ttl on it right away produces different results.

The following commands:

SET key 1 EX 1
GETEX key EX 10
TTL key

Return a ttl of 10 on Redis and 9 on our server.

It isn't clear if this is a race condition on the tests (I doubt it) or if our TTL implementation works differently.

Keys sorting

Introduced sorting by insertion time to our keys command implementation to make it deterministic.

Redis' seems to be deterministic (always the same for a given set of keys) but not guaranteed (changes across different runs and instances, eg. local vs. CI tests).

I changed the tests to only return one key at a time.

Copy link
Owner

@ndelvalle ndelvalle left a comment

Choose a reason for hiding this comment

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

I wonder if we should not update the implementation but rather the tests, doing a comparison that does not care about the orden.

src/store.rs Outdated
@@ -232,6 +238,7 @@ type Key = String;
pub struct Value {
pub data: Bytes,
pub expires_at: Option<Instant>,
pub inserted_at: Instant,
Copy link
Owner

Choose a reason for hiding this comment

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

created_at? I've always seen/used created_at and updated_at on db records.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's better

@gillchristian
Copy link
Collaborator Author

I wonder if we should not update the implementation but rather the tests, doing a comparison that does not care about the orden.

Even if we update the tests, I'd still keep the deterministic order on the implementation.

@gillchristian gillchristian merged commit 7db8830 into master Oct 20, 2024
0 of 2 checks passed
@gillchristian gillchristian deleted the support/failing-tests branch October 21, 2024 12:16
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.

2 participants