From d9e1e7e8ccdb31ce5af6ea0c7a2d1b9713a55d41 Mon Sep 17 00:00:00 2001 From: Mikhail Koviazin Date: Tue, 24 Sep 2024 16:23:25 +0300 Subject: [PATCH 1/2] examples: fix test_helper.err `TEST_HELPER.ERR` contained two issues: 1. It checked for the wrong amount of arguments. `args.len()` acts like `argc` and when one expects one required argument, they should check for `args.len() < 2` to fail. This function had `args.len() < 1` which was always false 2. It replied twice to the client that made client confused. First reply was done via call for `ctx.reply_error_string` that is a thin wrapper around `ValkeyModule_ReplyWithError`. The second reply was with returning `ValkeyResult` from the function, which internally translates to another call for `ValkeyModule_ReplyWith*`. Since the function must return `ValkeyResult`, it was changed to reply once via `ValkeyResult` Signed-off-by: Mikhail Koviazin --- examples/test_helper.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/examples/test_helper.rs b/examples/test_helper.rs index 45bff8d..b108f5c 100644 --- a/examples/test_helper.rs +++ b/examples/test_helper.rs @@ -20,15 +20,14 @@ fn test_helper_command_name(ctx: &Context, _args: Vec) -> ValkeyRe Ok(ctx.current_command_name()?.into()) } -fn test_helper_err(ctx: &Context, args: Vec) -> ValkeyResult { - if args.len() < 1 { +fn test_helper_err(_ctx: &Context, args: Vec) -> ValkeyResult { + if args.len() < 2 { return Err(ValkeyError::WrongArity); } let msg = args.get(1).unwrap(); - ctx.reply_error_string(msg.try_as_str().unwrap()); - Ok(().into()) + Err(ValkeyError::Str(msg.try_as_str().unwrap())) } fn add_info_impl(ctx: &InfoContext, _for_crash_report: bool) -> ValkeyResult<()> { From 5433e30379e6cb79625215b34b64e2d816ee1ea6 Mon Sep 17 00:00:00 2001 From: Mikhail Koviazin Date: Tue, 24 Sep 2024 16:27:46 +0300 Subject: [PATCH 2/2] tests: fix the comment in tests/integration.rs It used to contain a link to an issue where the certain command crashes the valkey server, but that bug was fixed. We can't uncomment the test yet because it tries to compare `valkey_version` with `redis_version` which are not guaranteed to be the same. Signed-off-by: Mikhail Koviazin --- tests/integration.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration.rs b/tests/integration.rs index e4d7616..d4e2f87 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -69,8 +69,9 @@ fn test_helper_version() -> Result<()> { .with_context(|| "failed to run test_helper.version")?; assert!(res[0] > 0); - // TODO https://github.com/valkey-io/valkeymodule-rs/issues/14 // Test also an internal implementation that might not always be reached + // TODO: this check is currently disabled because Valkey 8.0.0 returns + // redis_version:7.2.4 and the test expects it to be 8.0.0 // let res2: Vec = redis::cmd("test_helper._version_rm_call") // .query(&mut con) // .with_context(|| "failed to run test_helper._version_rm_call")?;