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

feat: basic websocket support #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kvalev
Copy link
Contributor

@kvalev kvalev commented Jan 26, 2020

This PR adds a very very basic (and highly likely buggy) support for mocking websockets.

@kvalev
Copy link
Contributor Author

kvalev commented Jan 26, 2020

@h2non the PR is missing a lot of tests, documentation and a bit of a polish, but let me know what you think so far.

@kvalev kvalev requested a review from h2non January 26, 2020 18:59
@kvalev kvalev force-pushed the websocket-interceptor branch 2 times, most recently from f8984a8 to 7d84960 Compare January 26, 2020 19:07
req.url = url

# TODO does this work multithreaded?!?
self._mock = self.engine.match(req)
Copy link
Owner

@h2non h2non Jan 28, 2020

Choose a reason for hiding this comment

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

It will not work. Mutex synchronization is required to make it safe. However, I feel that's another feature that has to be implemented in a separate PR.

Copy link
Owner

@h2non h2non left a comment

Choose a reason for hiding this comment

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

LGTM! This looks promising and ambitious!
Adding some extra tests would be great to cover more complex scenarios.

It would be great it can support as well this client, which works on top of asyncio:
https://github.com/aaugustin/websockets

At user API design level, I wonder about how the mock DSL can be improved to mock up exchanged frames with arbitrary responses provided by a routine.

I guess one obvious solution would be allowing the user to pass a function with a signature like:

def framer(frame, request) -> MockFrame:
   // Do magic things
   return mock

At a user definition API level, I'm inclined to overload the body() method to support this case:

def frame_mock(frame, request) -> MockFrame:
   // Do magic things
   return mock

(pook.get('ws://some-non-existing.org')
    .reply(204)
    .body(frame_mock)

Alternatively, implement a custom method for frames specific case:

(pook.get('ws://some-non-existing.org')
    .reply(204)
    .frames(frames_mock)

Opinions?

@piranna
Copy link

piranna commented Aug 11, 2024

Any update on this?

@piranna
Copy link

piranna commented Aug 11, 2024

body() could be abused if used múltiple times, being each message a body, if not It would have the full conversation, that's impractical.

@piranna
Copy link

piranna commented Aug 11, 2024

The same can be done for Server-Send Events, being body() used for each event.

@sarayourfriend
Copy link
Collaborator

@piranna can you clarify what you mean? Please feel free to open a PR targeting the latest master if you have ideas for how to implement this and I will review it gladly.

@piranna
Copy link

piranna commented Aug 12, 2024

can you clarify what you mean?

WebSockets (and Server-Send Events) don't have a single "body", but instead they send multiple messages, each one can be considered as a "body", and in fact they can come as as response to an incoming "request" message. body() method can be reused for this, but it should be allowed to be called several times for each "request".

Please feel free to open a PR targeting the latest master if you have ideas for how to implement this and I will review it gladly.

Maybe I'll do, that would depend if I can convince my boss to start doing testing...

@sarayourfriend
Copy link
Collaborator

Yes, understood. Calling body() multiple times is not supported in any meaningful sense today. While it does not cause an error, it blindly overwrites anything previously passed to body or related methods (json, xml). I don't see why this feature should change that, and the current implementation in this PR does not. It operates with a single call to body whose argument is interpreted as a list of frames or as a single frame otherwise.

In your original comments, were you referring to a specific issue with the implementation of this PR or just general feedback?

@piranna
Copy link

piranna commented Aug 12, 2024

General feedback, I think being able to send multiple messages would be a needed feature. Any idea how this could be implemented? In SSE is not so critical since it's unidirectional and on the wire response looks like a single resource, just only very slowly written, but for WebSockets it would be needed.

@sarayourfriend
Copy link
Collaborator

The implementation in this PR already supports sending multiple responses.

https://github.com/h2non/pook/pull/65/files#diff-f1377e282a873bc861cd98583286fcb6050a4841dc4cdecca8536634aafe4af4R61-R72

Each call to receive a frame continues the iteration through the list of frames passed to body().

@h2non's point was that the frames cannot be created in response to individual messages, so the suggestion of using a callback that can interpret the sent frame and create the response is meant to solve that. Is that what you're getting at?

A complication is that I'm not sure how to handle the fact that websocket connections are not all transactions, there isn't always a single response frame for every sent frame, and the timing of those responses may be significant. The frame generation callback could handle that nicely, if it's necessary to mock multiple frames sent after receiving a specific message or at the start of the connection, for example.

It's quite complicated.

SSE is not so complicated because of its unidirectionality, but should still be a separate feature and implementation.

@piranna
Copy link

piranna commented Aug 12, 2024

there isn't always a single response frame for every sent frame

That's the point I'm trying to get attention into.

SSE is not so complicated because of its unidirectionality, but should still be a separate feature and implementation.

Agree, just only since both can generate multiple messages back, I think solution could be common to both.

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.

4 participants