-
Notifications
You must be signed in to change notification settings - Fork 96
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
Gracefully degrade application failure error when deserializing #103
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Simulates Temporal::Connection::Serializer::Failure | ||
Fabricator(:application_failure, from: Temporal::Api::Failure::V1::Failure) do | ||
transient :error_class, :backtrace | ||
message { |attrs| attrs[:message] } | ||
stack_trace { |attrs| attrs[:backtrace].join("\n") } | ||
application_failure_info do |attrs| | ||
Temporal::Api::Failure::V1::ApplicationFailureInfo.new( | ||
type: attrs[:error_class], | ||
details: TestDeserializer.new.to_details_payloads(attrs[:message]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will fail when used in any other spec since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also since the fabricator is for a proto message it probably should not be concerned with error serialisation, but passed details for the error directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good flag. As for the second suggestion, feels like reusing production methods rather than reimplementing them trumps strict dependencies here, to make tests more understandable and maintainable. |
||
) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
require 'temporal/workflow/errors' | ||
|
||
class TestDeserializer | ||
include Temporal::Concerns::Payloads | ||
end | ||
|
||
class ErrorWithTwoArgs < StandardError | ||
def initialize(message, another_argument); end | ||
end | ||
|
||
class SomeError < StandardError; end | ||
|
||
describe Temporal::Workflow::Errors do | ||
describe '.generate_error' do | ||
it "instantiates properly when the client has the error" do | ||
message = "An error message" | ||
stack_trace = ["a fake backtrace"] | ||
failure = Fabricate( | ||
:application_failure, | ||
message: message, | ||
backtrace: stack_trace, | ||
error_class: SomeError.to_s | ||
) | ||
|
||
e = Temporal::Workflow::Errors.generate_error(failure) | ||
expect(e).to be_a(SomeError) | ||
expect(e.message).to eq(message) | ||
expect(e.backtrace).to eq(stack_trace) | ||
|
||
end | ||
|
||
it "falls back to StandardError when the client doesn't have the error class" do | ||
allow(Temporal.logger).to receive(:error) | ||
|
||
message = "An error message" | ||
stack_trace = ["a fake backtrace"] | ||
failure = Fabricate( | ||
:application_failure, | ||
message: message, | ||
backtrace: stack_trace, | ||
error_class: 'NonexistentError', | ||
) | ||
|
||
e = Temporal::Workflow::Errors.generate_error(failure) | ||
expect(e).to be_a(StandardError) | ||
expect(e.message).to eq("NonexistentError: An error message") | ||
expect(e.backtrace).to eq(stack_trace) | ||
expect(Temporal.logger) | ||
.to have_received(:error) | ||
.with( | ||
'Could not find original error class. Defaulting to StandardError.', | ||
{:original_error=>"NonexistentError"}, | ||
) | ||
|
||
end | ||
|
||
|
||
it "falls back to StandardError when the client can't initialize the error class" do | ||
allow(Temporal.logger).to receive(:error) | ||
|
||
message = "An error message" | ||
stack_trace = ["a fake backtrace"] | ||
failure = Fabricate( | ||
:application_failure, | ||
message: message, | ||
backtrace: stack_trace, | ||
error_class: ErrorWithTwoArgs.to_s, | ||
) | ||
|
||
e = Temporal::Workflow::Errors.generate_error(failure) | ||
expect(e).to be_a(StandardError) | ||
expect(e.message).to eq("ErrorWithTwoArgs: An error message") | ||
expect(e.backtrace).to eq(stack_trace) | ||
expect(Temporal.logger) | ||
.to have_received(:error) | ||
.with( | ||
'Could not instantiate original error. Defaulting to StandardError.', | ||
{ | ||
:original_error=>"ErrorWithTwoArgs", | ||
:instantiation_error_message=> "wrong number of arguments (given 1, expected 2)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any specific reason for not using the new Hash syntax here ( |
||
}, | ||
) | ||
end | ||
|
||
end | ||
end |
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.
nit: Can you please move this one over to
fabricators/grpc
and prefix withapi_
? Helps with distinguishing between domain and edge entities