Re-enable Graph tests (redis#3287)

Although the Graph module was deprecated, we want to have tests running as long as we keep the client code. Therefore re-enable the Graph tests, by adding to the docker-compose stack an older redis-stack-server image. The Graph tests run only with RESP2. Tweak the CI so that it runs against the latest RC release for now. This way we get all the features in RC release of server, so we can execute all tests, including the ones for hash field expiration. Some test failures brought to light issues with the RESP3 push message invalidation handler, align a bit the code in that area. Some more housekeeping around tests, here and there.
agnesnatasya · Jul 20, 2024 · 398a78c · 398a78c
1 parent 1a07ad2
commit 398a78c
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 65 deletions.
diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml
@@ -24,8 +24,9 @@ permissions:
   contents: read  #  to fetch code (actions/checkout)
 
 env:
-  REDIS_STACK_IMAGE: redis/redis-stack-server:7.4.0-rc1
   CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+  REDIS_IMAGE: redis/redis-stack-server:7.4.0-rc1
+  REDIS_STACK_IMAGE: redis/redis-stack-server:7.4.0-rc1
 
 jobs:
   dependency-audit:

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -7,7 +7,7 @@ services:
   redis:
     image: ${REDIS_IMAGE:-redis:latest}
     container_name: redis-standalone
-    command: redis-server --enable-debug-command yes
+    command: redis-server --enable-debug-command yes --protected-mode no
     ports:
       - 6379:6379
     profiles:
@@ -21,7 +21,7 @@ services:
     container_name: redis-replica
     depends_on:
       - redis
-    command: redis-server --replicaof redis 6379
+    command: redis-server --replicaof redis 6379 --protected-mode no
     ports:
       - 6380:6379
     profiles:
@@ -67,7 +67,7 @@ services:
     container_name: redis-sentinel
     depends_on:
       - redis
-    entrypoint: "/usr/local/bin/redis-sentinel /redis.conf --port 26379"
+    entrypoint: "redis-sentinel /redis.conf --port 26379"
     ports:
       - 26379:26379
     volumes:
@@ -81,7 +81,7 @@ services:
     container_name: redis-sentinel2
     depends_on:
       - redis
-    entrypoint: "/usr/local/bin/redis-sentinel /redis.conf --port 26380"
+    entrypoint: "redis-sentinel /redis.conf --port 26380"
     ports:
       - 26380:26380
     volumes:
@@ -95,7 +95,7 @@ services:
     container_name: redis-sentinel3
     depends_on:
       - redis
-    entrypoint: "/usr/local/bin/redis-sentinel /redis.conf --port 26381"
+    entrypoint: "redis-sentinel /redis.conf --port 26381"
     ports:
       - 26381:26381
     volumes:
@@ -114,3 +114,12 @@ services:
     profiles:
       - standalone
       - all
+
+  redis-stack-graph:
+    image: redis/redis-stack-server:6.2.6-v15
+    container_name: redis-stack-graph
+    ports:
+      - 6480:6379
+    profiles:
+      - standalone
+      - all
diff --git a/dockers/create_cluster.sh b/dockers/create_cluster.sh
@@ -31,7 +31,7 @@ dir /nodes/$PORT
 EOF
 
   set -x
-  /usr/local/bin/redis-server /nodes/$PORT/redis.conf
+  redis-server /nodes/$PORT/redis.conf
   sleep 1
   if [ $? -ne 0 ]; then
     echo "Redis failed to start, exiting."
@@ -40,8 +40,8 @@ EOF
   echo 127.0.0.1:$PORT >> /nodes/nodemap
 done
 if [ -z "${REDIS_PASSWORD}" ]; then
-    echo yes | /usr/local/bin/redis-cli --cluster create `seq -f 127.0.0.1:%g ${START_PORT} ${END_PORT}` --cluster-replicas 1
+    echo yes | redis-cli --cluster create `seq -f 127.0.0.1:%g ${START_PORT} ${END_PORT}` --cluster-replicas 1
 else
-    echo yes | /usr/local/bin/redis-cli -a ${REDIS_PASSWORD} --cluster create `seq -f 127.0.0.1:%g ${START_PORT} ${END_PORT}` --cluster-replicas 1
+    echo yes | redis-cli -a ${REDIS_PASSWORD} --cluster create `seq -f 127.0.0.1:%g ${START_PORT} ${END_PORT}` --cluster-replicas 1
 fi
 tail -f /redis.log
diff --git a/redis/_parsers/resp3.py b/redis/_parsers/resp3.py
@@ -15,7 +15,7 @@ class _RESP3Parser(_RESPBase):
     def __init__(self, socket_read_size):
         super().__init__(socket_read_size)
         self.pubsub_push_handler_func = self.handle_pubsub_push_response
-        self.invalidations_push_handler_func = None
+        self.invalidation_push_handler_func = None
 
     def handle_pubsub_push_response(self, response):
         logger = getLogger("push_response")
@@ -129,7 +129,10 @@ def _read_response(self, disable_decoding=False, push_request=False):
 
     def handle_push_response(self, response, disable_decoding, push_request):
         if response[0] in _INVALIDATION_MESSAGE:
-            res = self.invalidation_push_handler_func(response)
+            if self.invalidation_push_handler_func:
+                res = self.invalidation_push_handler_func(response)
+            else:
+                res = None
         else:
             res = self.pubsub_push_handler_func(response)
         if not push_request:
@@ -142,15 +145,15 @@ def handle_push_response(self, response, disable_decoding, push_request):
     def set_pubsub_push_handler(self, pubsub_push_handler_func):
         self.pubsub_push_handler_func = pubsub_push_handler_func
 
-    def set_invalidation_push_handler(self, invalidations_push_handler_func):
-        self.invalidation_push_handler_func = invalidations_push_handler_func
+    def set_invalidation_push_handler(self, invalidation_push_handler_func):
+        self.invalidation_push_handler_func = invalidation_push_handler_func
 
 
 class _AsyncRESP3Parser(_AsyncRESPBase):
     def __init__(self, socket_read_size):
         super().__init__(socket_read_size)
         self.pubsub_push_handler_func = self.handle_pubsub_push_response
-        self.invalidations_push_handler_func = None
+        self.invalidation_push_handler_func = None
 
     def handle_pubsub_push_response(self, response):
         logger = getLogger("push_response")
@@ -273,7 +276,10 @@ async def _read_response(
 
     async def handle_push_response(self, response, disable_decoding, push_request):
         if response[0] in _INVALIDATION_MESSAGE:
-            res = self.invalidation_push_handler_func(response)
+            if self.invalidation_push_handler_func:
+                res = self.invalidation_push_handler_func(response)
+            else:
+                res = None
         else:
             res = self.pubsub_push_handler_func(response)
         if not push_request:
@@ -286,5 +292,5 @@ async def handle_push_response(self, response, disable_decoding, push_request):
     def set_pubsub_push_handler(self, pubsub_push_handler_func):
         self.pubsub_push_handler_func = pubsub_push_handler_func
 
-    def set_invalidation_push_handler(self, invalidations_push_handler_func):
-        self.invalidation_push_handler_func = invalidations_push_handler_func
+    def set_invalidation_push_handler(self, invalidation_push_handler_func):
+        self.invalidation_push_handler_func = invalidation_push_handler_func
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -135,6 +135,8 @@ def _get_info(redis_url):
 def pytest_sessionstart(session):
     # during test discovery, e.g. with VS Code, we may not
     # have a server running.
+    protocol = session.config.getoption("--protocol")
+    REDIS_INFO["resp_version"] = int(protocol) if protocol else None
     redis_url = session.config.getoption("--redis-url")
     try:
         info = _get_info(redis_url)
@@ -279,6 +281,11 @@ def skip_if_cryptography() -> _TestDecorator:
         return pytest.mark.skipif(False, reason="No cryptography dependency")
 
 
+def skip_if_resp_version(resp_version) -> _TestDecorator:
+    check = REDIS_INFO.get("resp_version", None) == resp_version
+    return pytest.mark.skipif(check, reason=f"RESP version required != {resp_version}")
+
+
 def _get_client(
     cls, request, single_connection_client=True, flushdb=True, from_url=None, **kwargs
 ):

diff --git a/tests/test_asyncio/test_graph.py b/tests/test_asyncio/test_graph.py
@@ -4,23 +4,24 @@
 from redis.commands.graph import Edge, Node, Path
 from redis.commands.graph.execution_plan import Operation
 from redis.exceptions import ResponseError
-from tests.conftest import skip_if_redis_enterprise
+from tests.conftest import skip_if_redis_enterprise, skip_if_resp_version
 
 
 @pytest_asyncio.fixture()
 async def decoded_r(create_redis, stack_url):
-    return await create_redis(decode_responses=True, url=stack_url)
+    return await create_redis(decode_responses=True, url="redis://localhost:6480")
 
 
 @pytest.mark.redismod
+@skip_if_resp_version(3)
 async def test_bulk(decoded_r):
     with pytest.raises(NotImplementedError):
         await decoded_r.graph().bulk()
         await decoded_r.graph().bulk(foo="bar!")
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_graph_creation(decoded_r: redis.Redis):
     graph = decoded_r.graph()
 
@@ -66,7 +67,7 @@ async def test_graph_creation(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_array_functions(decoded_r: redis.Redis):
     graph = decoded_r.graph()
 
@@ -90,7 +91,7 @@ async def test_array_functions(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_path(decoded_r: redis.Redis):
     node0 = Node(node_id=0, label="L1")
     node1 = Node(node_id=1, label="L1")
@@ -111,7 +112,7 @@ async def test_path(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_param(decoded_r: redis.Redis):
     params = [1, 2.3, "str", True, False, None, [0, 1, 2]]
     query = "RETURN $param"
@@ -122,7 +123,7 @@ async def test_param(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_map(decoded_r: redis.Redis):
     query = "RETURN {a:1, b:'str', c:NULL, d:[1,2,3], e:True, f:{x:1, y:2}}"
 
@@ -140,7 +141,7 @@ async def test_map(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_point(decoded_r: redis.Redis):
     query = "RETURN point({latitude: 32.070794860, longitude: 34.820751118})"
     expected_lat = 32.070794860
@@ -158,7 +159,7 @@ async def test_point(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_index_response(decoded_r: redis.Redis):
     result_set = await decoded_r.graph().query("CREATE INDEX ON :person(age)")
     assert 1 == result_set.indices_created
@@ -174,7 +175,7 @@ async def test_index_response(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_stringify_query_result(decoded_r: redis.Redis):
     graph = decoded_r.graph()
 
@@ -229,7 +230,7 @@ async def test_stringify_query_result(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_optional_match(decoded_r: redis.Redis):
     # Build a graph of form (a)-[R]->(b)
     node0 = Node(node_id=0, label="L1", properties={"value": "a"})
@@ -255,7 +256,7 @@ async def test_optional_match(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_cached_execution(decoded_r: redis.Redis):
     await decoded_r.graph().query("CREATE ()")
 
@@ -276,7 +277,7 @@ async def test_cached_execution(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_slowlog(decoded_r: redis.Redis):
     create_query = """CREATE
     (:Rider {name:'Valentino Rossi'})-[:rides]->(:Team {name:'Yamaha'}),
@@ -291,7 +292,7 @@ async def test_slowlog(decoded_r: redis.Redis):
 
 @pytest.mark.xfail(strict=False)
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_query_timeout(decoded_r: redis.Redis):
     # Build a sample graph with 1000 nodes.
     await decoded_r.graph().query("UNWIND range(0,1000) as val CREATE ({v: val})")
@@ -306,7 +307,7 @@ async def test_query_timeout(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_read_only_query(decoded_r: redis.Redis):
     with pytest.raises(Exception):
         # Issue a write query, specifying read-only true,
@@ -316,7 +317,7 @@ async def test_read_only_query(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_profile(decoded_r: redis.Redis):
     q = """UNWIND range(1, 3) AS x CREATE (p:Person {v:x})"""
     profile = (await decoded_r.graph().profile(q)).result_set
@@ -333,7 +334,7 @@ async def test_profile(decoded_r: redis.Redis):
 
 @skip_if_redis_enterprise()
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_config(decoded_r: redis.Redis):
     config_name = "RESULTSET_SIZE"
     config_value = 3
@@ -366,7 +367,7 @@ async def test_config(decoded_r: redis.Redis):
 
 @pytest.mark.onlynoncluster
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_list_keys(decoded_r: redis.Redis):
     result = await decoded_r.graph().list_keys()
     assert result == []
@@ -390,7 +391,7 @@ async def test_list_keys(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_multi_label(decoded_r: redis.Redis):
     redis_graph = decoded_r.graph("g")
 
@@ -417,7 +418,7 @@ async def test_multi_label(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_execution_plan(decoded_r: redis.Redis):
     redis_graph = decoded_r.graph("execution_plan")
     create_query = """CREATE
@@ -437,7 +438,7 @@ async def test_execution_plan(decoded_r: redis.Redis):
 
 
 @pytest.mark.redismod
-@pytest.mark.skip(reason="Graph module removed from Redis Stack")
+@skip_if_resp_version(3)
 async def test_explain(decoded_r: redis.Redis):
     redis_graph = decoded_r.graph("execution_plan")
     # graph creation / population

diff --git a/tests/test_commands.py b/tests/test_commands.py
@@ -712,11 +712,15 @@ def test_client_kill_filter_by_user(self, r, request):
     @skip_if_redis_enterprise()
     @pytest.mark.onlynoncluster
     def test_client_kill_filter_by_maxage(self, r, request):
-        _get_client(redis.Redis, request, flushdb=False)
+        r2 = _get_client(redis.Redis, request, flushdb=False)
+        name = "target-foobar"
+        r2.client_setname(name)
         time.sleep(4)
-        assert len(r.client_list()) >= 2
+        initial_clients = [c["name"] for c in r.client_list()]
+        assert name in initial_clients
         r.client_kill_filter(maxage=2)
-        assert len(r.client_list()) == 1
+        final_clients = [c["name"] for c in r.client_list()]
+        assert name not in final_clients
 
     @pytest.mark.onlynoncluster
     @skip_if_server_version_lt("2.9.50")