Skip to content

add graphql websocket subsciption tests#768

Open
dwsutherland wants to merge 9 commits intocylc:1.8.xfrom
dwsutherland:graphql-ws-tests
Open

add graphql websocket subsciption tests#768
dwsutherland wants to merge 9 commits intocylc:1.8.xfrom
dwsutherland:graphql-ws-tests

Conversation

@dwsutherland
Copy link
Member

Adds an integration test for testing the graphql subscriptions via websocket connection.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@dwsutherland dwsutherland added this to the 1.8.2 milestone Dec 20, 2025
@dwsutherland dwsutherland self-assigned this Dec 20, 2025
@dwsutherland dwsutherland force-pushed the graphql-ws-tests branch 2 times, most recently from a0653ed to 8738722 Compare December 20, 2025 10:55
@dwsutherland dwsutherland changed the title add graphql websocket subsciption test add graphql websocket subsciption tests Dec 20, 2025
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM, one question (see comment).

This pushes code coverage in the right direction 👍:

Screenshot from 2026-01-05 10-58-11

There are still a few gaps:

  • Would be good to cover error handling, e.g, GraphQL validation errors.
  • Would be good to cover more of the websocket subprocotocol (though note that we will change subprotocol soon)
  • Would be good to cover connection termination (as this is something we've had issues with).

Comment on lines -410 to -427
def request_wants_html(self):
accepted = get_accepted_content_types(self.request)
accepted_length = len(accepted)
# the list will be ordered in preferred first - so we have to make
# sure the most preferred gets the highest number
html_priority = (
accepted_length - accepted.index("text/html")
if "text/html" in accepted
else 0
)
json_priority = (
accepted_length - accepted.index("application/json")
if "application/json" in accepted
else 0
)

return html_priority > json_priority

Copy link
Member

Choose a reason for hiding this comment

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

[note to reviewers]

This is presumably something we inherited via graphene-tornado that's no longer required - https://github.com/graphql-python/graphene-tornado/blob/e4fa7d7b4c2256fa37f5ad89cbfd4d4bf6fdb606/graphene_tornado/tornado_graphql_handler.py#L379

Can't find any trace of it in Tornado.

Copy link
Member Author

@dwsutherland dwsutherland Jan 13, 2026

Choose a reason for hiding this comment

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

Yes, I used Django as the starting point, and graphene-tornado as the reference (because graphene-django is more up to date)..
Some of the things removed were related to the inbuilt GraphiQL, where we use the UI/Vue version instead.

Comment on lines -95 to -113
def get_accepted_content_types(request: 'HTTPServerRequest') -> list:
def qualify(x):
parts = x.split(";", 1)
if len(parts) == 2:
match = re.match(
r"(^|;)q=(0(\.\d{,3})?|1(\.0{,3})?)(;|$)", parts[1])
if match:
return parts[0].strip(), float(match.group(2))
return parts[0].strip(), 1

raw_content_types = request.headers.get("Accept", "*/*").split(",")
qualified_content_types = map(qualify, raw_content_types)
return [
x[0]
for x in sorted(
qualified_content_types, key=lambda x: x[1], reverse=True)
]


Copy link
Member

Choose a reason for hiding this comment

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

[note to reviewers]

Unused function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

)
assert response.code == 200

# we should find the two dummy workflows in the response
Copy link
Member

Choose a reason for hiding this comment

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

Can only see one workflow in the response below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is a weird one.. Both graphene-django and graphene-tornado don't appear to be built to handle an actual batched response (or list of queries)...

I may have to change the code to "fix" this in accordance to the accepted behavior..

Copy link
Member

Choose a reason for hiding this comment

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

I more meant that the test code below doesn't seem to match the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry, probably a copy paste thing..

I would put this PR in draft mode, but I wanted the tests to run.

Comment on lines +341 to +356
response = json.loads(await ws.read_message())

assert response == {
'id': sub_id,
'type': 'data',
'payload': {
'data': {
'workflows': [
{
'id': '~me/foo',
'status': 'stopped'
}
]
}
}
}
Copy link
Member

@oliver-sanders oliver-sanders Jan 5, 2026

Choose a reason for hiding this comment

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

Would be good to test that the subscription can yield more than one result, i.e:

for _ in range(3):
    assert json.loads(await ws.read_message()) == { ...

    # poke UIS subscriptions as though something changed to make them yield again
    await do_something()

Not sure what do_something is though?

@oliver-sanders oliver-sanders modified the milestones: 1.8.2, 1.8.3 Jan 7, 2026
@dwsutherland
Copy link
Member Author

There are still a few gaps:

Yes, this is a work in process, I'll keep expanding the tests here (hopefully until 85% project coverage at min)

response = strip_null(response)

result = self.json_encode(response)
result = self.json_encode(response)
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to wrap json.dumps/encode in this try/except.. see:
https://github.com/graphql-python/graphene-django/blob/f02ea337a23df06d166d463d6f92cb7505fbb435/graphene_django/views.py#L215

response should always be JSON serializable..

@dwsutherland dwsutherland force-pushed the graphql-ws-tests branch 4 times, most recently from 1627744 to 5710e81 Compare January 30, 2026 08:07
@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 30, 2026

83% project coverage 🥇

Ping me when you want a review.

@dwsutherland
Copy link
Member Author

83% project coverage 🥇

Ping me when you want a review.

Just need to bump up the websocket/subscription side of things first.

@dwsutherland
Copy link
Member Author

dwsutherland commented Feb 17, 2026

@oliver-sanders - I think this will do for now, there's some more that could be done with the subscription handler.. But we might as well get this in as a first step for people to build on.

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.

2 participants