feat(testing): add msgpack support #2394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2394 +/- ##
===========================================
- Coverage 100.00% 99.92% -0.08%
===========================================
Files 64 64
Lines 7726 7732 +6
Branches 1071 1072 +1
===========================================
Hits 7726 7726
- Misses 0 5 +5
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
vytas7
changed the title
| .. towncrier release notes start | ||
|
|
||
|
|
||
| Misc |
There was a problem hiding this comment.
Please do not render out newsfragments yourself; a Falcon release manager does that when the time comes.
Moreover, this fragment looks stale, it is already release as part of Falcon 4.0.2.
There was a problem hiding this comment.
We need more extensive tests for this new feature.
You have a couple of test cases there, but we should also check whether the simulated body is correct, not just the content type.
I would suggest to use pytest.mark.parametrize(...) to create multiple test cases from different test data, but the same test code.
We also want to test the combination of msgpack= together with other parameters such as json=, etc, in order to verify that the documented precedence order is correct.
There was a problem hiding this comment.
I have a doubt about that, with json we pass parameters (json), (json, headers), (json,content-type), (json, headers, content-type), I did similar tests with both msgpack and json with msgpack, but mintests still breaks. What's the best way to parametrize it? And how do I check if simulated body is correct?
There was a problem hiding this comment.
Newsfragment file missing. You don't need to manually include it in 4.1.0.rst, but rather create a separate newsfragment file. Please check our docs how to contribute these files.
| @@ -808,6 +814,11 @@ async def _simulate_request_asgi( | |||
| overrides `body` and sets the Content-Type header to | |||
| ``'application/json'``, overriding any value specified by either | |||
| the `content_type` or `headers` arguments. | |||
| msgpack(Msgpack serializable): A Msgpack document to serialize as the | |||
There was a problem hiding this comment.
It is not documented what happens in the case both json and msgpack are specified.
There was a problem hiding this comment.
Hello @vytas7, sorry for taking so long, I added comments that say that the content-type will be MEDIA-MSGPACK when we pass both json and msgpack as parameters.
There was a problem hiding this comment.
We also need to add/adapt tests checking what happens when msgpack is not installed.
Judging from the tox -e mintest output, this case is not always handled as expected in the proposed changeset, at least not in the tests.
There was a problem hiding this comment.
How would I do that? Would it be with a try, except statement while importing msgpack? Or is there a better way for testing it?
Summary of Changes
Added the msgpack parameter in simulate_request, working in a similar way than the JSON one, when msgpack is not None we pass the content type as MEDIA_MSGPACK and it is fully documented on the comments and documentation.
Related Issues
Fixes #1026.
Relates to #333.
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
Couldn't find out where to place coverage tests for request_simulation and how to test msgpack parameter.
Didn't understand where to put the prefix with my GitHub nick.
docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.Didn't understand where to put the versionchanged prefix and if I should put it.
docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtowncrier --draftto ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.