coinbase / coinbase/temporal-ruby
Exceptions can be hard to catch in test environments
- Dominant language
- Ruby
- Stars
- 287
- Forks
- 113
- Avg merge
- 6d 11h
- Merged PRs (30d)
- 1
Description
Cool library, and thank you for your hard work on it! I'm trying to help improve it through this feedback, and would be willing to open a PR if there's guidance provided on desirable compromises or solutions.
The way that `Temporal::Workflow` [rescues `StandardError`](https://github.com/coinbase/temporal-ruby/blob/master/lib/temporal/workflow.rb#L19) makes catching exceptions in tests hard.
Take this spec as an example:
```ruby
$global_var = 0
class HelloWorldWorkflow < Temporal::Workflow
def execute(arg1, required_key:)
$global_var += 1
nil
end
end
describe HelloWorldWorkflow do
it "doesn't allow exceptions to surface very easily" do
Temporal::Testing.local! do
Temporal.start_workflow(HelloWorldWorkflow, "foo", required_key: "bar")
expect($global_var).to eq(1)
Temporal.start_workflow(HelloWorldWorkflow, "foo") # incorrect arguments
expect($global_var).to eq(2)
end
end
end
```
The result is:
```
expected: 2
got: 1
(compared using ==)
```
Yes, I can see in the logs that an exception is _logged_, but I feel like in tests raising exceptions should be the rule. Any number of things can break downstream within a workflow, and sometimes (when not directly testing the unit that is the workflow) it's useful to call a thing, and not have to make an assertion that it didn't log an exception -- if that makes sense. Rescuing `StandardError` is heavy handed in the tests, because allowing those to raise is way more useful in identifying any issues.
To address this I've wrapped the base class with my own, so I can re-raise the exception in a way that Temporal doesn't try to handle. I'm wondering if `Temporal::Testing` should take this into consideration and not rescue any exceptions when executing the workflow locally in tests.
```ruby
class ApplicationWorkflow < Temporal::Workflow
def execute(*args, **kwargs)
perform(*args, **kwargs)
rescue => e
raise(Exception, e.message) if defined?(Temporal::Testing) && Temporal::Testing.local?
raise(e)
end
end
class HelloWorldWorkflow < ApplicationWorkflow
def perform(arg1, required_key:)
$global_var += 1
nil
end
end
```
And the more useful result now includes the exception and stops the execution of the spec, which is what would be most helpful.
```
Exception: missing keyword: :required_key
```
It's still not the best solution though because it now has two rescues and pollutes the stack trace.
Contributor guide
Assessment
This issue has not been assessed yet.