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 cluster slots parsing tests #149

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

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"} 
            }
        }
    },
    ...
};

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