Add tests to validation behaviour of graphql-transport-ws subscriptions #3671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…le." This reverts commit 6198007.
kristjanvalur
force-pushed
the
subscription_tests
branch
from
f1f9efa
to
c4d0b05
Compare
11 tasks
CodSpeed Performance ReportMerging #3671 will not alter performanceComparing Summary
|
Reviewer's Guide by SourceryThis pull request adds new tests to validate the behavior of GraphQL Transport WebSocket (WS) subscriptions and queries. It focuses on ensuring validation runs on a worker task, error handling, and validation checks for both queries and subscriptions. The changes reveal a discrepancy in how validation errors are handled between queries and subscriptions. Sequence diagram for validation error handling in GraphQL WSsequenceDiagram
participant Client
participant Server
Client->>Server: Send query { conditionalFail(fail:true) }
Server-->>Client: ErrorMessage { "type": "error", "id": "sub1", "message": "failed after sleep None" }
Client->>Server: Send subscription { conditionalFail(fail:true) }
Server-->>Client: ErrorMessage { "type": "error", "id": "sub1", "message": "failed after sleep None" }
Class diagram for ConditionalFailPermission and Schema changesclassDiagram
class ConditionalFailPermission {
+string message
+bool has_permission(source, info, **kwargs)
+float sleep
+bool fail
}
class Query {
+string conditional_fail(sleep, fail)
}
class Subscription {
+string conditional_fail(sleep, fail)
}
Query --> ConditionalFailPermission
Subscription --> ConditionalFailPermission
note for ConditionalFailPermission "Handles permission with optional sleep and fail parameters"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kristjanvalur - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+993
to
+1002
| async def test_validation_query(ws: WebSocketClient): | ||
| """ | ||
| Test validation for query | ||
| """ | ||
| await ws.send_json( | ||
| SubscribeMessage( | ||
| id="sub1", | ||
| payload=SubscribeMessagePayload( | ||
| query="query { conditionalFail(fail:true) }" | ||
| ), |
There was a problem hiding this comment.
issue (testing): Inconsistency in error handling between queries and subscriptions
As noted in the PR description, there's a discrepancy in how validation errors are handled for queries vs. subscriptions. This test is failing because it expects an ErrorMessage, but receives a NextMessage instead. Consider updating the implementation to handle validation errors consistently across queries and subscriptions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3671 +/- ##
===========================================
- Coverage 97.01% 10.99% -86.02%
===========================================
Files 503 454 -49
Lines 33458 29078 -4380
Branches 5618 1659 -3959
===========================================
- Hits 32460 3198 -29262
- Misses 792 25719 +24927
+ Partials 206 161 -45 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Additional tests for queries and subscriptions over
graphql_transport_wsprotocol.Note:
The tests uncover a discrepancy regarding the validation handling for queries vs. subscriptions. This is a behavior change since the recent changes in the subscriptions code.
When a subscription fails during validation, an
errormessage is sent. But if the same method is a single result (query), anextmessage is sent.Ideally, validation should be handled the same for both. The "errors" field of the
ExecutionResultshould indicate an error during execution, not pre-validation.The failing test is
test_validation_querywhiletest_validation_subscriptionsucceeds.The PR is not complete, it serves to illustrate a problem, I am open to suggestions. Accept the behaviour? Make validation return a
PreExecutionError? Add some other special handling?Types of Changes
Issues Fixed or Closed by This PR
Checklist