-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update Valkey version to 8.0.0 in test matrix and version checks #2296
Conversation
expect(await client.getrange(key, -200, -100)).toEqual( | ||
cluster.checkIfServerVersionLessThan("8.0.0") ? "T" : "", | ||
); | ||
expect(await client.getrange(key, -200, -100)).toEqual("T"); |
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.
same here
assert await glide_client.getrange(key, -200, -100) == value[0].encode() | ||
else: | ||
assert await glide_client.getrange(key, -200, -100) == b"" | ||
assert await glide_client.getrange(key, -200, -100) == value[0].encode() |
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.
and here
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.
same
@shohamazon you should push to release branch |
aba3ab3
to
0c1db1d
Compare
} | ||
} catch (Exception e) { | ||
// General catch block in case any other exception occurs during client cleanup | ||
System.err.println("Error during cleanup for client: " + e.getMessage()); |
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.
Don't suppress an exception - we won't see a failure if it occurs (no one reads logs if CI is green). Junit catches them and reports.
Please remove try-catch blocks, we need to properly unsubscribe to avoid NOSUB
error from valkey 8
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 am waiting for your pr actually, I added this without noticing you added a fix
Can you also add a fix to node?
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.
Will do. Why it doesn't fail on python? Something it missing there.
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 think we check what commands we ran so it knows what exactly to unsubscribe from, but I need to take a look again at the test teardown
0c1db1d
to
c6fe3aa
Compare
Signed-off-by: Shoham Elias <shohame@amazon.com>
4e1d748
to
9f0a5e1
Compare
@shohamazon please link the originating issue |
…key-io#2296) Signed-off-by: Shoham Elias <shohame@amazon.com>
instead of testing with valkey8 rc on our ci, we will now test with valkey 8
inn valkey 8 rc the valkey version was 7.9.240, so all the version checks needs to be updated