Skip to content

Add support for MODULE LOADEX command#2490

Merged
monkey92t merged 12 commits into
redis:masterfrom
ktsivkov:loadex-support
Apr 18, 2023
Merged

Add support for MODULE LOADEX command#2490
monkey92t merged 12 commits into
redis:masterfrom
ktsivkov:loadex-support

Conversation

@ktsivkov

@ktsivkov ktsivkov commented Mar 18, 2023

Copy link
Copy Markdown
Contributor

Adding tests for it is too tricky.
It requires the redis-server to be run with flag --enable-module-command yes and the existance of an .so file for the module to be loaded.
implement #2405

@SoulPancake

Copy link
Copy Markdown
Contributor

How are you planning to test this ? @ktsivkov

@ktsivkov

Copy link
Copy Markdown
Contributor Author

I will be unable to do it this week, but my idea is to add inside the testing script to pull one of the officially supported plugins and use it for the test.

Unless you have a better idea that could be more lightweight?
@SoulPancake

@chayim

chayim commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

@ktsivkov I don't think you can do this that way - purely because you'll need a binary, guaranteed to be compatible on an OS, it's different for each user, etc.

IMHO for this (specifically) you should validate the syntax of the command - i.e use a mock.

@ktsivkov

ktsivkov commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

@chayim Was checking yesterday @SoulPancake's issue here go-redis/redismock#69 but it looks like that is not actually testing the command's syntax. Did you mean some other type of mock?

@SoulPancake

SoulPancake commented Apr 5, 2023

Copy link
Copy Markdown
Contributor

@ktsivkov
From what I understand
A straightforward way to do the syntax check would be to just test the syntax of the LoadexConfig struct and the ToArgs() function in the following way :

It("converts the configuration to a slice of arguments correctly", func() {
        conf := &LoadexConfig{
            Path: "/path/to/your/module.so",
            Conf: map[string]interface{}{
                "param1": "value1",
                "param2": "value2",
            },
            Args: []interface{}{
                "arg1",
                "arg2",
            },
        }

        args := conf.ToArgs()

        // Test if the arguments are in the correct order
        expectedArgs := []interface{}{
            "MODULE",
            "LOADEX",
            "/path/to/your/module.so",
            "CONFIG",
            "param1",
            "value1",
            "CONFIG",
            "param2",
            "value2",
            "ARGS",
            "arg1",
            "ARGS",
            "arg2",
        }

        Expect(args).To(Equal(expectedArgs))
    })
  
    
 Feels redundant to me tbh
 Not sure how else we can write a useful test

…d` to check that the loadex command fails as expected
@ktsivkov

ktsivkov commented Apr 7, 2023

Copy link
Copy Markdown
Contributor Author

@chayim I believe the PR is ready for a review.
I did what @SoulPancake suggested me to, but I also wrote a test that checks that the command is successfully executed on the server, and that it fails loading the module. At this point this is the closest I could get to testing the full behavior.

Comment thread module_loadex_test.go Outdated
SoulPancake
SoulPancake previously approved these changes Apr 7, 2023
@SoulPancake

Copy link
Copy Markdown
Contributor

@monkey92t
Can you please review this ?

Comment thread commands.go Outdated

@monkey92t monkey92t left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good

@monkey92t monkey92t merged commit 38ca7c1 into redis:master Apr 18, 2023
@ktsivkov ktsivkov deleted the loadex-support branch April 21, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants