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.

Sign up for GitHub

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

Jump to bottom

#1013 Migrating: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL') #1149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

#1013 Migrating: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL') #1149

wants to merge 1 commit into from

Conversation

SyedMa3
Copy link
Contributor

@SyedMa3 SyedMa3 commented Oct 18, 2024 •
edited by AshwinKul28
Loading

Migrated the following commands: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL')

Checklist

  • Migrated the evalXXX function with the latest definition
  • Update or add unit tests for the new implementation.
  • All unit tests pass successfully.
  • Ensure all integration tests pass successfully.
  • Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
  • Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

Closes: #1013

@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 19, 2024
JyotinderSingh
JyotinderSingh requested changes Oct 19, 2024
View reviewed changes
integration_tests/commands/http/copy_test.go
@@ -85,7 +85,7 @@ func TestCopy(t *testing.T) {
{Command: "TTL", Body: map[string]interface{}{"key": "k1"}},
{Command: "TTL", Body: map[string]interface{}{"key": "k2"}},
},
expected: []interface{}{"OK", float64(1), float64(-1), float64(-1)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

The data type of these results should not change with this migration. Same for all other instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the functions are returning clientio.IntegerOne, clientio.IntegerZero, etc., the decoder in (e *HTTPCommandExecutor) FireCommand is decoding it to string "1", "0", etc. Therefore I had to change the expected values from float to string where the migrated functions have been used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lucifercr07 could you please confirm this behavior?

integration_tests/commands/websocket/set_test.go
@@ -179,9 +179,9 @@ func TestSetWithExat(t *testing.T) {

resp, err = exec.FireCommandAndReadResponse(conn, "TTL k")
assert.Nil(t, err)
respFloat, ok = resp.(float64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to change this?

Copy link
Contributor Author

@SyedMa3 SyedMa3 Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

same explanation as above in case of websockets too

integration_tests/commands/websocket/set_test.go
@@ -200,9 +200,9 @@ func TestSetWithExat(t *testing.T) {

resp, err = exec.FireCommandAndReadResponse(conn, "TTL k")
assert.Nil(t, err)
respFloat, ok := resp.(float64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to change this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JyotinderSingh JyotinderSingh Awaiting requested review from JyotinderSingh

Assignees
No one assigned
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Command Migration: ('EXPIRE', 'EXPIREAT', 'EXPIRETIME', 'TTL', 'PTTL')
3 participants
@SyedMa3 @JyotinderSingh @AshwinKul28