jamesls/fakeredis

redis 3.3.0 returns different values than expected causing failing tests in test_fakeredis.py

isms opened this issue · 3 comments

isms commented

Excluding health check failures covered by #240, there are still this unexpected results:

======================================================================
FAIL: test_ping (__main__.TestFakeStrictRedis)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 2963, in test_ping
    self.assertEqual(self.redis.execute_command('ping', 'test'), b'test')
AssertionError: False != b'test'

======================================================================
FAIL: test_zadd_minus_zero (__main__.TestFakeStrictRedis)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 1786, in test_zadd_minus_zero
    self.assertEqual(self.redis.execute_command('zscore', 'foo', 'a'), b'-0')
AssertionError: -0.0 != b'-0'

======================================================================
FAIL: test_ping (__main__.TestFakeStrictRedisDecodeResponses)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 2963, in test_ping
    self.assertEqual(self.redis.execute_command('ping', 'test'), b'test')
  File "test_fakeredis.py", line 4271, in assertEqual
    super(DecodeMixin, self).assertEqual(a, self._decode(b), msg)
AssertionError: False != 'test'

======================================================================
FAIL: test_zadd_minus_zero (__main__.TestFakeStrictRedisDecodeResponses)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 1786, in test_zadd_minus_zero
    self.assertEqual(self.redis.execute_command('zscore', 'foo', 'a'), b'-0')
  File "test_fakeredis.py", line 4271, in assertEqual
    super(DecodeMixin, self).assertEqual(a, self._decode(b), msg)
AssertionError: -0.0 != '-0'

======================================================================
FAIL: test_ping (__main__.TestRealStrictRedis)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 2963, in test_ping
    self.assertEqual(self.redis.execute_command('ping', 'test'), b'test')
AssertionError: False != b'test'

======================================================================
FAIL: test_zadd_minus_zero (__main__.TestRealStrictRedis)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 1786, in test_zadd_minus_zero
    self.assertEqual(self.redis.execute_command('zscore', 'foo', 'a'), b'-0')
AssertionError: -0.0 != b'-0'

======================================================================
FAIL: test_ping (__main__.TestRealStrictRedisDecodeResponses)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 2963, in test_ping
    self.assertEqual(self.redis.execute_command('ping', 'test'), b'test')
  File "test_fakeredis.py", line 4271, in assertEqual
    super(DecodeMixin, self).assertEqual(a, self._decode(b), msg)
AssertionError: False != 'test'

======================================================================
FAIL: test_zadd_minus_zero (__main__.TestRealStrictRedisDecodeResponses)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_fakeredis.py", line 1786, in test_zadd_minus_zero
    self.assertEqual(self.redis.execute_command('zscore', 'foo', 'a'), b'-0')
  File "test_fakeredis.py", line 4271, in assertEqual
    super(DecodeMixin, self).assertEqual(a, self._decode(b), msg)
AssertionError: -0.0 != '-0'

----------------------------------------------------------------------
Raab70 commented

I think this is really two failures and I can confirm that these are both legitimate (albeit possibly not purposeful) redis-py API changes:

Running redis==3.3.2

>>> from redis import StrictRedis
>>> rc = StrictRedis()
>>> rc.execute_command('ping', 'test')
False
>>> rc.zadd('foo', {'a': -0.0})
1
>>> rc.zadd('foo', {'a': 0.0})
0
>>> rc.execute_command('zscore', 'foo', 'a')
-0.0

Running redis==3.2.1

>>> from redis import StrictRedis
>>> rc = StrictRedis()
>>> rc.execute_command('ping', 'test')
b'test'
>>> rc.zadd('foobarbaz', {'a': -0.0})
1
>>> rc.zadd('foobarbaz', {'a': 0.0})
0
>>> rc.execute_command('zscore', 'foobarbaz', 'a')
b'-0'

Neither of which is called out in the changelog: https://github.com/andymccurdy/redis-py/blob/master/CHANGES

Should we open issues in redis-py?

bmerry commented

I think it is most likely this issue from the changelog:

Response callbacks are now case insensitive. This allows users that
call Redis.execute_command() directly to pass lower-case command
names and still get reasonable responses. #1168

So I think the short-term fix is to change the fakeredis tests to pass the command name in uppercase and to expect the redis 3.3+ behaviour (which should also be the 3.2- behaviour for uppercase command names).

Unfortunately this will probably weaken some of the tests because they're ensuring that the raw value (prior to the conversions done by redis-py) exactly matches real redis e.g. the sign of zero returned by zscore. That may prove important if anyone ever extends fakeredis to support other redis frontends like aioredis.

Raab70 commented

@bmerry I was thinking that would be the issue as well but it seems not. The changelog does indicate a change in support for lowercase commands but it reads as though it is expanding support for lowercase commands. I can confirm that the case doesn't matter using redis==3.3.2:

>>> from redis import StrictRedis
>>> rc = StrictRedis()
>>> rc.execute_command("PING", "test")
False
>>> rc.ping()
True
>>> rc.execute_command("PING")
True

It certainly seems like they may have inadvertently removed support for the ping message functionality. Nothing really stands out in the diff as breaking this. I do see them using the message functionality here though: redis/redis-py@3.2.1...3.3.0#diff-085cbe85ef9bc2ad832d79163eb39172R3187

However the zadd issue appears to be related to how responses are being parsed here Since we were using a lowercase command execution it wasn't being picked up for parsing but now it is:

using redis==3.3.2

>>> rc.zadd('foo', {'a': -0.0})
1
>>> rc.zadd('foo', {'a': 0.0})
0
>>> rc.execute_command('zscore', 'foo', 'a')
-0.0
>>> rc.execute_command('ZSCORE', 'foo', 'a')
-0.0

Using redis==3.2.1

>>> rc.zadd('foo', {'a': -0.0})
1
>>> rc.zadd('foo', {'a': 0.0})
0
>>> rc.execute_command('zscore', 'foo', 'a')
b'-0'
>>> rc.execute_command('ZSCORE', 'foo', 'a')
-0.0

So if we switch that test to using the uppercase command we can expect -0.0 for all versions above 3.0