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.
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?
There was a problem hiding this comment.
Please check other existing tests, e.g., tests/test_media_multipart.py, or many others.
You can reuse the following pattern:
# Somewhere in the beginning of the file, towards the end of the import blocks:
try:
import msgpack
except ImportError:
msgpack = NoneThen, later, shield your tests in question with the pytest.mark.skipif(...) decorator, e.g.:
@pytest.mark.skipif(msgpack is None, reason='msgpack is required for this test')
def test_my_new_msgpack_parameter(some_param, ...):
...
| @@ -14,9 +14,21 @@ on GitHub. | |||
|
|
|||
| .. NOTE(vytas): No changes to the supported platforms (yet). | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
A diff still remains in this file against master. Please revert all your changes to this release template.
There was a problem hiding this comment.
Please check other existing tests, e.g., tests/test_media_multipart.py, or many others.
You can reuse the following pattern:
# Somewhere in the beginning of the file, towards the end of the import blocks:
try:
import msgpack
except ImportError:
msgpack = NoneThen, later, shield your tests in question with the pytest.mark.skipif(...) decorator, e.g.:
@pytest.mark.skipif(msgpack is None, reason='msgpack is required for this test')
def test_my_new_msgpack_parameter(some_param, ...):
...
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.