coinbase / coinbase/temporal-ruby

Exceptions can be hard to catch in test environments

Open
#197 0 comments 2 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.