Fix buffer is closed error when using PythonParser class #1213
Conversation
m-novikov
force-pushed
the
m-novikov-buffer-closed
branch
2 times, most recently
from
359e6d1 to
2a926ef
Compare
| @@ -373,7 +373,7 @@ def __init__(self, socket_read_size: int): | |||
| def on_connect(self, connection: "Connection"): | |||
| """Called when the stream connects""" | |||
| self._stream = connection._reader | |||
| if self._buffer is None or self._stream is None: | |||
| if self._stream is None: | |||
There was a problem hiding this comment.
I am not too sure what was the original intent behind this line, buffer only initialized after so it only can be None here
There was a problem hiding this comment.
i believe we've had unexpected behavior here before, so keeping that there would be nice
There was a problem hiding this comment.
self._stream is None check is kept and test seems to be passing with it, The problem was self._buffer is None which always resulted in an exception. As it's not possible for it to be something other than None at this point.
m-novikov
force-pushed
the
m-novikov-buffer-closed
branch
2 times, most recently
from
f18236b to
2bcb685
Compare
m-novikov
force-pushed
the
m-novikov-buffer-closed
branch
from
2bcb685 to
aba24b4
Compare
| @pytest.mark.asyncio | ||
| async def test_invalid_response(r): | ||
| @pytest.mark.parametrize("create_redis", [(True, PythonParser)], indirect=True) | ||
| async def test_invalid_response(create_redis): |
There was a problem hiding this comment.
This test seems like it wasn't run for some time, as in the case of connection pool it will fail with AttributeError, because only a single connection client has a connection set. Ref #1114
There was a problem hiding this comment.
And it also didn't work on py =< 3.7
There was a problem hiding this comment.
And it also didn't work on py =< 3.7
We should probably drop Python 3.6 now anyways since it's EOL soon.
There was a problem hiding this comment.
But for 3.7 it's still a problem and pypy seems to be unhappy about this test also.
AsyncMock solves this problem.
Codecov Report
@@ Coverage Diff @@
## master #1213 +/- ##
==========================================
+ Coverage 89.21% 90.71% +1.49%
==========================================
Files 21 21
Lines 6872 6872
Branches 794 793 -1
==========================================
+ Hits 6131 6234 +103
+ Misses 578 467 -111
- Partials 163 171 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This LGTM. Change was in the mypy integration in #1101 Thanks!
|
@m-novikov are the changes to the test files necessary though or is it possible to simply delete the if condition for |
|
@Andrew-Chen-Wang changes in tests files are necessary to ensure that test pass also with |
|
I am not actually sure if it worth to test all combinations, or only 3 of them should suffice. e.g. (hiredis pool and single) and python parser single |
|
I agree that #1114 can be closed. I think having everything being tested is nice, though on CLI it might be annoying if you don't want to wait a full minute for the test suite to finish. Let me know if you disagree on that point. If not, I'll be merging this soon. |
|
One option would be to add marks to params diff --git a/tests/conftest.py b/tests/conftest.py
index 9d85205..ba82cfa 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -132,19 +132,25 @@ def skip_unless_arch_bits(arch_bits):
@pytest.fixture(
params=[
- (True, PythonParser),
- (False, PythonParser),
+ pytest.param((True, PythonParser), marks=[pytest.mark.python_parser]),
+ pytest.param((False, PythonParser), marks=[pytest.mark.python_parser]),
pytest.param(
(True, HiredisParser),
- marks=pytest.mark.skipif(
- not HIREDIS_AVAILABLE, reason="hiredis is not installed"
- ),
+ marks=[
+ pytest.mark.skipif(
+ not HIREDIS_AVAILABLE, reason="hiredis is not installed"
+ ),
+ pytest.mark.hiredis_parser,
+ ],
),
pytest.param(
(False, HiredisParser),
- marks=pytest.mark.skipif(
- not HIREDIS_AVAILABLE, reason="hiredis is not installed"
- ),
+ marks=[
+ pytest.mark.skipif(
+ not HIREDIS_AVAILABLE, reason="hiredis is not installed"
+ ),
+ pytest.mark.hiredis_parser,
+ ],
),
],
ids=[And then only marked tests can be run via |
|
Let's merge this full test suite solution first; a separate PR for the marker solution would be great. I'll direct the other maintainers to decide on that. |
|
Thanks again! |
What do these changes do?
Fix issue when the exception is raised on connection initialization in PythonParser
Add parametrization for both parsers to all tests using redis client to catch these errors in the future
Are there changes in behavior for the user?
No
Related issue number
#1212
#1114
Checklist
CONTRIBUTORS.txt<Name> <Surname>.CHANGES/folder<issue_id>.<type>(e.g.588.bugfix)issue_idchange it to the pr id after creating the PR.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.