Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(testing): add msgpack support #2394
base: master
Are you sure you want to change the base?
feat(testing): add msgpack support #2394
Changes from all commits
9e9e3cd
8deb751
97a5a2a
58e251d
2cea4f6
f911793
aa6edf2
37d702e
4db15eb
918ba30
ef7a39d
f226b5b
f562383
41965f6
eab724a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 asjson=
, etc, in order to verify that the documented precedence order is correct.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can parametrize a parameter by specifying different values that it is called with.
As to checking whether a simulated body is correct, you can capture it in your test application that you are simulating requests against, and return it in the response. Or, you can use
SimpleTestResource
in your code.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.
Hello @vytas7, I have two doubts:
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check other existing tests, e.g.,
tests/test_media_multipart.py
, or many others.You can reuse the following pattern:
Then, later, shield your tests in question with the
pytest.mark.skipif(...)
decorator, e.g.: