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

Exceptions can be hard to catch in test environments #197

Open
jejacks0n opened this issue Oct 14, 2022 · 0 comments
Open

Exceptions can be hard to catch in test environments #197

jejacks0n opened this issue Oct 14, 2022 · 0 comments

Comments

@jejacks0n
Copy link

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 makes catching exceptions in tests hard.

Take this spec as an example:

$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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant