strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

Create initial structure for unified HTTP tests

Open patrick91 opened this issue 2 years ago • 1 comments

This PR refactors the HTTP tests to remove code duplication. Working on this PR also surfaced some inconsistencies and missing features between the integrations.

To make this PR I created some sort of abstraction for the http clients, I think I want to publish this abstraction as a separated package at some point, but for now I'll keep here until I know the API makes sense.

Also this client overlaps with our test client, we should combine them somehow.

In terms of inconsistencies I found:

  1. Chalice didn't support returning a custom status code, fixed in this PR (breaking change?)
  2. Sanic didn't support returning a custom status code, fixed in this PR (breaking change)
  3. Aiohttp didn't support returning a custom status code, fixed in this PR (breaking change)
  4. Flask was returning 415 when doing a GET request to the GraphQL view with GraphiQL disabled, now it returns 404
  5. Some integrations weren't checking errors when parsing request data, fixed in this PR
  6. Chalice's process result receives the request like the other integrations (breaking change)
    • [ ] Maybe we want to revert this (or do the same for flask)
  7. Same as above for sanic (breaking change) plus process result for sanic is async now
  8. Flask async view's get_root_value, get_context and process_results are now async

I've also introduce a class called TemporalResponse, this will be used to override the status_code and headers, but it will need more updates:

  • [ ] Add support for passing headers (in this PR)
  • [ ] Make all integration use this new class (django has its own for example)
  • [ ] Start using TypedDict for get_context return types (breaking change, mostly for typing)

Just to be clear, this work should lead to reducing the code duplication in the integrations (see also #1870, we might want to move the integration somewhere else, but that's to be discusses)

patrick91 avatar Apr 23 '22 04:04 patrick91

Codecov Report

Merging #1840 (e4c60ce) into main (c8f44e5) will decrease coverage by 0.96%. The diff coverage is 95.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1840      +/-   ##
==========================================
- Coverage   97.75%   96.79%   -0.97%     
==========================================
  Files         180      180              
  Lines        7362     7421      +59     
  Branches     1324     1331       +7     
==========================================
- Hits         7197     7183      -14     
- Misses         80      142      +62     
- Partials       85       96      +11     

codecov[bot] avatar Apr 23 '22 04:04 codecov[bot]

I'm going to move some of these changes into individual PRs and then merge this when it only touches tests :)

patrick91 avatar Sep 29 '22 12:09 patrick91

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release does some internal refactoring of the HTTP views, hopefully it doesn't affect anyone. It mostly changes the status codes returned in case of errors (e.g. bad JSON, missing queries and so on).

It also improves the testing, and adds an entirely new test suite for the HTTP views, this means in future we'll be able to keep all the HTTP views in sync feature-wise.


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to @patrick91 for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

botberry avatar Dec 08 '22 22:12 botberry