Skip to content

Conversation

@bjosv
Copy link
Collaborator

@bjosv bjosv commented Jan 17, 2025

Adds CLUSTER SLOTS parser tests, which uses a testsuite-local primitive parser of a JSON subset to encode arrays to RESP.

Signed-off-by: Björn Svensson <[email protected]>
@bjosv bjosv requested a review from zuiderkwast January 17, 2025 13:14
Comment on lines +525 to +528
valkeyReply *reply = create_cluster_slots_reply(
"[[0, 5460, ['127.0.0.1', 30001, 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', ['hostname', 'localhost']],"
" ['127.0.0.1', 30004, '07c37dfeb235213a872192d90877d0cd55635b91', ['hostname', 'localhost']]],"
" [5461, 10922, ['127.0.0.1', 30002, '67ed2db8d677e59ec4a4cefb06858cf2a1a89fa1', ['hostname', 'localhost']],"
Copy link
Collaborator

@zuiderkwast zuiderkwast Jan 17, 2025

Choose a reason for hiding this comment

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

Interesting! I guess it's pretty easy to convert this pseudo-JSON to RESP, althought it adds some non-trivial code to the testing. My first though is that single-quoted stings are not allowed in JSON. :)

My second thought is that it might be possible to construct a parse tree structure that is readable and easy to serialize to RESP, without parsing. Ideally, test code should be trivial, so there is no doubt if there's a bug, is it a bug in the test code or in the real implementation?

Here is a reply literal with the first slot range (untested). It's much bigger than the pseudo-JSON obviously.

valkeyReply reply = {.type = VALKEY_REPLY_ARRAY, .elements = 3, .element = (valkeyReply[]){
    {.type = VALKEY_REPLY_ARRAY, .elements = 4, .element = (valkeyReply[]){
        {.type = VALKEY_REPLY_INTEGER, .integer = 0},
        {.type = VALKEY_REPLY_INTEGER, .integer = 5460},
        {.type = VALKEY_REPLY_ARRAY, .elements = 4, .element = (valkeyReply[]){
            {.type = VALKEY_REPLY_STRING, .str = "127.0.0.1"},
            {.type = VALKEY_REPLY_INTEGER, .integer = 30005},
            {.type = VALKEY_REPLY_STRING, .str = "e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca"},
            {.type = VALKEY_REPLY_ARRAY, .elements = 2, .element = (valkeyReply[]){
                {.type = VALKEY_REPLY_STRING, .str = "hostname"},
                {.type = VALKEY_REPLY_STRING, .str = "localhost"} 
            }
        }
    },
    ...
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, are there similar tests in the valkey repo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, but similar nested struct literals, in some modules used for module API tests. The one for testing key specifications IIRC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests for replies like CLUSTER SLOTS use Tcl code which is compact but most often doesn't check the types.

A nested reply like that is a Tcl list on the form

{{0 5460 {127.0.0.1 30001 abcde123456 {hostname localhost} ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hum, I've been trying to get something nicer to work, but have failed so far.
The problem I face is that .element is a valkeyReply **.
Not sure if splitting up the initialization makes it readable, maybe a macro can be used in some way..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use the pseudo-JSON then. It's pretty simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll merge this variant then.

@bjosv bjosv merged commit e63166b into valkey-io:main Jan 21, 2025
43 checks passed
@bjosv bjosv deleted the cluster-slots-test branch January 21, 2025 19:34
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