From 3b2c6b39463173a2f14706e54ce4a7868747405c Mon Sep 17 00:00:00 2001 From: Jay Miller Date: Tue, 30 Jul 2024 14:52:50 -0400 Subject: [PATCH] adds isclose checks to flaky geotests - add geochecks to conftest - uses isclose to check float values - renamed check to assert_geo_is_close - update_assert_is_geo_tests - format with black - updates from pr Signed-off-by: Jay Miller --- tests/conftest.py | 23 +++ tests/test_asyncio/test_commands.py | 98 +++++++----- tests/test_commands.py | 224 ++++++++++++++-------------- 3 files changed, 196 insertions(+), 149 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 09ac315b..60d5242e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import argparse +import math import time from typing import Callable, TypeVar from unittest import mock @@ -493,6 +494,28 @@ def assert_resp_response(r, response, resp2_expected, resp3_expected): assert response == resp3_expected +def assert_geo_is_close(coords, expected_coords): + """ + Verifies that the coordinates are close within the floating point tolerance + + Valkey uses 52-bit presicion + """ + for a, b in zip(coords, expected_coords): + assert math.isclose(a, b) + + +def assert_resp_response_isclose(r, response, resp2_expected, resp3_expected): + """Verifies that the responses are close within the floating point tolerance""" + protocol = get_protocol_version(r) + + if protocol in [2, "2", None]: + assert_geo_is_close(response[0], resp2_expected[0]) + assert response[1:] == resp2_expected[1:] + else: + assert_geo_is_close(response[0], resp3_expected[0]) + assert response[1:] == resp3_expected[1:] + + def assert_resp_response_in(r, response, resp2_expected, resp3_expected): protocol = get_protocol_version(r) if protocol in [2, "2", None]: diff --git a/tests/test_asyncio/test_commands.py b/tests/test_asyncio/test_commands.py index 3a97dd6d..c50d844e 100644 --- a/tests/test_asyncio/test_commands.py +++ b/tests/test_asyncio/test_commands.py @@ -5,6 +5,7 @@ import asyncio import binascii import datetime +import math import re import sys from string import ascii_letters @@ -13,8 +14,10 @@ import pytest_asyncio import valkey from tests.conftest import ( + assert_geo_is_close, assert_resp_response, assert_resp_response_in, + assert_resp_response_isclose, is_resp2_connection, skip_if_server_version_gte, skip_if_server_version_lt, @@ -2459,7 +2462,7 @@ async def test_geopos(self, r: valkey.Valkey): await r.geoadd("barcelona", values) # valkey uses 52 bits precision, hereby small errors may be introduced. - assert_resp_response( + assert_resp_response_isclose( r, await r.geopos("barcelona", "place1", "place2"), [ @@ -2519,7 +2522,30 @@ async def test_georadius_units(self, r: valkey.Valkey): @skip_unless_arch_bits(64) @skip_if_server_version_lt("3.2.0") - async def test_georadius_with(self, r: valkey.Valkey): + @pytest.mark.parametrize( + "georadius_kwargs, expected_georadius_result", + [ + ( + {"withdist": True, "withcoord": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ( + {"withdist": True, "withcoord": True}, + [b"place1", 0.0881], + ), + ( + {"withhash": True, "withcoord": True}, + [b"place1", 3471609698139488], + ), + ( + {"withdist": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ], + ) + async def test_georadius_with( + self, r: valkey.Valkey, georadius_kwargs, expected_georadius_result + ): values = (2.1909389952632, 41.433791470673, "place1") + ( 2.1873744593677, 41.406342043777, @@ -2530,33 +2556,23 @@ async def test_georadius_with(self, r: valkey.Valkey): # test a bunch of combinations to test the parse response # function. - assert await r.georadius( + georadius_result = await r.georadius( "barcelona", 2.191, 41.433, 1, unit="km", - withdist=True, - withcoord=True, - withhash=True, - ) == [ - [ - b"place1", - 0.0881, - 3471609698139488, - (2.19093829393386841, 41.43379028184083523), - ] - ] - - assert await r.georadius( - "barcelona", 2.191, 41.433, 1, unit="km", withdist=True, withcoord=True - ) == [[b"place1", 0.0881, (2.19093829393386841, 41.43379028184083523)]] + **georadius_kwargs, + ) - assert await r.georadius( - "barcelona", 2.191, 41.433, 1, unit="km", withhash=True, withcoord=True - ) == [ - [b"place1", 3471609698139488, (2.19093829393386841, 41.43379028184083523)] - ] + assert len(georadius_result) == 1 + if "withcoord" in georadius_kwargs: + assert_geo_is_close( + georadius_result[0][-1], (2.19093829393386841, 41.43379028184083523) + ) + assert georadius_result[0][:-1] == expected_georadius_result + else: + assert georadius_result == [expected_georadius_result] # test no values. assert ( @@ -2566,9 +2582,7 @@ async def test_georadius_with(self, r: valkey.Valkey): 1, 1, unit="km", - withdist=True, - withcoord=True, - withhash=True, + **georadius_kwargs, ) == [] ) @@ -2632,7 +2646,8 @@ async def test_georadius_store_dist(self, r: valkey.Valkey): "barcelona", 2.191, 41.433, 1000, store_dist="places_barcelona" ) # instead of save the geo score, the distance is saved. - assert await r.zscore("places_barcelona", "place1") == 88.05060698409301 + z_score = await r.zscore("places_barcelona", "place1") + assert math.isclose(z_score, 88.05060698409301) @skip_unless_arch_bits(64) @skip_if_server_version_lt("3.2.0") @@ -2650,21 +2665,22 @@ async def test_georadiusmember(self, r: valkey.Valkey): ] assert await r.georadiusbymember("barcelona", "place1", 10) == [b"place1"] - assert await r.georadiusbymember( + radius_place2, radius_place1 = await r.georadiusbymember( "barcelona", "place1", 4000, withdist=True, withcoord=True, withhash=True - ) == [ - [ - b"\x80place2", - 3067.4157, - 3471609625421029, - (2.187376320362091, 41.40634178640635), - ], - [ - b"place1", - 0.0, - 3471609698139488, - (2.1909382939338684, 41.433790281840835), - ], + ) + + assert_geo_is_close(radius_place2[-1], (2.187376320362091, 41.40634178640635)) + assert radius_place2[:-1] == [ + b"\x80place2", + 3067.4157, + 3471609625421029, + ] + + assert_geo_is_close(radius_place1[-1], (2.1909382939338684, 41.433790281840835)) + assert radius_place1[:-1] == [ + b"place1", + 0.0, + 3471609698139488, ] @skip_if_server_version_lt("5.0.0") diff --git a/tests/test_commands.py b/tests/test_commands.py index 120590c1..49bc6b4e 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -1,5 +1,6 @@ import binascii import datetime +import math import re import threading import time @@ -21,8 +22,10 @@ from .conftest import ( _get_client, + assert_geo_is_close, assert_resp_response, assert_resp_response_in, + assert_resp_response_isclose, is_resp2_connection, skip_if_server_version_gte, skip_if_server_version_lt, @@ -3543,7 +3546,7 @@ def test_geopos(self, r): ) r.geoadd("barcelona", values) # valkey uses 52 bits precision, hereby small errors may be introduced. - assert_resp_response( + assert_resp_response_isclose( r, r.geopos("barcelona", "place1", "place2"), [ @@ -3605,33 +3608,34 @@ def test_geosearch_member(self, r): ) r.geoadd("barcelona", values) - assert r.geosearch("barcelona", member="place1", radius=4000) == [ - b"\x80place2", - b"place1", - ] assert r.geosearch("barcelona", member="place1", radius=10) == [b"place1"] - assert r.geosearch( + geosearch_place2, geosearch_place1 = r.geosearch( "barcelona", member="place1", radius=4000, withdist=True, withcoord=True, withhash=True, - ) == [ - [ - b"\x80place2", - 3067.4157, - 3471609625421029, - (2.187376320362091, 41.40634178640635), - ], - [ - b"place1", - 0.0, - 3471609698139488, - (2.1909382939338684, 41.433790281840835), - ], + ) + + # All but the coordinates are identical + geosearch_place2[:-1] == [ + b"\x80place2", + 3067.4157, + 3471609625421029, + ] + assert_geo_is_close( + geosearch_place2[-1], (2.187376320362091, 41.40634178640635) + ) + geosearch_place1[:-1] == [ + b"place1", + 0.0, + 3471609698139488, ] + assert_geo_is_close( + geosearch_place1[-1], (2.1909382939338684, 41.433790281840835) + ) @skip_if_server_version_lt("6.2.0") def test_geosearch_sort(self, r): @@ -3650,7 +3654,28 @@ def test_geosearch_sort(self, r): @skip_unless_arch_bits(64) @skip_if_server_version_lt("6.2.0") - def test_geosearch_with(self, r): + @pytest.mark.parametrize( + "geosearch_kwargs, expected_geosearch_result", + [ + ( + {"withdist": True, "withcoord": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ( + {"withdist": True, "withcoord": True}, + [b"place1", 0.0881], + ), + ( + {"withhash": True, "withcoord": True}, + [b"place1", 3471609698139488], + ), + ( + {"withdist": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ], + ) + def test_geosearch_with(self, r, geosearch_kwargs, expected_geosearch_result): values = (2.1909389952632, 41.433791470673, "place1") + ( 2.1873744593677, 41.406342043777, @@ -3660,44 +3685,23 @@ def test_geosearch_with(self, r): # test a bunch of combinations to test the parse response # function. - assert r.geosearch( + geosearch_result = r.geosearch( "barcelona", longitude=2.191, latitude=41.433, radius=1, unit="km", - withdist=True, - withcoord=True, - withhash=True, - ) == [ - [ - b"place1", - 0.0881, - 3471609698139488, - (2.19093829393386841, 41.43379028184083523), - ] - ] - assert r.geosearch( - "barcelona", - longitude=2.191, - latitude=41.433, - radius=1, - unit="km", - withdist=True, - withcoord=True, - ) == [[b"place1", 0.0881, (2.19093829393386841, 41.43379028184083523)]] - assert r.geosearch( - "barcelona", - longitude=2.191, - latitude=41.433, - radius=1, - unit="km", - withhash=True, - withcoord=True, - ) == [ - [b"place1", 3471609698139488, (2.19093829393386841, 41.43379028184083523)] - ] - # test no values. + **geosearch_kwargs, + ) + assert len(geosearch_result) == 1 + if "withcoord" in geosearch_kwargs: + assert_geo_is_close( + geosearch_result[0][-1], (2.1909382939338684, 41.433790281840835) + ) + assert geosearch_result[0][:-1] == expected_geosearch_result + else: + assert geosearch_result == [expected_geosearch_result] + assert ( r.geosearch( "barcelona", @@ -3705,9 +3709,7 @@ def test_geosearch_with(self, r): latitude=1, radius=1, unit="km", - withdist=True, - withcoord=True, - withhash=True, + **geosearch_kwargs, ) == [] ) @@ -3793,7 +3795,7 @@ def test_geosearchstore_dist(self, r): storedist=True, ) # instead of save the geo score, the distance is saved. - assert r.zscore("places_barcelona", "place1") == 88.05060698409301 + assert math.isclose(r.zscore("places_barcelona", "place1"), 88.05060698409301) @skip_if_server_version_lt("3.2.0") def test_georadius_Issue2609(self, r): @@ -3837,7 +3839,28 @@ def test_georadius_units(self, r): @skip_unless_arch_bits(64) @skip_if_server_version_lt("3.2.0") - def test_georadius_with(self, r): + @pytest.mark.parametrize( + "georadius_kwargs, expected_georadius_result", + [ + ( + {"withdist": True, "withcoord": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ( + {"withdist": True, "withcoord": True}, + [b"place1", 0.0881], + ), + ( + {"withhash": True, "withcoord": True}, + [b"place1", 3471609698139488], + ), + ( + {"withdist": True, "withhash": True}, + [b"place1", 0.0881, 3471609698139488], + ), + ], + ) + def test_georadius_with(self, r, georadius_kwargs, expected_georadius_result): values = (2.1909389952632, 41.433791470673, "place1") + ( 2.1873744593677, 41.406342043777, @@ -3848,48 +3871,26 @@ def test_georadius_with(self, r): # test a bunch of combinations to test the parse response # function. - assert r.georadius( + georadius_result = r.georadius( "barcelona", 2.191, 41.433, 1, unit="km", - withdist=True, - withcoord=True, - withhash=True, - ) == [ - [ - b"place1", - 0.0881, - 3471609698139488, - (2.19093829393386841, 41.43379028184083523), - ] - ] - - assert r.georadius( - "barcelona", 2.191, 41.433, 1, unit="km", withdist=True, withcoord=True - ) == [[b"place1", 0.0881, (2.19093829393386841, 41.43379028184083523)]] + **georadius_kwargs, + ) - assert r.georadius( - "barcelona", 2.191, 41.433, 1, unit="km", withhash=True, withcoord=True - ) == [ - [b"place1", 3471609698139488, (2.19093829393386841, 41.43379028184083523)] - ] + assert len(georadius_result) == 1 + if "withcoord" in georadius_kwargs: + assert_geo_is_close( + georadius_result[0][-1], (2.19093829393386841, 41.43379028184083523) + ) + assert georadius_result[0][:-1] == expected_georadius_result + else: + assert georadius_result == [expected_georadius_result] # test no values. - assert ( - r.georadius( - "barcelona", - 2, - 1, - 1, - unit="km", - withdist=True, - withcoord=True, - withhash=True, - ) - == [] - ) + assert r.georadius("barcelona", 2, 1, 1, unit="km", **georadius_kwargs) == [] @skip_if_server_version_lt("6.2.0") def test_georadius_count(self, r): @@ -3949,11 +3950,14 @@ def test_georadius_store_dist(self, r): r.geoadd("barcelona", values) r.georadius("barcelona", 2.191, 41.433, 1000, store_dist="places_barcelona") # instead of save the geo score, the distance is saved. - assert r.zscore("places_barcelona", "place1") == 88.05060698409301 + assert math.isclose(r.zscore("places_barcelona", "place1"), 88.05060698409301) @skip_unless_arch_bits(64) @skip_if_server_version_lt("3.2.0") def test_georadiusmember(self, r): + """ + Checks that r.georadiusbymember can return point, distance, and coordinate data + """ values = (2.1909389952632, 41.433791470673, "place1") + ( 2.1873744593677, 41.406342043777, @@ -3961,28 +3965,32 @@ def test_georadiusmember(self, r): ) r.geoadd("barcelona", values) - assert r.georadiusbymember("barcelona", "place1", 4000) == [ + assert r.georadiusbymember(name="barcelona", member="place1", radius=4000) == [ b"\x80place2", b"place1", ] assert r.georadiusbymember("barcelona", "place1", 10) == [b"place1"] - assert r.georadiusbymember( + radius_place2, radius_place1 = r.georadiusbymember( "barcelona", "place1", 4000, withdist=True, withcoord=True, withhash=True - ) == [ - [ - b"\x80place2", - 3067.4157, - 3471609625421029, - (2.187376320362091, 41.40634178640635), - ], - [ - b"place1", - 0.0, - 3471609698139488, - (2.1909382939338684, 41.433790281840835), - ], + ) + + # coordinates are in the last position + + assert radius_place2[:-1] == [ + b"\x80place2", + 3067.4157, + 3471609625421029, + ] + + assert_geo_is_close(radius_place2[-1], (2.187376320362091, 41.40634178640635)) + + assert radius_place1[:-1] == [ + b"place1", + 0.0, + 3471609698139488, ] + assert_geo_is_close(radius_place1[-1], (2.1909382939338684, 41.433790281840835)) @skip_if_server_version_lt("6.2.0") def test_georadiusmember_count(self, r):