Fixed missing CRLF at end of a chunk in malformed chunked encoding me…#10359
Fixed missing CRLF at end of a chunk in malformed chunked encoding me…#10359xdegaye wants to merge 3 commits intoaio-libs:masterfrom
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10359 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 122 122
Lines 37038 37089 +51
Branches 2041 2045 +4
=======================================
+ Hits 36585 36636 +51
Misses 314 314
Partials 139 139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #10359 will not alter performanceComparing Summary
|
|
It is not clear how to make the |
| assert out.is_eof() | ||
| assert b"asdf" == b"".join(out._buffer) | ||
|
|
||
| async def test_parse_chunked_payload_split_CRLF( |
There was a problem hiding this comment.
Can we get a test that uses the response: HttpResponseParser fixture, rather than using HttpPayloadParser directly?
I think we'd want to ensure that both parsers work the same way, this only tests the Python parser (which is not used by most users).
There was a problem hiding this comment.
Which is this second parser ?
Please bear with me, I have only been looking at aiohttp source code for few days.
The tests check the changes made in HttpPayloadParser, not in the second parser.
There was a problem hiding this comment.
Yes, if you use the fixture as mentioned above, the test will be run against both parsers (i.e. run twice).
There was a problem hiding this comment.
You did not answer my question. Please, what is the name of the second parser ?
Why should changes that affect HttpPayloadParser be tested against the second parser ?
What if the tests against the second parser fail ?
In that case:
- do you consider that these failures must be fixed within this PR ?
- would'nt it be better to update the title of the PR and the issue to restrict their scope to HttpPayloadParser ?
There was a problem hiding this comment.
There's a Python parser and a C parser (llhttp). Both should behave the same way, hence why we have a fixture to run the same tests against both parsers.
bdraco
added
the
pr-unfinished
3 participants
See issue #10355.