From ce5f8540d33bd4e74cc807743a6e27cd894bca4a Mon Sep 17 00:00:00 2001 From: avifenesh Date: Sun, 9 Feb 2025 18:49:07 +0000 Subject: [PATCH] Update dependencies and improve error handling in Redis and cluster management Signed-off-by: avifenesh --- glide-core/redis-rs/redis/Cargo.toml | 3 +- .../redis-rs/redis/tests/support/cluster.rs | 8 +- .../redis-rs/redis/tests/support/mod.rs | 30 ++++--- node/.ort.yml | 11 +-- .../commonjs-test/commonjs-test.cjs | 88 +++++++++--------- .../commonjs-test/package.json | 11 +-- .../ecmascript-test/ecmascript-test.mjs | 84 +++++++++-------- .../ecmascript-test/package.json | 9 +- node/npm/glide/tests/ExportedSymbols.test.ts | 8 +- node/package.json | 3 +- node/tests/AsyncClient.test.ts | 54 ----------- node/tests/GlideClusterClient.test.ts | 10 ++- node/tests/TestUtilities.ts | 89 +++++++++++++++++-- utils/TestUtils.ts | 6 +- utils/cluster_manager.py | 5 +- 15 files changed, 227 insertions(+), 192 deletions(-) delete mode 100644 node/tests/AsyncClient.test.ts diff --git a/glide-core/redis-rs/redis/Cargo.toml b/glide-core/redis-rs/redis/Cargo.toml index 534b59fc81..abcfacd381 100644 --- a/glide-core/redis-rs/redis/Cargo.toml +++ b/glide-core/redis-rs/redis/Cargo.toml @@ -179,8 +179,9 @@ tempfile = "=3.6.0" once_cell = "1" anyhow = "1" sscanf = "0.4.1" -serial_test = "2" +serial_test = "^2" versions = "6.3" +which = "7.0.1" [[test]] name = "test_async" diff --git a/glide-core/redis-rs/redis/tests/support/cluster.rs b/glide-core/redis-rs/redis/tests/support/cluster.rs index 991331cfca..50ec2d3441 100644 --- a/glide-core/redis-rs/redis/tests/support/cluster.rs +++ b/glide-core/redis-rs/redis/tests/support/cluster.rs @@ -217,7 +217,13 @@ impl RedisCluster { )); } - let mut cmd = process::Command::new("redis-cli"); + let cli_command = ["valkey-cli", "redis-cli"] + .iter() + .find(|cmd| which::which(cmd).is_ok()) + .map(|&cmd| cmd) + .unwrap_or_else(|| panic!("Neither valkey-cli nor redis-cli exists in the system.")); + + let mut cmd = process::Command::new(cli_command); cmd.stdout(process::Stdio::null()) .arg("--cluster") .arg("create") diff --git a/glide-core/redis-rs/redis/tests/support/mod.rs b/glide-core/redis-rs/redis/tests/support/mod.rs index 72dc7c9a78..cb4ad4d1d1 100644 --- a/glide-core/redis-rs/redis/tests/support/mod.rs +++ b/glide-core/redis-rs/redis/tests/support/mod.rs @@ -225,17 +225,25 @@ impl RedisServer { modules: &[Module], spawner: F, ) -> RedisServer { - let mut redis_cmd = process::Command::new("redis-server"); + // Check wether the server available is redis or valkey + let server_command = ["valkey-server", "redis-server"] + .iter() + .find(|cmd| which::which(cmd).is_ok()) + .map(|&cmd| cmd) + .unwrap_or_else(|| { + panic!("Neither valkey-server nor redis-server exists in the system.") + }); + let mut server_cmd = process::Command::new(server_command); if let Some(config_path) = config_file { - redis_cmd.arg(config_path); + server_cmd.arg(config_path); } // Load Redis Modules for module in modules { match module { Module::Json => { - redis_cmd + server_cmd .arg("--loadmodule") .arg(env::var("REDIS_RS_REDIS_JSON_PATH").expect( "Unable to find path to RedisJSON at REDIS_RS_REDIS_JSON_PATH, is it set?", @@ -244,24 +252,24 @@ impl RedisServer { }; } - redis_cmd + server_cmd .stdout(process::Stdio::null()) .stderr(process::Stdio::null()); let tempdir = tempfile::Builder::new() .prefix("redis") .tempdir() .expect("failed to create tempdir"); - redis_cmd.arg("--logfile").arg(Self::log_file(&tempdir)); + server_cmd.arg("--logfile").arg(Self::log_file(&tempdir)); match addr { redis::ConnectionAddr::Tcp(ref bind, server_port) => { - redis_cmd + server_cmd .arg("--port") .arg(server_port.to_string()) .arg("--bind") .arg(bind); RedisServer { - process: spawner(&mut redis_cmd), + process: spawner(&mut server_cmd), tempdir, addr, tls_paths: None, @@ -273,7 +281,7 @@ impl RedisServer { let auth_client = if mtls_enabled { "yes" } else { "no" }; // prepare redis with TLS - redis_cmd + server_cmd .arg("--tls-port") .arg(port.to_string()) .arg("--port") @@ -300,20 +308,20 @@ impl RedisServer { }; RedisServer { - process: spawner(&mut redis_cmd), + process: spawner(&mut server_cmd), tempdir, addr, tls_paths: Some(tls_paths), } } redis::ConnectionAddr::Unix(ref path) => { - redis_cmd + server_cmd .arg("--port") .arg("0") .arg("--unixsocket") .arg(path); RedisServer { - process: spawner(&mut redis_cmd), + process: spawner(&mut server_cmd), tempdir, addr, tls_paths: None, diff --git a/node/.ort.yml b/node/.ort.yml index 720b517e45..2adcbe1694 100644 --- a/node/.ort.yml +++ b/node/.ort.yml @@ -8,11 +8,6 @@ excludes: reason: "BUILD_TOOL_OF" comment: "Template file for CD" -resolutions: - issues: - - message: "NPM failed to resolve dependencies for path 'hybrid-node-tests/commonjs-test/package.json': FileNotFoundException: .*valkey-glide/package.json.*" - reason: "BUILD_TOOL_ISSUE" - comment: "The 'hybrid-node-tests' are not built, so there is no 'package.json' in the expected location. The 'valkey-glide' package has already been scanned for licenses, so there's no need to scan it again." - - message: "NPM failed to resolve dependencies for path 'hybrid-node-tests/ecmascript-test/package.json': FileNotFoundException: .*valkey-glide/package.json.*" - reason: "BUILD_TOOL_ISSUE" - comment: "The 'hybrid-node-tests' are not built, so there is no 'package.json' in the expected location. The 'valkey-glide' package has already been scanned for licenses, so there's no need to scan it again." + - pattern: "hybrid-node-tests/**" + reason: "TEST_OF" + comment: "Test files for hybrid node tests" diff --git a/node/hybrid-node-tests/commonjs-test/commonjs-test.cjs b/node/hybrid-node-tests/commonjs-test/commonjs-test.cjs index 45b4ff731a..d12a6d6349 100644 --- a/node/hybrid-node-tests/commonjs-test/commonjs-test.cjs +++ b/node/hybrid-node-tests/commonjs-test/commonjs-test.cjs @@ -1,57 +1,57 @@ /* eslint no-undef: off */ /* eslint @typescript-eslint/no-require-imports: off */ -const { AsyncClient } = require("glide-rs"); -const RedisServer = require("redis-server"); +const { GlideClient } = require("@valkey/valkey-glide"); const FreePort = require("find-free-port"); -const { execFile } = require("child_process"); +const { checkWhichCommandAvailable } = require("../../tests/TestUtilities"); const PORT_NUMBER = 4001; -let server; -let port; - -function flushallOnPort(port) { - return new Promise((resolve, reject) => { - execFile("redis-cli", ["-p", port, "FLUSHALL"], (error, _, stderr) => { - if (error) { - console.error(stderr); - reject(error); - } else { + +async function main() { + console.log("Starting main"); + const port = await FreePort(PORT_NUMBER).then(([free_port]) => free_port); + const server = await checkWhichCommandAvailable( + "valkey-server", + "redis-server", + ); + + const serverProcess = spawn(server, ["--port", port.toString()], { + stdio: ["ignore", "pipe", "pipe"], + }); + + await new Promise((resolve) => { + serverProcess.stdout.on("data", (data) => { + console.log(`${data}`); + + if (data.toString().includes("Ready to accept connections")) { resolve(); } }); + + serverProcess.stderr.on("data", (data) => { + console.error(`${data}`); + }); }); -} -FreePort(PORT_NUMBER) - .then(([free_port]) => { - port = free_port; - server = new RedisServer(port); - server.open(async (err) => { - if (err) { - console.error("Error opening server:", err); - throw err; - } + const client = await GlideClient.createClient({ + addresses: [{ host: "localhost", port }], + }); + const setResult = await client.set("test", "test"); + console.log(setResult); + let getResult = await client.get("test"); + console.log(getResult); - const client = AsyncClient.CreateConnection( - `redis://localhost:${port}`, - ); - await client.set("test", "test"); - let result = await client.get("test"); + if (getResult !== "test") { + throw new Error("Common Test failed"); + } else { + console.log("Common Test passed"); + } - if (result !== "test") { - throw new Error("Common Test failed"); - } else { - console.log("Common Test passed"); - } + await client.flushall(); + client.close(); + serverProcess.kill(); +} - await flushallOnPort(port).then(() => { - console.log("db flushed"); - }); - await server.close().then(() => { - console.log("server closed"); - }); - }); - }) - .catch((error) => { - console.error("Error occurred while finding a free port:", error); - }); +main().then(() => { + console.log("Done"); + process.exit(0); +}); diff --git a/node/hybrid-node-tests/commonjs-test/package.json b/node/hybrid-node-tests/commonjs-test/package.json index 6e1291a680..362fe7675f 100644 --- a/node/hybrid-node-tests/commonjs-test/package.json +++ b/node/hybrid-node-tests/commonjs-test/package.json @@ -2,7 +2,7 @@ "name": "common-test", "version": "1.0.0", "description": "", - "main": "Common-test.cjs", + "main": "commonjs-test.cjs", "type": "commonjs", "scripts": { "build": "cd ../../../node && npm run build", @@ -14,13 +14,10 @@ "author": "Amazon Web Services", "license": "Apache-2.0", "dependencies": { - "child_process": "^1.0.2", - "find-free-port": "^2.0.0", - "valkey-glide": "file:../../../node/build-ts/cjs" + "@valkey/valkey-glide": "file:../..", + "find-free-port": "^2.0.0" }, "devDependencies": { - "@types/node": "^22.8", - "prettier": "^3.3", - "typescript": "^5.6" + "prettier": "^3.5" } } diff --git a/node/hybrid-node-tests/ecmascript-test/ecmascript-test.mjs b/node/hybrid-node-tests/ecmascript-test/ecmascript-test.mjs index 1668b88668..49857b79ac 100644 --- a/node/hybrid-node-tests/ecmascript-test/ecmascript-test.mjs +++ b/node/hybrid-node-tests/ecmascript-test/ecmascript-test.mjs @@ -1,50 +1,60 @@ /* eslint no-undef: off */ -import { execFile } from "child_process"; +import { spawn } from "child_process"; import findFreePorts from "find-free-ports"; -import { AsyncClient } from "glide-rs"; -import RedisServer from "redis-server"; +import { GlideClient } from "@valkey/valkey-glide"; +import { checkWhichCommandAvailable } from "../../tests/TestUtilities"; const PORT_NUMBER = 4001; -let server; -let port; - -function flushallOnPort(port) { - return new Promise((resolve, reject) => { - execFile("redis-cli", ["-p", port, "FLUSHALL"], (error, _, stderr) => { - if (error) { - console.error(stderr); - reject(error); - } else { + +async function main() { + console.log("Starting main"); + const port = await findFreePorts(PORT_NUMBER).then( + ([free_port]) => free_port, + ); + const server = await checkWhichCommandAvailable( + "valkey-server", + "redis-server", + ); + + const serverProcess = spawn(server, ["--port", port.toString()], { + stdio: ["ignore", "pipe", "pipe"], + }); + + await new Promise((resolve) => { + serverProcess.stdout.on("data", (data) => { + console.log(`${data}`); + + if (data.toString().includes("Ready to accept connections")) { resolve(); } }); - }); -} -port = await findFreePorts(PORT_NUMBER).then(([free_port]) => free_port); -server = await new Promise((resolve, reject) => { - const server = new RedisServer(port); - server.open(async (err) => { - if (err) { - reject(err); - } + serverProcess.stderr.on("data", (data) => { + console.error(`${data}`); + }); + }); - resolve(server); + const client = await GlideClient.createClient({ + addresses: [{ host: "localhost", port }], }); -}); -const client = AsyncClient.CreateConnection("redis://localhost:" + port); -await client.set("test", "test"); -let result = await client.get("test"); - -if (result !== "test") { - throw new Error("Ecma Test failed"); -} else { - console.log("Ecma Test passed"); + const setResult = await client.set("test", "test"); + console.log(setResult); + let getResult = await client.get("test"); + console.log(getResult); + + if (getResult !== "test") { + throw new Error("Ecma Test failed"); + } else { + console.log("Ecma Test passed"); + } + + await client.flushall(); + client.close(); + serverProcess.kill(); + console.log("Done"); } -await flushallOnPort(port).then(() => { - console.log("db flushed"); -}); -await server.close().then(() => { - console.log("server closed"); +main().catch((err) => { + console.error(err); + process.exit(1); }); diff --git a/node/hybrid-node-tests/ecmascript-test/package.json b/node/hybrid-node-tests/ecmascript-test/package.json index d1d91d5a97..6c14a152ad 100644 --- a/node/hybrid-node-tests/ecmascript-test/package.json +++ b/node/hybrid-node-tests/ecmascript-test/package.json @@ -14,13 +14,10 @@ "prettier:format": "./node_modules/.bin/prettier --write . " }, "dependencies": { - "child_process": "^1.0.2", - "find-free-ports": "^3.1.1", - "valkey-glide": "file:../../../node/build-ts/mjs" + "@valkey/valkey-glide": "file:../..", + "find-free-ports": "^3.1.1" }, "devDependencies": { - "@types/node": "^22.8", - "prettier": "^3.3", - "typescript": "^5.6" + "prettier": "^3.5" } } diff --git a/node/npm/glide/tests/ExportedSymbols.test.ts b/node/npm/glide/tests/ExportedSymbols.test.ts index 7cd4220f15..3d18ae43e5 100644 --- a/node/npm/glide/tests/ExportedSymbols.test.ts +++ b/node/npm/glide/tests/ExportedSymbols.test.ts @@ -49,11 +49,11 @@ describe("Validation of Exported Symbols", () => { for (const file of filesWithNodeCode) { const sourceCode = await f.readFile(file, "utf8"); - const sourceFile = await ts.createSourceFile( + const sourceFile = ts.createSourceFile( file, sourceCode, ts.ScriptTarget.Latest, - true, + true ); internallyExported.push(...visitRoot(sourceFile)); } @@ -74,7 +74,7 @@ describe("Validation of Exported Symbols", () => { if (missingSymbols.length > 0) { console.log( "The following symbols are exported from npm/glide package but missing " + - "from the internal node package export. These symbols might be from glide-rs package", + "from the internal node package export. These symbols might be from glide-rs package", ); console.log(missingSymbols); } @@ -120,7 +120,7 @@ async function getFiles(folderName: string): Promise { function visitRoot(root: ts.Node) { // (Root Level)->(Level 1) - const children: ts.Node[] = root.getChildren(); + const children: readonly ts.Node[] = root.getChildren(); const resultList: string[] = []; // (Root Level) -> (Level 1) -> Level 2. This is the level in the AST where all the exported symbols in a file are present. diff --git a/node/package.json b/node/package.json index 93b701ad30..3163c69d25 100644 --- a/node/package.json +++ b/node/package.json @@ -56,9 +56,8 @@ "jest": "^29.7.0", "jest-html-reporter": "^3.10.2", "protobufjs-cli": "^1.1.3", - "redis-server": "^1.2.2", "replace": "^1.2.2", - "semver": "^7.6.3", + "semver": "7.7.1", "ts-jest": "^29.2.5", "ts-node": "^10.9.2", "typescript": "^5.6.3", diff --git a/node/tests/AsyncClient.test.ts b/node/tests/AsyncClient.test.ts deleted file mode 100644 index c810b07f91..0000000000 --- a/node/tests/AsyncClient.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 - */ - -import { afterAll, afterEach, beforeAll, describe } from "@jest/globals"; -import { AsyncClient } from "glide-rs"; -import RedisServer from "redis-server"; -import { runCommonTests } from "./SharedTests"; -import { flushallOnPort } from "./TestUtilities"; -/* eslint-disable @typescript-eslint/no-require-imports */ -const FreePort = require("find-free-port"); - -const PORT_NUMBER = 4000; - -describe("AsyncClient", () => { - let server: RedisServer; - let port: number; - beforeAll(async () => { - port = await FreePort(PORT_NUMBER).then( - ([free_port]: number[]) => free_port, - ); - server = await new Promise((resolve, reject) => { - const server = new RedisServer(port); - server.open(async (err: Error | null) => { - if (err) { - reject(err); - } - - resolve(server); - }); - }); - }); - - afterEach(async () => { - await flushallOnPort(port); - }); - - afterAll(async () => { - await server.close(); - }); - - runCommonTests({ - init: async () => { - const client = await AsyncClient.CreateConnection( - "redis://localhost:" + port, - ); - - return { client, context: {} }; - }, - close: () => { - // GC takes care of dropping the object - }, - }); -}); diff --git a/node/tests/GlideClusterClient.test.ts b/node/tests/GlideClusterClient.test.ts index 0de8f5e15e..525553be20 100644 --- a/node/tests/GlideClusterClient.test.ts +++ b/node/tests/GlideClusterClient.test.ts @@ -22,6 +22,7 @@ import { FunctionStatsSingleResponse, GeoUnit, GlideClusterClient, + GlideRecord, GlideReturnType, GlideString, InfoOptions, @@ -332,7 +333,7 @@ describe("GlideClusterClient", () => { "cluster-node-timeout": "16000", }) .configGet(["timeout", "cluster-node-timeout"]); - const result = await client.exec(transaction); + const result = await client.exec(transaction) as GlideRecord[]; const convertedResult = [ result[0], convertGlideRecordToRecord(result[1]), @@ -1802,7 +1803,12 @@ describe("GlideClusterClient", () => { decoder: Decoder.Bytes, route: route, }); - const data = result?.[0] as Buffer; + + if (!result) { + throw new Error("Transaction failed: result is null"); + } + + const data = result[0] as Buffer; // Verify functionRestore transaction = new ClusterTransaction() diff --git a/node/tests/TestUtilities.ts b/node/tests/TestUtilities.ts index 234e82f259..6c7eca4b9e 100644 --- a/node/tests/TestUtilities.ts +++ b/node/tests/TestUtilities.ts @@ -4,7 +4,10 @@ import { expect } from "@jest/globals"; import { exec } from "child_process"; +import { promisify } from "util"; +const execAsync = promisify(exec); import { v4 as uuidv4 } from "uuid"; +import { Socket } from "net"; import { BaseClient, BaseClientConfiguration, @@ -183,6 +186,25 @@ export interface Client { get: (key: string) => Promise; } +export async function checkWhichCommandAvailable( + valkeyCommand: string, + redisCommand: string, +): Promise { + try { + if (await checkCommandAvailability(valkeyCommand)) { + return valkeyCommand; + } + } catch { + // ignore + } + + if (await checkCommandAvailability(redisCommand)) { + return redisCommand; + } + + throw new Error("No available command found."); +} + export async function GetAndSetRandomValue(client: Client) { const key = uuidv4(); // Adding random repetition, to prevent the inputs from always having the same alignment. @@ -193,17 +215,21 @@ export async function GetAndSetRandomValue(client: Client) { expect(intoString(result)).toEqual(value); } -export function flushallOnPort(port: number): Promise { - return new Promise((resolve, reject) => - exec(`redis-cli -p ${port} FLUSHALL`, (error, _, stderr) => { +export async function flushallOnPort(port: number): Promise { + try { + const command = await checkWhichCommandAvailable( + "valkey-cli", + "redis-cli", + ); + exec(`${command} -p ${port} flushall`, (error) => { if (error) { - console.error(stderr); - reject(error); - } else { - resolve(); + console.error(`exec error: ${error}`); + return; } - }), - ); + }); + } catch (error) { + console.error(`Error flushing on port ${port}: ${error}`); + } } /** @@ -2106,3 +2132,48 @@ export async function getServerVersion( return version; } + +/** + * Check if a command is available on the system + */ +export function checkCommandAvailability(command: string): Promise { + return new Promise((resolve) => { + exec(`which ${command}`, (error) => { + resolve(!error); + }); + }); +} + +/** + * Starts a Valkey/Redis-compatible server on the specified port + */ +export async function startServer( + port: number, +): Promise<{ process: any; command: string }> { + // check which command is available + const serverCmd = await checkWhichCommandAvailable( + "valkey-server", + "redis-server", + ); + // run server, and wait for it to start + const serverProcess = await execAsync(`${serverCmd} --port ${port}`) + .then(async (process) => { + // wait for the server to start + await new Promise((resolve) => setTimeout(resolve, 1000)); + // check if the server is running by connecting to it using a socket + const client = new Socket(); + await new Promise((resolve) => { + client.connect(port, "localhost", () => { + client.end(); + resolve(); + }); + }); + return process; + }) + .catch((err) => { + console.error("Failed to start the server:", err); + process.exit(1); + }); + + return { process: serverProcess, command: serverCmd }; +} diff --git a/utils/TestUtils.ts b/utils/TestUtils.ts index 9c89788528..24e836caa6 100644 --- a/utils/TestUtils.ts +++ b/utils/TestUtils.ts @@ -21,9 +21,9 @@ function parseOutput(input: string): { .split(",") .map((address) => address.split(":")) .map((address) => [address[0], Number(address[1])]) as [ - string, - number, - ][]; + string, + number, + ][]; if (clusterFolder === undefined || ports === undefined) { throw new Error(`Insufficient data in input: ${input}`); diff --git a/utils/cluster_manager.py b/utils/cluster_manager.py index 14e251b815..5a9482bb70 100755 --- a/utils/cluster_manager.py +++ b/utils/cluster_manager.py @@ -355,8 +355,7 @@ def get_server_command() -> str: return server except Exception as e: logging.error(f"Error checking {server}: {e}") - raise Exception( - "Neither valkey-server nor redis-server found in the system.") + raise Exception("Neither valkey-server nor redis-server found in the system.") def get_server_version(server_name): result = subprocess.run( @@ -693,7 +692,7 @@ def wait_for_all_topology_views( if output is not None and output.count(f"{server.host}") == len(servers): # Server is ready, get the node's role cmd_args = [ - "redis-cli", + CLI_COMMAND, "-h", server.host, "-p",