-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fake redis server to properly test connection handshake #2947
base: master
Are you sure you want to change the base?
Add fake redis server to properly test connection handshake #2947
Conversation
18e881b
to
0acc6ab
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2947 +/- ##
==========================================
+ Coverage 91.32% 91.56% +0.24%
==========================================
Files 126 128 +2
Lines 32719 33258 +539
==========================================
+ Hits 29881 30454 +573
+ Misses 2838 2804 -34
☔ View full report in Codecov by Sentry. |
0acc6ab
to
e9990f2
Compare
tests/resp.py
Outdated
""" | ||
|
||
def __init__( | ||
self, protocol: int = 2, encoding: str = "utf-8", errorhander="strict" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why no type hint for errorhander
here?
tests/resp.py
Outdated
if self.generator is not None: | ||
self.generator.close() | ||
self.generator = None | ||
del self.consumed[:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do self.consumed.clear()
here to make it more readable? same for all other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point
if isinstance(data, dict): | ||
if self.protocol > 2: | ||
code = "|" if isinstance(data, Attribute) else "%" | ||
result = f"{code}{len(data)}\r\n".encode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use CRNL
variable here and in all other lines below since its already defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, CRNL is a bytes object, just a shorthand for parsing, not emitting where we use str.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i commented on wrong line. there are still a few insurances of b"\r\n"
in code in addition to strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like it like that since it makes the code more consistent and explicit where we are writing RESP responses.
a7ad42c
to
92d5067
Compare
6f55c63
to
78a5f92
Compare
… is fixed. This reverts commit 09b51d4.
78a5f92
to
5a79252
Compare
Pull Request check-list
Please make sure to review and check all of these items:
Description of change
This PR provides a transport agnostic RESP parser and encoder for the unit tests.
It also provides a simple, transport agnostic, RESP server. This allows us to share code and logic
between sync and async code.
A parametrized test is added to test the connection handshake for different client settings against different fake REDIS servers,
both sync and async.