WIP: Run framework agnostic tests #828
Conversation
Documented in AbstractApi._response_from_handler. This method is implemented by subclasses. Body and tuples are validated by connexion.operations.validation.validate_operation_output fixes spec-first#578
AioHttpApp is handling errors like FlaskApp's common_error_handler
| @@ -34,8 +38,39 @@ def __init__(self, | |||
| content_type=None, | |||
| body=None, | |||
| headers=None): | |||
| if not isinstance(status_code, int) or not (100 <= status_code <= 505): | |||
| raise ValueError("{} is not a valid status code".format(status_code)) | |||
There was a problem hiding this comment.
Should connexion really be policing which status codes the user is allowed to send?
|
Hey @Panpann - I really like the idea of patching up the differences in the API between the different frameworks. I definitely applaud your effort to fix this. I'm also excited about the idea of being compatible with more frameworks like quart. I started reading through your PR, and it's quite long and included many orthogonal changes. Can you think about how you might break it up into chunks that all make sense on their own? Some things that could make sense as standalone fixes/improvements:
I'd love to review any of these individually, and continue to discuss the broader goal of cleaner abstractions, and supporting more frameworks. Sorry I wasn't able to get back to you sooner. I was on holiday. Great work! |
|
I like the idea to support tuple as return values for AioHttp handler, like for flask. |
|
Hi @dtkav I wasn't available last few days, I know there is a lot of changes in this PR, this is because I'm debugging one test file in tests/api/ folder after another. Do you have another solution ? |
|
That sounds great. @Jyhess has already been pulling off smaller chunks too, so looks like there's some good collaboration! I'm excited about the direction this is going. |
|
OK I pushed my last local changes as a reference. My first step will be to create the I will find a way to explicitly set a test as running in FlaskApp or AioHttpApp or both, something like @pytest.mark.app("FlaskApp")
def test_specific_for_flask():
passThis will allow to incrementally run both Flaskapp and AioHttpApp on specific tests covered by thoses small PRs |
|
After quart support gets in, I'd love to see sanic join the connexion crew as well :) this is awesome stuff. Is #849 the only break out PR so far? |
|
This will need to be rebased, but probably after #849. Some of the commits will drop out as they are fixed or implemented elsewhere. 58e3fbb is implemented in #849 (ready) These are moving things to operation validation - Why? does that make sense? Parts of these are candidates for dedicated coding style PRs: It looks like the other commits fall into three general categories:
|
* Support aiohttp handlers to return tuples * Minor update from #828 review * Factorize more code between Flask and AioHttp response * Fix CI * Drop six string types * Standardize response logging * Handle one-tuples that only contain data * clean up a couple of type hint comments * Add a few more get_response tests * Adjust _prepare_body interface to simplify improving _serialize_data Rename _jsonify_data to _serialize_data to make its purpose easier to understand (this was also known as _cast_body in aiohttp_api). In exploring how to harmonize json serialization between aiothttp and flask, we needed to be able to adjust the mimetype from within _serialize_data. Harmonizing the actual serialization has to wait until backwards incompatible changes can be made, but we can keep the new interface, as these functions were introduced in this PR (#849). * Add deprecation warnings about implicit serialization
|
Any chance this pull request can be pushed forward? |
|
Closing as this is based on outdated code. Since we're splitting a lot of functionality into framework-agnostic middleware, this will become a lot easier. Would still be interested to review a PR for a connexion test client. |
Hello,
context
a couple weeks ago, I created a branch to add quart support to connexion. I managed to implement the app and api but I struggled when it came time to test my work.
I wasn't familiar at all with the connexion codebase and didn't feel able to add tests.
Also this was frustrating because Flask and Quart follow almost the same API (except async for quart). I just wanted to use all tests that request the tests API via
test_client().I like your idea of API first principle, it makes APIs consistent by taking the time to think. And to write an openapi before starting coding helps to normalize data sent and returned by the API. It is easy to implement well documented API contracts and keep the code clean and business focused.
In the future, I would like to have the possibility to choose any python framework and to use connexion on it.
I think that to test the same way each implementation could help contributors to add more frameworks to connexion, if the contributor only has to implement the interface and see a concrete error if its code is wrong. It would also ensure that whatever server I'm using, the API is consistent and responds the same way.
proposed changes
I've started working on it,
AbstractApp.test_client()which returns anAbstractClient, each web server has to implement 1 synchronous method to request the API and returns an enhanced version of ConnexionResponse (just added helpers like response.json, response.text).For now the API to send request via this AbstractClient respects the client's flask API.
Are you okay with changes I'm proposing in this pull request ?
The work currently won't pass the CI, I'm creating this pull request to share my idea with you and have your opinion on it