Refactor map comparison in Python and Node #1319

shohamazon · 2024-04-21T08:42:36Z

Issue #, if available:

Description of changes:

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

Please have a look on #1257.
LinkedHashMap in java ensures the order, while regular HashMap - don't. Probably, you need to do similar fix in python and node clients.

node/tests/SharedTests.ts

python/python/tests/test_async_client.py

barshaul

+1 to what Yuri commented, lets make the comparison clearer

shohamazon · 2024-05-05T08:36:16Z

+1 to what Yuri commented, lets make the comparison clearer

@barshaul round

node/tests/TestUtilities.ts

barshaul · 2024-05-07T08:57:08Z

node/tests/TestUtilities.ts

+        if (
+            map[key] !== null &&
+            typeof map[key] === "object" &&
+            map2[key] !== null &&
+            typeof map2[key] === "object" &&
+            !Array.isArray(map[key]) &&
+            !Array.isArray(map2[key])


this is really not readable.
I think map[key] !== null can be removed if we check for typeof map2[key] === "object".
Try to check if there's a better way of checking if it's a map, and add a comment explaining what you do there

barshaul · 2024-05-07T08:59:27Z

node/tests/TestUtilities.ts

+        ) {
+            // If the value is a nested map, recursively compare them
+            compareMaps(
+                map[key] as Record<string, unknown>,


Is it sure that the record's key is a string?

node/tests/TestUtilities.ts

barshaul · 2024-05-07T09:01:43Z

python/python/tests/test_async_client.py

@@ -117,6 +117,14 @@ async def check_if_server_version_lt(client: TRedisClient, min_version: str) ->
    return version.parse(redis_version) < version.parse(min_version)


+def compare_maps(map, map2):


same- add description

and move it to a utils file

barshaul

Please add unit tests for the map compare functions. make sure recursive calls and maps with same items but different order return the expected results

Yury-Fridlyand

LGTM, but one small comment.
Thanks for testing the test tools - that is a really good approach!

python/python/tests/utils/utils.py

barshaul · 2024-05-08T06:52:26Z

node/tests/UnitTests.test.ts

+import { describe, expect, it } from "@jest/globals";
+import { compareMaps } from "./TestUtilities";
+
+describe("test compareMaps", () => {


All of the tests here should be under TestUtilities, please remove this file

barshaul · 2024-05-08T06:55:03Z

node/tests/UnitTests.test.ts

+import { compareMaps } from "./TestUtilities";
+
+describe("test compareMaps", () => {
+    it("should compare two empty maps", () => {


"should compare " doesn't imply what the test checks for - should it be equal? not equal? instead write something like "Map comparison of empty maps should return true"

barshaul · 2024-05-08T06:56:26Z

node/tests/TestUtilities.ts

+    map: Record<string | number | symbol, unknown>,
+    map2: Record<string | number | symbol, unknown>,
+) {
+    expect(JSON.stringify(map)).toEqual(JSON.stringify(map2));


return true for equal and false if not. then do the expect true or false inside the tests. it is more readable from the function name and from reading the test

python/python/tests/utils/utils.py

barshaul · 2024-05-09T08:12:26Z

node/tests/TestUtilities.ts

+ * ```
+ */
+export function compareMaps(
+    map: Record<string | number | symbol, unknown>,


why symbol, unknown?

All possible values of a key of map in typescript

I can remove that

barshaul · 2024-05-09T08:14:38Z

node/tests/UtilsTests.test.ts

+        expect(compareMaps(map1, map2)).toBe(true);
+    });
+
+    it("Map comparison of maps with mixed types of values should return true", () => {


wrong description

barshaul · 2024-05-09T08:14:57Z

node/tests/UtilsTests.test.ts

+        expect(compareMaps(map1, map2)).toBe(true);
+    });
+
+    it("Map comparison of maps with nested maps having different values should return true", () => {


wrong description

barshaul · 2024-05-09T08:17:28Z

python/python/tests/test_async_client.py

+            RangeByScore(start=InfBound.NEG_INF, stop=InfBound.POS_INF),
+        )
+        expected_map = {"one": 1.0, "two": 2.0, "three": 3.0}
+        assert compare_maps(zrange_map, expected_map)


minor: it's more readable when explicit
assert compare_maps(zrange_map, expected_map) is True
(throughout all)

barshaul · 2024-05-09T08:18:05Z

python/python/tests/test_utils.py

+            "f": [1, "2", False],
+            "e": "string",
+        }
+        assert not compare_maps(map1, map2)


same - make it more readable with is True is False everywhere

barshaul · 2024-05-09T08:18:33Z

python/python/tests/utils/utils.py

+    return version.parse(redis_version) < version.parse(min_version)
+
+
+def compare_maps(map, map2):


what about typing?

barshaul

minor comments

node/tests/UtilsTests.test.ts

python/python/tests/test_async_client.py

python/python/tests/test_utils.py

Yury-Fridlyand · 2024-05-09T21:29:40Z

node/tests/TestUtilities.ts

+ */
+export function compareMaps(
+    map: Record<string | number | symbol, unknown>,
+    map2: Record<string | number | symbol, unknown>,


optional:

Suggested change

map2: Record<string | number | symbol, unknown>,

map2: Record<string, unknown>,

AFAIK all maps in redis have string keys.

Yury-Fridlyand · 2024-05-09T21:30:52Z

python/python/glide/constants.py

@@ -18,6 +18,7 @@
    int,
    None,
    Dict[str, T],
+    Mapping[str, "TResult"],


Is it possible?

Suggested change

Mapping[str, "TResult"],

Mapping[str, TResult],

Nope 😕 , this is the syntax of what you're suggesting

shohamazon added python node labels Apr 21, 2024

shohamazon requested a review from a team as a code owner April 21, 2024 08:42

shohamazon force-pushed the map_ordering branch 2 times, most recently from 1fa0c2b to 9d039bf Compare April 21, 2024 08:50

Yury-Fridlyand reviewed Apr 24, 2024

View reviewed changes

node/tests/SharedTests.ts Outdated Show resolved Hide resolved

python/python/tests/test_async_client.py Show resolved Hide resolved

barshaul requested changes May 2, 2024

View reviewed changes

shohamazon force-pushed the map_ordering branch 4 times, most recently from 079dcb5 to 3390553 Compare May 5, 2024 08:24

shohamazon requested a review from barshaul May 5, 2024 08:36

Yury-Fridlyand reviewed May 6, 2024

View reviewed changes

node/tests/TestUtilities.ts Outdated Show resolved Hide resolved

barshaul reviewed May 7, 2024

View reviewed changes

node/tests/TestUtilities.ts Show resolved Hide resolved

barshaul reviewed May 7, 2024

View reviewed changes

barshaul requested changes May 7, 2024

View reviewed changes

Yury-Fridlyand approved these changes May 7, 2024

View reviewed changes

python/python/tests/utils/utils.py Outdated Show resolved Hide resolved

barshaul reviewed May 8, 2024

View reviewed changes

python/python/tests/utils/utils.py Outdated Show resolved Hide resolved

Yury-Fridlyand approved these changes May 8, 2024

View reviewed changes

python/python/tests/utils/utils.py Outdated Show resolved Hide resolved

barshaul reviewed May 9, 2024

View reviewed changes

barshaul approved these changes May 9, 2024

View reviewed changes