From 8678d1c2585dcc44856c30454b9c87a414965dfe Mon Sep 17 00:00:00 2001 From: Victor Polevoy Date: Fri, 24 Nov 2023 11:14:57 +0100 Subject: [PATCH 1/3] Simplify the integration test code --- tests/integration.rs | 259 +++++++++++++++++-------------------------- 1 file changed, 104 insertions(+), 155 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 7864e366..65bd50a9 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1,21 +1,58 @@ +use std::sync::atomic::AtomicU16; use std::thread; use std::time::Duration; use crate::utils::{get_redis_connection, start_redis_server_with_module}; use anyhow::Context; use anyhow::Result; -use redis::Value; -use redis::{RedisError, RedisResult}; +use redis::{Connection, RedisError, RedisResult, Value}; +use redis_module::RedisValue; +use utils::ChildGuard; mod utils; +fn start_redis(module_name: &str, port: u16) -> Result, &'static str> { + Ok(vec![start_redis_server_with_module(module_name, port) + .map_err(|_| "failed to start redis server")?]) +} + +struct TestConnection { + _guards: Vec, + connection: Connection, +} + +static mut TEST_PORT: AtomicU16 = AtomicU16::new(6479); + +impl TestConnection { + fn new(module_name: &str) -> Self { + unsafe { + let port = TEST_PORT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + + Self { + _guards: start_redis(module_name, port).expect("Redis instance started."), + connection: get_redis_connection(port).expect("Established connection to server."), + } + } + } +} + +impl std::ops::Deref for TestConnection { + type Target = Connection; + + fn deref(&self) -> &Self::Target { + &self.connection + } +} + +impl std::ops::DerefMut for TestConnection { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.connection + } +} + #[test] fn test_hello() -> Result<()> { - let port: u16 = 6479; - let _guards = vec![start_redis_server_with_module("hello", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("hello"); let res: Vec = redis::cmd("hello.mul") .arg(&[3, 4]) @@ -34,11 +71,7 @@ fn test_hello() -> Result<()> { #[test] fn test_keys_pos() -> Result<()> { - let port: u16 = 6480; - let _guards = vec![start_redis_server_with_module("keys_pos", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("keys_pos"); let res: Vec = redis::cmd("keys_pos") .arg(&["a", "1", "b", "2"]) @@ -57,11 +90,7 @@ fn test_keys_pos() -> Result<()> { #[test] fn test_helper_version() -> Result<()> { - let port: u16 = 6481; - let _guards = vec![start_redis_server_with_module("test_helper", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("test_helper"); let res: Vec = redis::cmd("test_helper.version") .query(&mut con) @@ -79,13 +108,7 @@ fn test_helper_version() -> Result<()> { #[test] fn test_command_name() -> Result<()> { - use redis_module::RedisValue; - - let port: u16 = 6482; - let _guards = vec![start_redis_server_with_module("test_helper", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("test_helper"); // Call the tested command let res: Result = redis::cmd("test_helper.name").query(&mut con); @@ -126,11 +149,7 @@ fn test_helper_info() -> Result<()> { MODULES .into_iter() .try_for_each(|(module, has_dictionary)| { - let port: u16 = 6483; - let _guards = vec![start_redis_server_with_module(module, port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new(module); let res: String = redis::cmd("INFO") .arg(module) @@ -151,11 +170,7 @@ fn test_info_handler_multiple_sections() -> Result<()> { const MODULES: [&str; 1] = ["info_handler_multiple_sections"]; MODULES.into_iter().try_for_each(|module| { - let port: u16 = 6500; - let _guards = vec![start_redis_server_with_module(module, port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new(module); let res: String = redis::cmd("INFO") .arg(format!("{module}_InfoSection2")) @@ -172,11 +187,7 @@ fn test_info_handler_multiple_sections() -> Result<()> { #[allow(unused_must_use)] #[test] fn test_test_helper_err() -> Result<()> { - let port: u16 = 6484; - let _guards = vec![start_redis_server_with_module("hello", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("hello"); // Make sure embedded nulls do not cause a crash redis::cmd("test_helper.err") @@ -192,11 +203,7 @@ fn test_test_helper_err() -> Result<()> { #[test] fn test_string() -> Result<()> { - let port: u16 = 6485; - let _guards = vec![start_redis_server_with_module("string", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("string"); redis::cmd("string.set") .arg(&["key", "value"]) @@ -212,11 +219,7 @@ fn test_string() -> Result<()> { #[test] fn test_scan() -> Result<()> { - let port: u16 = 6486; - let _guards = vec![start_redis_server_with_module("scan_keys", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("scan_keys"); redis::cmd("set") .arg(&["x", "1"]) @@ -238,11 +241,7 @@ fn test_scan() -> Result<()> { #[test] fn test_stream_reader() -> Result<()> { - let port: u16 = 6487; - let _guards = vec![start_redis_server_with_module("stream", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("stream"); let _: String = redis::cmd("XADD") .arg(&["s", "1-1", "foo", "bar"]) @@ -278,11 +277,7 @@ fn test_stream_reader() -> Result<()> { #[test] fn test_call() -> Result<()> { - let port: u16 = 6488; - let _guards = vec![start_redis_server_with_module("call", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("call"); let res: String = redis::cmd("call.test") .query(&mut con) @@ -295,11 +290,7 @@ fn test_call() -> Result<()> { #[test] fn test_ctx_flags() -> Result<()> { - let port: u16 = 6489; - let _guards = vec![start_redis_server_with_module("ctx_flags", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("ctx_flags"); let res: String = redis::cmd("my_role").query(&mut con)?; @@ -310,11 +301,7 @@ fn test_ctx_flags() -> Result<()> { #[test] fn test_get_current_user() -> Result<()> { - let port: u16 = 6490; - let _guards = vec![start_redis_server_with_module("acl", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("acl"); let res: String = redis::cmd("get_current_user").query(&mut con)?; @@ -325,11 +312,7 @@ fn test_get_current_user() -> Result<()> { #[test] fn test_verify_acl_on_user() -> Result<()> { - let port: u16 = 6491; - let _guards = vec![start_redis_server_with_module("acl", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("acl"); let res: String = redis::cmd("verify_key_access_for_user") .arg(&["default", "x"]) @@ -366,16 +349,12 @@ fn test_verify_acl_on_user() -> Result<()> { #[test] fn test_key_space_notifications() -> Result<()> { - let port: u16 = 6492; - let _guards = vec![start_redis_server_with_module("events", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("events"); let res: usize = redis::cmd("events.num_key_miss").query(&mut con)?; assert_eq!(res, 0); - let _ = redis::cmd("GET").arg(&["x"]).query(&mut con)?; + redis::cmd("GET").arg(&["x"]).query(&mut con)?; let res: usize = redis::cmd("events.num_key_miss").query(&mut con)?; assert_eq!(res, 1); @@ -390,11 +369,7 @@ fn test_key_space_notifications() -> Result<()> { #[test] fn test_context_mutex() -> Result<()> { - let port: u16 = 6493; - let _guards = vec![start_redis_server_with_module("threads", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("threads"); let res: String = redis::cmd("set_static_data") .arg(&["foo"]) @@ -412,11 +387,7 @@ fn test_context_mutex() -> Result<()> { #[test] fn test_server_event() -> Result<()> { - let port: u16 = 6494; - let _guards = vec![start_redis_server_with_module("server_events", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("server_events"); redis::cmd("flushall") .query(&mut con) @@ -461,69 +432,67 @@ fn test_server_event() -> Result<()> { #[test] fn test_configuration() -> Result<()> { - let port: u16 = 6495; - let _guards = vec![start_redis_server_with_module("configuration", port) - .with_context(|| "failed to start redis server")?]; + let mut con = TestConnection::new("configuration"); - let config_get = |config: &str| -> Result { - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let config_get = |con: &mut TestConnection, config: &str| -> Result { let res: Vec = redis::cmd("config") .arg(&["get", config]) - .query(&mut con) + .query(con) .with_context(|| "failed to run flushall")?; Ok(res[1].clone()) }; - let config_set = |config: &str, val: &str| -> Result<()> { - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let config_set = |con: &mut TestConnection, config: &str, val: &str| -> Result<()> { let res: String = redis::cmd("config") .arg(&["set", config, val]) - .query(&mut con) + .query(con) .with_context(|| "failed to run flushall")?; assert_eq!(res, "OK"); Ok(()) }; - assert_eq!(config_get("configuration.i64")?, "10"); - config_set("configuration.i64", "100")?; - assert_eq!(config_get("configuration.i64")?, "100"); + assert_eq!(config_get(&mut con, "configuration.i64")?, "10"); + config_set(&mut con, "configuration.i64", "100")?; + assert_eq!(config_get(&mut con, "configuration.i64")?, "100"); - assert_eq!(config_get("configuration.atomic_i64")?, "10"); - config_set("configuration.atomic_i64", "100")?; - assert_eq!(config_get("configuration.atomic_i64")?, "100"); + assert_eq!(config_get(&mut con, "configuration.atomic_i64")?, "10"); + config_set(&mut con, "configuration.atomic_i64", "100")?; + assert_eq!(config_get(&mut con, "configuration.atomic_i64")?, "100"); - assert_eq!(config_get("configuration.redis_string")?, "default"); - config_set("configuration.redis_string", "new")?; - assert_eq!(config_get("configuration.redis_string")?, "new"); + assert_eq!( + config_get(&mut con, "configuration.redis_string")?, + "default" + ); + config_set(&mut con, "configuration.redis_string", "new")?; + assert_eq!(config_get(&mut con, "configuration.redis_string")?, "new"); - assert_eq!(config_get("configuration.string")?, "default"); - config_set("configuration.string", "new")?; - assert_eq!(config_get("configuration.string")?, "new"); + assert_eq!(config_get(&mut con, "configuration.string")?, "default"); + config_set(&mut con, "configuration.string", "new")?; + assert_eq!(config_get(&mut con, "configuration.string")?, "new"); - assert_eq!(config_get("configuration.mutex_string")?, "default"); - config_set("configuration.mutex_string", "new")?; - assert_eq!(config_get("configuration.mutex_string")?, "new"); + assert_eq!( + config_get(&mut con, "configuration.mutex_string")?, + "default" + ); + config_set(&mut con, "configuration.mutex_string", "new")?; + assert_eq!(config_get(&mut con, "configuration.mutex_string")?, "new"); - assert_eq!(config_get("configuration.atomic_bool")?, "yes"); - config_set("configuration.atomic_bool", "no")?; - assert_eq!(config_get("configuration.atomic_bool")?, "no"); + assert_eq!(config_get(&mut con, "configuration.atomic_bool")?, "yes"); + config_set(&mut con, "configuration.atomic_bool", "no")?; + assert_eq!(config_get(&mut con, "configuration.atomic_bool")?, "no"); - assert_eq!(config_get("configuration.bool")?, "yes"); - config_set("configuration.bool", "no")?; - assert_eq!(config_get("configuration.bool")?, "no"); + assert_eq!(config_get(&mut con, "configuration.bool")?, "yes"); + config_set(&mut con, "configuration.bool", "no")?; + assert_eq!(config_get(&mut con, "configuration.bool")?, "no"); - assert_eq!(config_get("configuration.enum")?, "Val1"); - config_set("configuration.enum", "Val2")?; - assert_eq!(config_get("configuration.enum")?, "Val2"); + assert_eq!(config_get(&mut con, "configuration.enum")?, "Val1"); + config_set(&mut con, "configuration.enum", "Val2")?; + assert_eq!(config_get(&mut con, "configuration.enum")?, "Val2"); - assert_eq!(config_get("configuration.enum_mutex")?, "Val1"); - config_set("configuration.enum_mutex", "Val2")?; - assert_eq!(config_get("configuration.enum_mutex")?, "Val2"); + assert_eq!(config_get(&mut con, "configuration.enum_mutex")?, "Val1"); + config_set(&mut con, "configuration.enum_mutex", "Val2")?; + assert_eq!(config_get(&mut con, "configuration.enum_mutex")?, "Val2"); - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; let res: i64 = redis::cmd("configuration.num_changes") .query(&mut con) .with_context(|| "failed to run flushall")?; @@ -534,11 +503,7 @@ fn test_configuration() -> Result<()> { #[test] fn test_response() -> Result<()> { - let port: u16 = 6496; - let _guards = vec![start_redis_server_with_module("response", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("response"); redis::cmd("hset") .arg(&["k", "a", "b", "c", "d", "e", "b", "f", "g"]) @@ -566,11 +531,7 @@ fn test_response() -> Result<()> { #[test] fn test_command_proc_macro() -> Result<()> { - let port: u16 = 6497; - let _guards = vec![start_redis_server_with_module("proc_macro_commands", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("proc_macro_commands"); let res: Vec = redis::cmd("COMMAND") .arg(&["GETKEYS", "classic_keys", "x", "foo", "y", "bar"]) @@ -605,11 +566,7 @@ fn test_command_proc_macro() -> Result<()> { #[test] fn test_redis_value_derive() -> Result<()> { - let port: u16 = 6498; - let _guards = vec![start_redis_server_with_module("proc_macro_commands", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("proc_macro_commands"); let res: Value = redis::cmd("redis_value_derive") .query(&mut con) @@ -629,11 +586,7 @@ fn test_redis_value_derive() -> Result<()> { #[test] fn test_call_blocking() -> Result<()> { - let port: u16 = 6499; - let _guards = vec![start_redis_server_with_module("call", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("call"); let res: Option = redis::cmd("call.blocking") .query(&mut con) @@ -652,11 +605,7 @@ fn test_call_blocking() -> Result<()> { #[test] fn test_open_key_with_flags() -> Result<()> { - let port: u16 = 6501; - let _guards = vec![start_redis_server_with_module("open_key_with_flags", port) - .with_context(|| "failed to start redis server")?]; - let mut con = - get_redis_connection(port).with_context(|| "failed to connect to redis server")?; + let mut con = TestConnection::new("open_key_with_flags"); // Avoid active expriation redis::cmd("DEBUG") From 758b11af13f7dc6b06661d8e6ddbf75164467d80 Mon Sep 17 00:00:00 2001 From: Victor Polevoy Date: Mon, 27 May 2024 10:01:46 +0200 Subject: [PATCH 2/3] Remove the unsafe and mut from the atomic TEST_PORT --- tests/integration.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index de6f4db5..e84cc613 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -21,17 +21,15 @@ struct TestConnection { connection: Connection, } -static mut TEST_PORT: AtomicU16 = AtomicU16::new(6479); +static TEST_PORT: AtomicU16 = AtomicU16::new(6479); impl TestConnection { fn new(module_name: &str) -> Self { - unsafe { - let port = TEST_PORT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + let port = TEST_PORT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - Self { - _guards: start_redis(module_name, port).expect("Redis instance started."), - connection: get_redis_connection(port).expect("Established connection to server."), - } + Self { + _guards: start_redis(module_name, port).expect("Redis instance started."), + connection: get_redis_connection(port).expect("Established connection to server."), } } } From 0868f37692640e7add1a3b7797321115cd23962f Mon Sep 17 00:00:00 2001 From: Victor Polevoy Date: Mon, 27 May 2024 16:44:13 +0200 Subject: [PATCH 3/3] Move the TestConnection from integration to the utils module. --- tests/integration.rs | 43 ++----------------------------------------- tests/utils.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index e84cc613..1cc5716e 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1,53 +1,14 @@ -use std::sync::atomic::AtomicU16; use std::thread; use std::time::Duration; -use crate::utils::{get_redis_connection, start_redis_server_with_module}; +use crate::utils::{get_redis_connection, start_redis_server_with_module, TestConnection}; use anyhow::Context; use anyhow::Result; -use redis::{Connection, RedisError, RedisResult, Value}; +use redis::{RedisError, RedisResult, Value}; use redis_module::RedisValue; -use utils::ChildGuard; mod utils; -fn start_redis(module_name: &str, port: u16) -> Result, &'static str> { - Ok(vec![start_redis_server_with_module(module_name, port) - .map_err(|_| "failed to start redis server")?]) -} - -struct TestConnection { - _guards: Vec, - connection: Connection, -} - -static TEST_PORT: AtomicU16 = AtomicU16::new(6479); - -impl TestConnection { - fn new(module_name: &str) -> Self { - let port = TEST_PORT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - - Self { - _guards: start_redis(module_name, port).expect("Redis instance started."), - connection: get_redis_connection(port).expect("Established connection to server."), - } - } -} - -impl std::ops::Deref for TestConnection { - type Target = Connection; - - fn deref(&self) -> &Self::Target { - &self.connection - } -} - -impl std::ops::DerefMut for TestConnection { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.connection - } -} - #[test] fn test_hello() -> Result<()> { let mut con = TestConnection::new("hello"); diff --git a/tests/utils.rs b/tests/utils.rs index 76e2e859..c8aac648 100644 --- a/tests/utils.rs +++ b/tests/utils.rs @@ -4,9 +4,53 @@ use redis::Connection; use std::fs; use std::path::PathBuf; use std::process::Command; +use std::sync::atomic::AtomicU16; use std::time::Duration; +/// Starts a redis instance with the module provided as a module name +/// and a port, returns the connection guards (`ChildGuard`) through +/// which the redis instance can be interacted with. +pub fn start_redis(module_name: &str, port: u16) -> Result, &'static str> { + Ok(vec![start_redis_server_with_module(module_name, port) + .map_err(|_| "failed to start redis server")?]) +} + +pub struct TestConnection { + _guards: Vec, + connection: Connection, +} + +static TEST_PORT: AtomicU16 = AtomicU16::new(6479); + +impl TestConnection { + /// Creates a new connection to a Redis server with the module + /// provided as a module name. + pub fn new(module_name: &str) -> Self { + let port = TEST_PORT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + + Self { + _guards: start_redis(module_name, port).expect("Redis instance started."), + connection: get_redis_connection(port).expect("Established connection to server."), + } + } +} + +impl std::ops::Deref for TestConnection { + type Target = Connection; + + fn deref(&self) -> &Self::Target { + &self.connection + } +} + +impl std::ops::DerefMut for TestConnection { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.connection + } +} + /// Ensure child process is killed both on normal exit and when panicking due to a failed test. +#[derive(Debug)] pub struct ChildGuard { name: &'static str, child: std::process::Child,