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

Pass null-terminated node ID for VM_RegisterClusterMessageReceiver and add test coverage #1708

Merged

Conversation

Nikhil-Manglore
Copy link
Contributor

@Nikhil-Manglore Nikhil-Manglore commented Feb 11, 2025

Closes #1656.

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.14%. Comparing base (da3f1c6) to head (18117ba).
Report is 37 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 0.00% 3 Missing ⚠️
src/module.c 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1708      +/-   ##
============================================
+ Coverage     71.06%   71.14%   +0.08%     
============================================
  Files           121      123       +2     
  Lines         65254    65534     +280     
============================================
+ Hits          46371    46627     +256     
- Misses        18883    18907      +24     
Files with missing lines Coverage Δ
src/module.c 9.61% <0.00%> (+0.01%) ⬆️
src/cluster_legacy.c 85.87% <0.00%> (-0.39%) ⬇️

... and 32 files with indirect coverage changes

@Nikhil-Manglore
Copy link
Contributor Author

Looking into the CI Failure

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.

@hpatro hpatro requested a review from zuiderkwast February 17, 2025 20:37
@Nikhil-Manglore Nikhil-Manglore changed the title Added test coverage for module callback registration for cluster message Null terminated the Node ID and added test coverage for module callback registration for cluster message Feb 19, 2025
@Nikhil-Manglore Nikhil-Manglore changed the title Null terminated the Node ID and added test coverage for module callback registration for cluster message Null-terminated Node ID and add test coverage for module cluster message callbacks Feb 19, 2025
Signed-off-by: Nikhil Manglore <[email protected]>
@hpatro hpatro changed the title Null-terminated Node ID and add test coverage for module cluster message callbacks Pass null-terminated node ID for VM_RegisterClusterMessageReceiver and add test coverage Feb 19, 2025
Signed-off-by: Nikhil Manglore <[email protected]>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Yeah, just some nits, then let's merge it.

@zuiderkwast zuiderkwast merged commit 206f7f6 into valkey-io:unstable Feb 20, 2025
51 checks passed
@Nikhil-Manglore Nikhil-Manglore deleted the module_callback_registration branch February 20, 2025 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To be backported
Development

Successfully merging this pull request may close these issues.

Add test coverage for module callback registration for cluster message via VM_RegisterClusterMessageReceiver
4 participants