Skip to content

Pass through the exception type of several durable object internal errors - #2157

Merged
jclee merged 3 commits into
mainfrom
jlee/retryable-exception-passthrough
May 28, 2024
Merged

Pass through the exception type of several durable object internal errors#2157
jclee merged 3 commits into
mainfrom
jlee/retryable-exception-passthrough

Conversation

@jclee

@jclee jclee commented May 24, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@jclee
jclee requested review from a team as code owners May 24, 2024 05:53
@jclee
jclee force-pushed the jlee/retryable-exception-passthrough branch 2 times, most recently from 2566b84 to 354a88c Compare May 24, 2024 23:03
@jclee

jclee commented May 27, 2024

Copy link
Copy Markdown
Contributor Author

(Per internal feedback, updated PR to move WD_EXPECT_THROW() test macro to a more accessible location.)

@jclee
jclee requested a review from justin-mp May 27, 2024 23:44
Comment thread src/workerd/util/test.h
#define WD_EXPECT_THROW(expException, code, ...) \
do { \
auto expExcObj = expException; \
KJ_IF_SOME(e, ::kj::runCatchingExceptions([&]() { (void)({ code; }); })) { \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: what about exceptions like jsg::JsExceptionThrown? If those aren't important to capture here, all good... but might be good to specify that in a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point... The function does not currently provide special handling for jsg::JsExceptionThrown, and I hadn't been fully aware that those exceptions might need to be handled differently.

However, I think the function as-is is sufficient for a test to ensure that a thrown kj::Exception has the expected type and message, which is what it is currently used for. If a JsExceptionThrown is thrown instead, it would be captured via getCaughtExceptionAsKj() as a kj::Exception of type "failed", with an extra std::exception in the description. So I think it would hard at least for a JsExceptionThrown to accidentally match an expected non-JsExceptionThrown exception.

I've added a comment advising that the macro is intended for matching kj::Exceptions, and that someone who wants to assert on a JS exception should probably use a separate macro.

@jclee
jclee force-pushed the jlee/retryable-exception-passthrough branch from 354a88c to d900202 Compare May 28, 2024 17:27
@jclee
jclee force-pushed the jlee/retryable-exception-passthrough branch from d900202 to c00e345 Compare May 28, 2024 17:48
@jclee
jclee merged commit 81d1477 into main May 28, 2024
@jclee
jclee deleted the jlee/retryable-exception-passthrough branch May 28, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants