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

Add support for LMPOP #2440

Merged
merged 13 commits into from Feb 13, 2023
Merged

Add support for LMPOP #2440

merged 13 commits into from Feb 13, 2023

Conversation

SoulPancake
Copy link
Contributor

Adding command and command test for LMPOP command.

@monkey92t
Copy link
Collaborator

You should handle redis-server response data correctly.

@SoulPancake
Copy link
Contributor Author

@monkey92t I'm a little confused about decoding the RESP array parts , In the other tests with array reply it looked straightforward however here the string slice is in that format , Do you have any suggestions for writing meaningful tests for that part? Is this something I should refer to ? https://redis.io/docs/reference/protocol-spec/#resp-arrays:~:text=the%20reply%20object).-,RESP%20Arrays,-Clients%20send%20commands

@SoulPancake
Copy link
Contributor Author

@monkey92t Can you suggest a function I could refer to regarding Array Replies like these so I make tests similarly?

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 13, 2023

@SoulPancake

You should look at the return value of the command:

LMPOP 2 mylist mylist2 right count 3
*2
$6
mylist
*3
$3
one
$3
two
$5
three

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 13, 2023

The LMPOP command responds not to a slice, but to a complex structure, an example in the go structure:

type Data struct {
         key string
         Element [] string
}

@SoulPancake
Copy link
Contributor Author

@monkey92t So I should change my approach from a stringSliceCmd to a custom struct correct?

@SoulPancake
Copy link
Contributor Author

The LMPOP command responds not to a slice, but to a complex structure

Do we already have a struct for running such complex structure command responses that I can re-use or do I need to create it as well?

@monkey92t
Copy link
Collaborator

The LMPOP command responds not to a slice, but to a complex structure

Do we already have a struct for running such complex structure command responses that I can re-use or do I need to create it as well?

Yes, when you find that there is no suitable data structure in command.go, you should create a new CMD.

@monkey92t
Copy link
Collaborator

@SoulPancake

If you need a test environment, welcome to use the tool I made: "https://github.com/monkey92t/grte" 😁

@SoulPancake
Copy link
Contributor Author

@monkey92t Thanks a lot and sorry for bothering so many times, I am new to this I am trying to learn and help.
I will close this and re-open it after I've figured it out.

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 13, 2023

@SoulPancake

OK, "https://github.com/monkey92t/grte" This tool is relatively simple, but use it to do some simple testing work, the following is an example:

monkey@MonkeyiMac redis % grte go test ./...                                                                           
[go-redis]  🐳 Prepare docker image ==> goredis/grte:v9
[go-redis]  🐳 Use image cache, ID ==> sha256:f4a6149b55e000ed40b5a68041615e7223b52119178d6ec4190f4714b8b88a49
[go-redis]  🐳 Create docker container...
[go-redis]  🐳 ContainerID: 28f0d7c79293a106bf0c54a76ea3a4ede41ee052332fe49263e24f8609a2aa7d
[go-redis]  🐳 WorkDir: /Users/monkey/Desktop/redis
[go-redis]  🐳 Command: [go test ./...]
go: downloading github.com/bsm/gomega v1.26.0
go: downloading github.com/bsm/ginkgo/v2 v2.7.0
go: downloading github.com/cespare/xxhash/v2 v2.2.0
go: downloading github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f
go: downloading github.com/stretchr/testify v1.8.1
go: downloading github.com/davecgh/go-spew v1.1.1
go: downloading github.com/pmezard/go-difflib v1.0.0
go: downloading gopkg.in/yaml.v3 v3.0.1
ok  	github.com/redis/go-redis/v9	103.717s
ok  	github.com/redis/go-redis/v9/internal	0.007s
ok  	github.com/redis/go-redis/v9/internal/hashtag	0.008s
ok  	github.com/redis/go-redis/v9/internal/hscan	0.009s
ok  	github.com/redis/go-redis/v9/internal/pool	1.589s
ok  	github.com/redis/go-redis/v9/internal/proto	0.009s
?   	github.com/redis/go-redis/v9/internal/rand	[no test files]
?   	github.com/redis/go-redis/v9/internal/util	[no test files]
[go-redis]  ✅  Success!

If you want to test other directories:

monkey@MonkeyiMac redis % cd internal/pool
monkey@MonkeyiMac pool % grte go test ./...
[go-redis]  🐳 Prepare docker image ==> goredis/grte:v9
[go-redis]  🐳 Use image cache, ID ==> sha256:f4a6149b55e000ed40b5a68041615e7223b52119178d6ec4190f4714b8b88a49
[go-redis]  🐳 Create docker container...
[go-redis]  🐳 ContainerID: b094b74369998707dbcfeb128827a14dace8af67783460b5cbaa2d27d922eb45
[go-redis]  🐳 WorkDir: /Users/monkey/Desktop/redis/internal/pool
[go-redis]  🐳 Command: [go test ./...]
ok  	github.com/redis/go-redis/v9/internal/pool	1.529s
[go-redis]  ✅  Success!
monkey@MonkeyiMac pool % 

hope it helps you....

@SoulPancake
Copy link
Contributor Author

Thanks a lot @monkey92t

@SoulPancake SoulPancake reopened this Feb 13, 2023
@SoulPancake SoulPancake reopened this Feb 13, 2023
@SoulPancake
Copy link
Contributor Author

@monkey92t The test failure that I can see seems to be unrelated from the implementation or test, Can you take a look at it once?

@monkey92t
Copy link
Collaborator

monkey92t commented Feb 13, 2023

@monkey92t The test failure that I can see seems to be unrelated from the implementation or test, Can you take a look at it once?

The []interface{} type is not convenient to use.

@SoulPancake
Copy link
Contributor Author

@monkey92t I had referred to

Expect(hMGet.Val()).To(Equal([]interface{}{
as they were using a similar approach,
To be able to use SliceCmd this was an approach but I guess now a custom struct is a nicer approach?

Signed-off-by: monkey92t <golang@88.com>
@monkey92t monkey92t marked this pull request as ready for review February 13, 2023 14:37
Copy link
Collaborator

@monkey92t monkey92t left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

None yet

2 participants