set default response_callbacks to redis.asyncio.cluster.ClusterNode

[rapidia]

Pull Request check-list

Please make sure to review and check all of these items:

• Does $ tox pass with this change (including linting)?
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)?
• Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please provide a description of the change here.

asyncio.cluster.ClusterNode needs default response_callbacks, not None.

When I initialize RedisCluster with startup_nodes(instead host= and port=), response_callbacks of ClusterNode will be None, unless I specify response_callbacks.
It could be assigned when create ClusterNode, but having default value looks more reasonable.

[codecov-commenter]

Codecov Report

Merging #2201 (9f03e95) into master (9167a0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2201   +/-   ##
=======================================
  Coverage   91.99%   92.00%           
=======================================
  Files         108      108           
  Lines       27373    27377    +4     
=======================================
+ Hits        25183    25189    +6     
+ Misses       2190     2188    -2     

Impacted Files	Coverage Δ
redis/asyncio/cluster.py	89.08% <ø> (ø)
tests/test_asyncio/test_cluster.py	97.60% <100.00%> (+<0.01%)	⬆️
tests/test_cluster.py	97.14% <0.00%> (+0.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9167a0e...9f03e95. Read the comment docs.

[utkarshgupta137]

+1. Would you be able to add a test as well?

[rapidia]

@utkarshgupta137
I don't know how to test this case, properly. because, this is a just assignment of a value, not a logic.

basically, If response_callbacks is None, execute_command function will be brake with TypeError: 'NoneType' object is not subscriptable.

in the test case of test_startup_nodes, you are mocking execute_command, so you couldn't find this problem.

I can check response_callbacks of ClusterNode is not None. but still, you can set response_callbacks to None at runtime. I don't think it's a meaningful test case.

to prevent setting response_callbacks to None, there could be lots of ways.
I could make getter/setter for response_callbacks. but, I think it's getting out of purpose of this PR.

[utkarshgupta137]

Even if response_callbacks is a dictionary, then it would fail at https://github.com/redis/redis-py/blob/master/redis/asyncio/cluster.py#L972. So you need to add a test along the lines of:

node = ClusterNode(host="localhost", port=16379)
rc = RedisCluster(startup_nodes=[node])
assert await rc.set("A", 1)
assert await rc.get("A") == 1

[utkarshgupta137]

The test looks good. But its scope is not limited to just this argument. It would be better to merge it into test_startup_nodes.

[utkarshgupta137]

Awesome! @chayim @dvora-h This is a somewhat major bug, it should be released on priority.

[rapidia]

@utkarshgupta137 thank you for your advice 👍🏼

[dvora-h]

@rapidia Great catch, and a great PR, thank you!