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

Added an option to pass connection params when initiating connection … #3403

Conversation

selvarajrajkanna
Copy link
Contributor

@selvarajrajkanna selvarajrajkanna commented Mar 3, 2024

…on strawberry channels test case

Description

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).

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We have skipped reviewing this pull request. All the files appear to be test files, which we're not great at reviewing... yet!

@botberry
Copy link
Member

botberry commented Mar 3, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release adds support to allow passing connection_params as dictionary to GraphQLWebsocketCommunicator class when testing channels integration

Example

GraphQLWebsocketCommunicator(
    application=application,
    path="/graphql",
    connection_params={"username": "strawberry"},
)

Here's the tweet text:

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

Get it here 👉 https://beta.strawberry.rocks/release/(next)

@selvarajrajkanna
Copy link
Contributor Author

selvarajrajkanna commented Mar 3, 2024

Im following https://strawberry.rocks/docs/integrations/channels#testing to write test cases for my app. I noticed that there is no parameter/method to pass connection_params when initiating the connection.

In https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/channels/testing.py file, on method gql_init() line 102, we are directly passing connection init message without any possibility to pass connection_params.

@DoctorJohn
Copy link
Member

DoctorJohn commented Mar 4, 2024

Thanks for the PR @selvarajrajkanna . Would you be interested in adding a test case for this? :)

(The test case could for example test sending connection init params and then querying them via a resolver)

Yup, we even have tests for the testing module. They can be found here: https://github.com/strawberry-graphql/strawberry/blob/main/tests/channels/test_testing.py

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Merging #3403 (17f3667) into main (364d152) will increase coverage by 1.05%.
The diff coverage is 100.00%.

❗ Current head 17f3667 differs from pull request most recent head 700c29c. Consider uploading reports for the commit 700c29c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3403      +/-   ##
==========================================
+ Coverage   95.35%   96.40%   +1.05%     
==========================================
  Files         498      498              
  Lines       31131    31137       +6     
  Branches     3809     3815       +6     
==========================================
+ Hits        29685    30019     +334     
+ Misses       1237      912     -325     
+ Partials      209      206       -3     

@selvarajrajkanna
Copy link
Contributor Author

selvarajrajkanna commented Mar 4, 2024

@DoctorJohn Added test case :)

Copy link

codspeed-hq bot commented Mar 4, 2024

CodSpeed Performance Report

Merging #3403 will improve performances by 37.04%

Comparing selvarajrajkanna:channels-test-connection-params (700c29c) with main (364d152)

Summary

⚡ 1 improvements
✅ 12 untouched benchmarks

Benchmarks breakdown

Benchmark main selvarajrajkanna:channels-test-connection-params Change
test_execute_basic 13.5 ms 9.8 ms +37.04%

Copy link
Member

@DoctorJohn DoctorJohn left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for adding tests and the code optimization @selvarajrajkanna! I just noticed mentioning this in our docs would be great too. You're the expert on these changes so if you're interesting in adding a secting to the testing docs, feel free to do so. We'll merge this PR shortly. Let me know whether you would be interested in adding docs. That could either be done in one go with this PR or a separate one.

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.

Thanks! I left a couple of changes for the changelog 😊

RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
@selvarajrajkanna
Copy link
Contributor Author

@patrick91 updated the RELEASE.md file with recommendation :)

@selvarajrajkanna
Copy link
Contributor Author

@DoctorJohn , Documentation isn't exactly my jam, but I'm a quick learner. Mind pointing me in the right direction?

@DoctorJohn
Copy link
Member

DoctorJohn commented Mar 6, 2024

@DoctorJohn , Documentation isn't exactly my jam, but I'm a quick learner. Mind pointing me in the right direction?

Gladly! Our documentation is written in markdown. The Channels documentation can be found under docs/integrations/channels. There is already a testing section. Therefore it would be sufficient to add a small subsection that mentions that connection init params can be passed and then possibly the code sample you already have in the RELEASE.md file.

Could look something like this:

    ### Passing connection params
    
    Connection parameters can be passed using the `connection_params` parameter of the `GraphQLWebsocketCommunicator` class. This is particularily useful to test websocket authentication. 

    ```python
    GraphQLWebsocketCommunicator(
        application=application,
        path="/graphql",
        connection_params={"token": "strawberry"},
    )
    ```

@selvarajrajkanna
Copy link
Contributor Author

@DoctorJohn , Essentially added your suggestion to the doc :)

@DoctorJohn
Copy link
Member

@DoctorJohn , Essentially added your suggestion to the doc :)

Awesome, thank you! I moved the new section in the docs a little further down so that the new subheading is scoped correctly. This is good to go now.

@DoctorJohn DoctorJohn merged commit 0c923bb into strawberry-graphql:main Mar 8, 2024
60 checks passed
@botberry
Copy link
Member

botberry commented Mar 8, 2024

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants