Port of redis code improvements from prior revision #1132
Conversation
|
@auvipy It should not break master -- there are no merge conflicts and the travis-ci passes. It is not functional changes but rather refactoring. |
|
I am talking about celery master, not kombu |
|
@auvipy Does not break celery master, that code base does not reference |
Codecov Report
@@ Coverage Diff @@
## master #1132 +/- ##
==========================================
- Coverage 88.83% 88.61% -0.23%
==========================================
Files 65 65
Lines 6788 6796 +8
Branches 815 816 +1
==========================================
- Hits 6030 6022 -8
- Misses 665 678 +13
- Partials 93 96 +3
Continue to review full report at Codecov.
|
|
@auvipy This one has been updated for review again. |
|
I spent days on this before this summer and when the original PR was reverted I was heart broken so I thought to make the best of it I would capture the testing bits and other variable naming improvements that is not functional code changes, and so to see it sit here and linger and be not considered important is again heart breaking. Especially when on the other half of my PRs that have no testing the common ask is for testing, but we need to enhance the Redis tests and that is exactly what this PR does! |
|
LGTM. |
kombu/transport/redis.py
Outdated
| else: | ||
| queue_args = (queue, '', '') | ||
| priority_queue_name = '%s%s%s' % queue_args | ||
| return priority_queue_name |
There was a problem hiding this comment.
It's a question of taste, I guess, but IMO this rewrite of the function is not an improvement. The 2-line version was clear enough, and easier to read.
| class FakeRedisKombuTransport(FakeRedisKombuTransportLite): | ||
| Channel = FakeRedisKombuChannel | ||
|
|
||
|
|
There was a problem hiding this comment.
So, without getting into too much detail -- the above is nearly 500 lines of adaptations and setting up clients on top of FakeRedis.
- If you're already refactoring, I'd take these out to their own file. Maybe more than one.
- Why is all this necessary? there seem to be no comments explaining it, and coming in knowing nothing, I'd basically expect a FakeRedis-based test-suite to just set up FakeRedis as the Redis server, and then run everything, as is, against that -- 10-20 lines of setup before the actual tests begin, not 500.
I note that about half of these lines are pre-existing, but IIUC that was before FakeRedis -- so the test basically included "a small FakeRedis" inside it. Now that an actual FakeRedis is used, I'd expect these to go away. Or, otherwise, explain the need for them.
| global _fake_redis_client | ||
| _fake_redis_client = None |
There was a problem hiding this comment.
Why is tearDown() tearing down something setUp() did not set up?
t/unit/transport/test_redis.py
Outdated
| except Exception: | ||
| raise |
t/unit/transport/test_redis.py
Outdated
| conn_kwargs = dict( | ||
| transport=FakeRedisKombuTransport, | ||
| transport_options={'sep': ':'}) | ||
| with Connection(**conn_kwargs) as conn: |
There was a problem hiding this comment.
Suggestion:
with Connection(
transport=FakeRedisKombuTransport,
transport_options={'sep': ':'},
) as conn:| _fake_redis_client = None | ||
|
|
||
|
|
||
| def _get_fake_redis_client(): | ||
| global _fake_redis_client | ||
| if _fake_redis_client is None: | ||
| _fake_redis_client = FakeRedisClient() | ||
| return _fake_redis_client |
There was a problem hiding this comment.
While this implies you'd want to use the fake redis client as a singleton -- set up once and then used forever -- in actual use in the test suite, you take care to destroy the global in tearDown() functions, to force it to be set up anew later. This seems wrong -- it looks like you should be using normal objects, perhaps setting the client as an attribute of the TestCase instance, rather than try to make it a singleton fetched through a function and then "reach behind the function's back" to manipulate the global.
matusvalo
force-pushed
the
redis-improvements
branch
from
9b8bf9a
to
5b9263e
Compare
|
Thank you @matteius. I am sorry for long delay :-). |
thanks both of you for bringing & drawing the finishing line. |
|
I had to revert this PR due failing integration tests. I prefer reopen this PR (not sure how can be done) and fix the problems with failing integration tests. |
|
Revert the revert and edit the title. |
This brings back the naming convention refactor and the test cases to make use of the fakeredis testing module which allowed me to introduce some new tests and cleanup how imports were handled in the test file. This was lost when my prior "redis fix" was reverted: https://github.com/celery/kombu/pull/1106/files
The changes here are more minimal and change not how the redis module functions, but changes it to be more readable and to update the test class to be more complete.