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 MODULE LOADEX command #2490

Merged
merged 12 commits into from
Apr 18, 2023
Merged

Conversation

ktsivkov
Copy link
Contributor

@ktsivkov ktsivkov commented Mar 18, 2023

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
Contributor

How are you planning to test this ? @ktsivkov

@ktsivkov
Copy link
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
Copy link
Contributor

chayim commented Mar 28, 2023

@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
Copy link
Contributor Author

ktsivkov commented Apr 5, 2023

@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
Copy link
Contributor

SoulPancake commented Apr 5, 2023

@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
Copy link
Contributor Author

ktsivkov commented Apr 7, 2023

@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.

module_loadex_test.go Outdated Show resolved Hide resolved
SoulPancake
SoulPancake previously approved these changes Apr 7, 2023
@SoulPancake
Copy link
Contributor

@monkey92t
Can you please review this ?

commands.go Outdated Show resolved Hide resolved
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.

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