-
Notifications
You must be signed in to change notification settings - Fork 6
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
Event Stream Stubbing #215
Conversation
… start refactoring event stubs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I probably missed a bunch, but I trust the test coverage.
stub_data_class: Stubs::AllQueryStringTypes, | ||
stub_error_classes: [], | ||
stub_message_encoder: Hearth::EventStream::Binary.const_get(:MessageEncoder).new, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need const_get here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the module is intended to work from codegen - similar to the way we do an error module.
The specific encoding of messages is protocol specific (all existing ones currently use this same logic, but its supposed to be configurable in codegen per protocol) - so the protocol itself defines just a module to use and that module is expected to porovide a MessageEncoder
and a MessageDecoder
.
@@ -53,8 +53,10 @@ def self.build(config, options = {}) | |||
stack.use(Middleware::RequestId) | |||
stack.use(Hearth::Middleware::Send, | |||
client: config.http_client, | |||
response_events: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a true example here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for rails json - I don't think it really makes sense to try to add.
But theres good examples in white label with this.
end | ||
|
||
it 'signals the stubbed event' do | ||
subject.stub_responses(:start_event_stream, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonky formatting on all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, rubocop kept mucking these up. I think I've fixed them all.
@@ -90,6 +92,7 @@ def setup_response_events(context) | |||
event_handler: @event_handler | |||
) | |||
context.response.body = decoder | |||
context.metadata[:event_handler] = @event_handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a named key for this? Or is metadata fine?
Implement Event Stream Stubbing.
Adds support for stubbing for event streams. Interface:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.