Skip to content
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

Consolidate websocket protocol tests #2635

Merged

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Mar 13, 2023

Move tests for the websocket protocols (graphql_ws, graphql_transport_ws) into a separate place (tests/websocket)
where they can be parametrized and run on the four different integration which support websockets.

Description

Previously, websocket tests were duplicated for the aiohttp, asgi, fastapi and channels integrations, with both
protocols having a full set of tests in each.

Following what has been previously done with http, we now extend the abstract HttpClient classes in tests/http/clients
to provide async websocket functionality, parametrize the unit tests, and write them once.

This makes it easier to maintain a coherent set of unittests for the websocket protocols, while ensuring that they
do get executed for all of the available integrations.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #2635 (50e1cc2) into main (0b34c71) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2635      +/-   ##
==========================================
- Coverage   96.33%   96.31%   -0.02%     
==========================================
  Files         185      185              
  Lines        7714     7714              
  Branches     1421     1421              
==========================================
- Hits         7431     7430       -1     
- Misses        182      183       +1     
  Partials      101      101              

@kristjanvalur kristjanvalur marked this pull request as ready for review March 13, 2023 12:34
@botberry
Copy link
Member

botberry commented Mar 13, 2023

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)

Here's the tweet text:

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

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

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot, thanks! :D

CleanShot 2023-03-15 at 00 25 36@2x

I'm going to remove the release file, hope that's ok! I think we can skip the release since we are updating just tests 😊

@patrick91
Copy link
Member

@kristjanvalur I can't push, can you remove the release file? 😊

@kristjanvalur
Copy link
Contributor Author

yes, about the pushes, I'm using an organization repo for this, policy doesn't permit external pushes.

@patrick91 patrick91 merged commit 20e2027 into strawberry-graphql:main Mar 15, 2023
@kristjanvalur kristjanvalur deleted the kristjan/consolidate-tests branch March 15, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants