Allow endpoint specification for Python and Node test #1375

shohamazon · 2024-05-02T14:22:01Z

Issue #, if available:

Description of changes:
This PR introduces the ability to specify endpoints when running tests for both Python and Node environments.

To run the tests in python using with external endpoints:
pytest --asyncio-mode=auto --cluster-endpoints=host:port,host2:port2 --standalone-endpoints=host1:port

In Node:
npm run test -- --cluster-endpoints=host:port,host2:port2 --standalone-endpoints=host1:port
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Yury-Fridlyand

Do you want us to implement similar stuff for java, C# and go?

node/tests/RedisClient.test.ts

Yury-Fridlyand · 2024-05-02T20:53:31Z

node/tests/RedisClusterClient.test.ts

    beforeAll(async () => {
-        cluster = await RedisCluster.createCluster(true, 3, 0);
+        const args = process.argv.slice(2);


Can you reuse this piece of code by creating a function?

why not using parseArg https://simonplend.com/command-line-argument-parsing-with-node-js-core/

Yury-Fridlyand · 2024-05-02T20:55:15Z

python/python/tests/conftest.py

@@ -71,8 +85,23 @@ def call_before_all_pytests(request):
    """
    tls = request.config.getoption("--tls")
    load_module = request.config.getoption("--load-module")
-    pytest.redis_cluster = RedisCluster(tls, True, load_module=load_module)
-    pytest.standalone_cluster = RedisCluster(tls, False, 1, 1, load_module=load_module)
+    endpoints = request.config.getoption("--cluster-endpoints")


rename to cluster_endpoints

shohamazon · 2024-05-02T20:57:55Z

Do you want us to implement similar stuff for java, C# and go?

Let's first wait for Bar's review for this pr and then decide what we should do

node/tests/RedisClient.test.ts

barshaul

@shohamazon Please keep this PR only for python changes, and open the node PR only after it will be approved.
Instead of having standalone endpoints and cluster endpoints, use single endpoints option and add cluster mode flag, as we do everywhere else. Also it will require passing TLS flag

barshaul · 2024-05-09T10:29:22Z

python/python/tests/conftest.py

+    endpoints = request.config.getoption("--cluster-endpoints")
+    standalone_endpoints = request.config.getoption("--standalone-endpoints")
+
+    if endpoints or standalone_endpoints:


add a comment explaining the endpoints were passed by the caller, not creating clusters internally

barshaul · 2024-05-09T10:30:04Z

python/python/tests/conftest.py

+            pytest.redis_cluster = RedisCluster(tls=tls, addresses=endpoints)
+        if standalone_endpoints:
+            standalone_endpoints = [
+                endpoint.split(":") for endpoint in standalone_endpoints.split(",")


create an helper function parse_endpoints, and raise relevant error if the endpoints aren't parsable as they aren't in the right format

barshaul · 2024-05-09T10:30:33Z

python/python/tests/conftest.py

+
+    if endpoints or standalone_endpoints:
+        if endpoints:
+            endpoints = [endpoint.split(":") for endpoint in endpoints.split(",")]


here also, endpoints -> cluster_endpoints

barshaul · 2024-05-09T10:33:03Z

python/python/tests/conftest.py

+            pytest.standalone_cluster = RedisCluster(
+                tls, addresses=standalone_endpoints
+            )
+    else:


add comment
// Not endpoints were provided, create new clusters

barshaul · 2024-05-09T10:33:41Z

python/python/tests/conftest.py

+            tls, True, load_module=load_module, addresses=endpoints
+        )
+        pytest.standalone_cluster = RedisCluster(
+            tls, False, 1, 1, load_module=load_module, addresses=standalone_endpoints


please add the filed names to make it more readable

barshaul · 2024-05-09T10:35:00Z

python/python/tests/conftest.py

@@ -93,13 +126,37 @@ def pytest_sessionfinish(session, exitstatus):
        pass


+def pytest_collection_modifyitems(config, items):


add documentation what are you doing in this function

barshaul · 2024-05-09T10:36:13Z

python/python/tests/conftest.py

+        ):
+            if "cluster_mode" in item.fixturenames:
+                cluster_mode_value = item.callspec.params.get("cluster_mode", None)
+                if cluster_mode_value and not config.getoption("--cluster-endpoints"):


if cluster_mode_value is True

barshaul · 2024-05-09T10:36:21Z

python/python/tests/conftest.py

+                            reason="Test skipped because cluster_mode=False"
+                        )
+                    )
+                elif not cluster_mode_value and not config.getoption(


elif cluster_mode_value is False

barshaul · 2024-05-09T10:37:01Z

python/python/tests/conftest.py

+                if cluster_mode_value and not config.getoption("--cluster-endpoints"):
+                    item.add_marker(
+                        pytest.mark.skip(
+                            reason="Test skipped because cluster_mode=False"


.. and cluster endpoints weren't provided

barshaul · 2024-05-09T10:37:28Z

python/python/tests/conftest.py

+                ):
+                    item.add_marker(
+                        pytest.mark.skip(
+                            reason="Test skipped because cluster_mode=True"


barshaul · 2024-05-09T10:38:01Z

python/python/tests/conftest.py

 @pytest.fixture()
 async def redis_client(
    request, cluster_mode: bool, protocol: ProtocolVersion
 ) -> AsyncGenerator[TRedisClient, None]:
    "Get async socket client for tests"
    client = await create_client(request, cluster_mode, protocol=protocol)
    yield client
+    await flush_all_on_cluster(request, cluster_mode, protocol)


make sure to put all flush related changes in a different commit

barshaul · 2024-05-09T10:38:32Z

python/python/tests/conftest.py

+    parser.addoption(
+        "--cluster-endpoints",
+        action="store",
+        help="Comma-separated list of cluster endpoints in the format host1:port1,host2:port2,...",


please add a note that the cluster will be flushed (both for standalone)

barshaul · 2024-05-09T10:39:14Z

python/python/tests/conftest.py

        )
        return await RedisClient.create(config)
+
+
+async def flush_all_on_cluster(request, cluster_mode: bool, protocol: ProtocolVersion):


flush_all_on_cluster -> teardown

barshaul · 2024-05-09T10:40:35Z

python/python/tests/conftest.py

+
+
+async def flush_all_on_cluster(request, cluster_mode: bool, protocol: ProtocolVersion):
+    client = await create_client(request, cluster_mode, protocol=protocol, timeout=2000)


add a comment why are we creating a new client

barshaul · 2024-05-09T10:41:34Z

python/python/tests/test_async_client.py

@@ -259,7 +259,8 @@ async def test_can_connect_with_auth_acl(
            # Delete this user
            await redis_client.custom_command(["ACL", "DELUSER", username])

-    async def test_select_standalone_database_id(self, request):
+    @pytest.mark.parametrize("cluster_mode", [False])
+    async def test_select_standalone_database_id(self, request, cluster_mode):
        redis_client = await create_client(request, cluster_mode=False, database_id=4)


cluster_mode=False -> can be changed to cluster_mode

barshaul · 2024-05-09T10:42:38Z

python/python/tests/utils/cluster.py

@@ -65,7 +69,16 @@ def parse_cluster_script_start_output(self, output: str):
                    nodes_list.append(NodeAddress(host, int(port)))
                self.nodes_addr = nodes_list

+    def existing_cluster(self, addresses: List[List[str]]):


existing_cluster -> init_from_existing_cluster

barshaul · 2024-05-09T10:45:26Z

python/python/tests/utils/cluster.py

+        self.cluster_folder = ""
+        self.nodes_addr = []
+        for [host, port] in addresses:
+            print("host, port ", host, port)


barshaul · 2024-05-09T10:46:36Z

python/python/tests/utils/cluster.py

+        if self.cluster_folder == "":
+            return
        args_list = [sys.executable, SCRIPT_FILE]
        if self.tls:
            args_list.append("--tls")


Suggested change

if self.cluster_folder == "":

return

args_list = [sys.executable, SCRIPT_FILE]

if self.tls:

args_list.append("--tls")

if self.cluster_folder:

args_list = [sys.executable, SCRIPT_FILE]

if self.tls:

args_list.append("--tls")

node/tests/RedisClusterClient.test.ts

barshaul · 2024-05-09T10:55:42Z

node/tests/RedisClusterClient.test.ts

@@ -50,12 +77,12 @@ describe("RedisClusterClient", () => {
    });

    const getOptions = (
-        ports: number[],
+        host_port: [string, number][],


host_port -> address

barshaul · 2024-05-09T10:59:50Z

node/tests/TestUtilities.ts

@@ -40,6 +40,17 @@ export function flushallOnPort(port: number): Promise<void> {
    );
 }

+export const parseEndpoints = (endpoints: string): [string, number][] => {
+    if (!endpoints.trim()) {


why? what does this scenario mean? if endpoints is an empty string, please add appropriate check

barshaul · 2024-05-09T11:00:30Z

node/tests/TestUtilities.ts

+    }
+
+    return endpoints.split(",").map((endpoint) => {
+        const [host, port] = endpoint.split(":");


handle errors and raise appropriate error if recieved format is bad

barshaul · 2024-05-09T11:00:50Z

node/tests/TestUtilities.ts

+
+    return endpoints.split(",").map((endpoint) => {
+        const [host, port] = endpoint.split(":");
+        return [host, Number(port)];


same - Number(port) can raise an error, make sure to handle those

node/tests/RedisClusterClient.test.ts

barshaul · 2024-05-09T11:04:15Z

node/tests/TestUtilities.ts


-    private constructor(ports: number[], clusterFolder: string) {
-        this.usedPorts = ports;
+    private constructor(ports: [string, number][], clusterFolder?: string) {


rename 'ports' to 'addresses'

barshaul · 2024-05-09T11:04:53Z

node/tests/TestUtilities.ts

-    private constructor(ports: number[], clusterFolder: string) {
-        this.usedPorts = ports;
+    private constructor(ports: [string, number][], clusterFolder?: string) {
+        this.address = ports;


can't we get multiple addresses? change to this.addresses if so

barshaul · 2024-05-09T11:05:35Z

node/tests/TestUtilities.ts

-            .map((address) => address.split(":")[1])
-            .map((port) => Number(port));
+            .map((address) => address.split(":"))
+            .map((port) => [port[0], Number(port[1])]) as [string, number][];


Suggested change

.map((port) => [port[0], Number(port[1])]) as [string, number][];

.map((address) => [address[0], Number(address[1])]) as [string, number][];

barshaul · 2024-05-09T11:05:54Z

node/tests/TestUtilities.ts

@@ -295,23 +306,33 @@ export class RedisCluster {
        });
    }

+    public static connectToCluster(port: [string, number][]): RedisCluster {


Suggested change

public static connectToCluster(port: [string, number][]): RedisCluster {

public static connectToCluster(address: [string, number][]): RedisCluster {

are we connecting to the cluster (e.g. creating a client), or just creating an cluster object ?
if not, change to initFromExistingCluster

barshaul · 2024-05-09T11:08:35Z

node/tests/TestUtilities.ts

-                },
-            ),
-        );
+        if (this.clusterFolder !== "") {


if (this.clusterFolder) isn't good enough?

barshaul

See comments inline

barshaul

fix all before merging

Yury-Fridlyand · 2024-05-13T18:29:25Z

node/tests/TestUtilities.ts

@@ -4,6 +4,7 @@

 import { beforeAll, expect } from "@jest/globals";
 import { exec } from "child_process";
+import parseArgs from "minimist";


Please, update developer.md and/or package.json to include this module

It's updated, look at the pr

I see, need to update dev doc.

shohamazon requested a review from a team as a code owner May 2, 2024 14:22

shohamazon force-pushed the cluste_input branch 3 times, most recently from 9293633 to 3b5801e Compare May 2, 2024 14:50

shohamazon added python node labels May 2, 2024

shohamazon force-pushed the cluste_input branch from 3b5801e to d7242c3 Compare May 2, 2024 14:51

Yury-Fridlyand requested review from acarbonetto and aaron-congo May 2, 2024 18:07

Yury-Fridlyand added the testing label May 2, 2024

Yury-Fridlyand reviewed May 2, 2024

View reviewed changes

barshaul reviewed May 6, 2024

View reviewed changes

node/tests/RedisClient.test.ts Outdated Show resolved Hide resolved

barshaul requested changes May 6, 2024

View reviewed changes

shohamazon force-pushed the cluste_input branch 3 times, most recently from 24a04cc to 325a861 Compare May 8, 2024 11:06

barshaul reviewed May 9, 2024

View reviewed changes

node/tests/RedisClusterClient.test.ts Show resolved Hide resolved

barshaul reviewed May 9, 2024

View reviewed changes

node/tests/RedisClusterClient.test.ts Show resolved Hide resolved

barshaul reviewed May 9, 2024

View reviewed changes

node/tests/TestUtilities.ts Outdated

},

),

);

if (this.clusterFolder !== "") {

Copy link

Contributor

barshaul May 9, 2024

Choose a reason for hiding this comment

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

if (this.clusterFolder) isn't good enough?

barshaul requested changes May 9, 2024

View reviewed changes

barshaul approved these changes May 10, 2024

View reviewed changes

shohamazon force-pushed the cluste_input branch from 2212918 to 4c7dd40 Compare May 12, 2024 13:00

shohamazon added 2 commits May 12, 2024 13:09

Allow endpoint specification for Python and Node test

eadcad4

Python, Node: flush cluster on tests teardown

d07a7fb

shohamazon force-pushed the cluste_input branch from 4c7dd40 to d07a7fb Compare May 12, 2024 13:10

shohamazon merged commit 1b960dc into aws:main May 12, 2024
12 checks passed

shohamazon deleted the cluste_input branch May 12, 2024 13:19

Yury-Fridlyand reviewed May 13, 2024

View reviewed changes

		@@ -93,13 +126,37 @@ def pytest_sessionfinish(session, exitstatus):
		pass


		def pytest_collection_modifyitems(config, items):



		async def flush_all_on_cluster(request, cluster_mode: bool, protocol: ProtocolVersion):
		client = await create_client(request, cluster_mode, protocol=protocol, timeout=2000)

	.map((port) => [port[0], Number(port[1])]) as [string, number][];
	.map((address) => [address[0], Number(address[1])]) as [string, number][];

	public static connectToCluster(port: [string, number][]): RedisCluster {
	public static connectToCluster(address: [string, number][]): RedisCluster {

Allow endpoint specification for Python and Node test #1375

Allow endpoint specification for Python and Node test #1375

Conversation

shohamazon commented May 2, 2024 • edited

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shohamazon commented May 2, 2024

barshaul left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shohamazon commented May 2, 2024 •

edited

barshaul left a comment •

edited

barshaul May 9, 2024 •

edited

barshaul May 9, 2024 •

edited

barshaul May 9, 2024 •

edited

barshaul May 9, 2024 •

edited