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 tests for src/module examples #1407

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

Conversation

Codebells
Copy link
Contributor

We found there is coredump in hellodict #1395 , and there is no tcl test for these example modules.

  1. Add tcl for src/module example.
    • helloblock tcl
    • hellodict tcl
    • hellotimer tcl
    • hellotype tcl
    • helloworld tcl
  2. Add a common tcl function check_commands_specs to check command by command getkeys.
    • Use this function, we can test all command by command getkeys, except the following two kind of command.
      • cmd flags is CMD_KEY_NOT_KEY
      • cmd with special parameters, like xread.

I recommand all module's tcl test to add this function to check command.

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.85%. Comparing base (e8078b7) to head (aeb2109).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1407      +/-   ##
============================================
+ Coverage     70.78%   70.85%   +0.07%     
============================================
  Files           118      118              
  Lines         63561    63561              
============================================
+ Hits          44994    45039      +45     
+ Misses        18567    18522      -45     

see 12 files with indirect coverage changes


assert_match [r hello.leftpad td 4 0] "00td"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please also do a module unload in the end

Copy link
Contributor Author

@Codebells Codebells Dec 8, 2024

Choose a reason for hiding this comment

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

When I try to unload hellotype module, I found there is only VM_CreateDataTypefunction, why don't have any function to delete it? So that I can't unload hellotype module, because of ERR Error unloading module: the module exports one or more module-side data types can't unload.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think u can skip unload hello_type module

@Codebells Codebells force-pushed the fix/src_module_tcl branch 2 times, most recently from 324242e to 19c8ddb Compare December 8, 2024 16:14
@zuiderkwast zuiderkwast changed the title Add tcl for src/module example Add tests for src/module examples Dec 10, 2024
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I'm sort of skeptical of maintaining these src modules at all. They originally existed because we didn't have module API tests. I think now that we have module API tests, we should move all of the references to those test modules.

@Codebells
Copy link
Contributor Author

Thanks for the contribution. I'm sort of skeptical of maintaining these src modules at all. They originally existed because we didn't have module API tests. I think now that we have module API tests, we should move all of the references to those test modules.

OK,I will move src/modules to test/modules, so we can maintain all these modules in test/modules.

@enjoy-binbin
Copy link
Member

OK,I will move src/modules to test/modules, so we can maintain all these modules in test/modules.

i think we should either to keep it as a demo module that can guaid people to write the special type module. Or we should just remove it from the code.

i think all the content in src/modules are cover by test/modules, we don't need to keep src/modules in the test folder.

@madolson
Copy link
Member

madolson commented Dec 20, 2024

i think we should either to keep it as a demo module that can guide people to write the special type module. Or we should just remove it from the code.

I think that is more what the module API reference is for https://valkey.io/topics/modules-api-ref. I know Redis had an SDK repo as well for modules, maybe we have a module example repo?

If we make a module example repo, I would also like to raise the quality of the modules, because I think they're a bit hacky today.

@Codebells
Copy link
Contributor Author

OK,I will move src/modules to test/modules, so we can maintain all these modules in test/modules.

i think we should either to keep it as a demo module that can guaid people to write the special type module. Or we should just remove it from the code.

i think all the content in src/modules are cover by test/modules, we don't need to keep src/modules in the test folder.

We need to make a dicision on whether to delete or keep these files.

No matter how we deal with these files, we should test command to avoid the problem in hellodict happen again.

I think use the function check_commands_specs to check all command is a good way.

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.

4 participants