👍 Show stack-trace on plugin error #429
Conversation
WalkthroughThe changes introduce new test cases and modify existing ones for the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
- Coverage 95.55% 95.42% -0.14%
==========================================
Files 25 25
Lines 1418 1420 +2
Branches 180 180
==========================================
Hits 1355 1355
- Misses 60 62 +2
Partials 3 3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/denops/testdata/dummy_dispatcher_plugin.ts (1)
17-20: LGTM! Consider making the error message configurable.The implementation is clean and follows the existing patterns. The addition of this method will help improve test coverage for error handling scenarios.
Consider making the error message configurable to support different test scenarios:
- fail: async () => { + fail: async (message = "Dummy failure") => { await delay(MIMIC_DISPATCHER_METHOD_DELAY); - throw new Error("Dummy failure"); + throw new Error(message); },tests/denops/runtime/functions/denops/request_test.ts (1)
69-84: LGTM! Consider expanding error scenarios.The test case properly verifies error propagation from the dispatcher method.
Consider adding more test scenarios:
await t.step("if the dispatcher method throws an error", async (t) => { await t.step("throws an error", async () => { await assertRejects( () => host.call( "denops#request", "dummy", "fail", ["foo"], ), Error, "Failed to call 'fail' API in 'dummy': Dummy failure", ); }); + await t.step("throws with custom error message", async () => { + await assertRejects( + () => + host.call( + "denops#request", + "dummy", + "fail", + ["custom error"], + ), + Error, + "Failed to call 'fail' API in 'dummy': custom error", + ); + }); });tests/denops/runtime/functions/denops/request_async_test.ts (1)
118-150: Consider adding additional test assertions for completeness.To make the error handling test more comprehensive, consider:
- Verifying that the success callback is never called
- Adding assertions to ensure the error type consistency across different failure scenarios
Example addition:
await t.step("calls failure callback", async () => { await wait(() => host.call("eval", "len(g:__test_denops_events)")); + // Verify success callback was not called + const events = await host.call("eval", "g:__test_denops_events") as unknown[]; + assertEquals( + events.filter((e) => + Array.isArray(e) && e[0] === "TestDenopsRequestAsyncSuccess:Called" + ).length, + 0 + ); assertObjectMatch( - await host.call("eval", "g:__test_denops_events") as unknown[], + events, { "TestDenopsRequestAsyncFailure:Called",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
tests/denops/runtime/functions/denops/request_async_test.ts(1 hunks)tests/denops/runtime/functions/denops/request_test.ts(2 hunks)tests/denops/testdata/dummy_dispatcher_plugin.ts(1 hunks)
🔇 Additional comments (4)
tests/denops/runtime/functions/denops/request_test.ts (2)
96-96: LGTM! Error message improvement.
The updated error message provides better clarity by specifically indicating that the dispatcher method is not a function.
Line range hint 69-96: Verify corresponding implementation changes.
The test changes look good, but let's ensure the implementation properly supports these error scenarios.
✅ Verification successful
Implementation properly supports the test scenarios
The implementation in service.ts correctly handles the error scenarios being tested:
- In the
Plugin.call()method (line 271-280), it properly wraps dispatcher method errors with the exact error message format being tested:
async call(fn: string, ...args: unknown[]): Promise<unknown> {
try {
return await this.#denops.dispatcher[fn](...args);
} catch (err) {
const errMsg = err instanceof Error ? err.message : String(err);
throw new Error(
`Failed to call '${fn}' API in '${this.name}': ${errMsg}`,
);
}
}This implementation correctly handles both test cases:
- When the dispatcher method throws an error (the "Dummy failure" case)
- When the method doesn't exist (the "not a function" case)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of error handling in the source code
rg -A 5 "Failed to call.*API in.*:" --glob '!tests/**'
Length of output: 349
Script:
#!/bin/bash
# Let's examine the implementation in service.ts to understand the error handling
ast-grep --pattern 'class $_ {
$$$
async request($$$) {
$$$
}
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# Let's try a different approach to find the request implementation
cat denops/@denops-private/service.ts
Length of output: 9716
tests/denops/runtime/functions/denops/request_async_test.ts (2)
118-150: LGTM! Well-structured test case for error handling.
The test case effectively verifies both immediate return behavior and proper failure callback invocation. The structure follows the established patterns and maintains consistency with other tests in the file.
122-122: Verify the dummy dispatcher implementation.
The test calls the 'fail' method, which should be implemented in the dummy dispatcher plugin.
✅ Verification successful
The 'fail' method is properly implemented in the dummy dispatcher plugin
The verification shows that the 'fail' method is implemented in tests/denops/testdata/dummy_dispatcher_plugin.ts as an async function that throws a "Dummy failure" error after a delay, which is exactly what's needed for testing the failure callback in the test case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the 'fail' method implementation in the dummy dispatcher plugin
# Find and check the content of the dummy dispatcher plugin
rg -A 5 "fail.*=.*async.*function" "$(fd -t f dummy_dispatcher_plugin.ts)"
Length of output: 123
Script:
#!/bin/bash
# Let's first check if the file exists and its content
fd -t f dummy_dispatcher_plugin.ts
# Let's also try to find any file containing 'dummy' in its name
fd -t f dummy
# Let's check for any file containing 'fail' method implementation
rg -A 5 "fail.*="
Length of output: 13074
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
denops/@denops-private/service.ts (2)
278-283: Consider unifying error handling withtoVimErrorThe
toVimErrorutility function already handles stack traces, but it's only used in thedispatchmethod. Consider using it in thecallmethod as well to maintain consistency in error handling across sync and async operations.Here's a suggested refactor:
} catch (err) { - const errMsg = err instanceof Error - ? err.stack ?? err.message // Prefer 'stack' if available - : String(err); + const errMsg = toVimError(err); throw new Error( `Failed to call '${fn}' API in '${this.name}': ${errMsg}`, ); }Also applies to: 391-397
Line range hint
146-159: Consider standardizing error format between sync and async dispatchThe async dispatch uses
toErrorObjectwhile sync dispatch usestoVimError, which could lead to inconsistent error reporting. Consider standardizing the error format to provide a consistent debugging experience.Some options to consider:
- Use
toVimErrorconsistently across both sync and async paths- Create a new utility function that handles both cases uniformly
- Document the intentional difference if it serves a specific purpose
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
denops/@denops-private/service.ts(1 hunks)tests/denops/runtime/functions/denops/request_async_test.ts(3 hunks)tests/denops/runtime/functions/denops/request_test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/denops/runtime/functions/denops/request_async_test.ts
- tests/denops/runtime/functions/denops/request_test.ts
🔇 Additional comments (1)
denops/@denops-private/service.ts (1)
278-280: LGTM! Enhanced error reporting implementation.
The implementation correctly prioritizes the stack trace while falling back to the error message, which aligns with the PR objective of improving error reporting. The use of the nullish coalescing operator ensures graceful fallback.
Let's verify the consistency of error handling across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
Error handling is consistent across the codebase
The codebase already follows consistent error handling patterns. The enhanced error reporting with stack traces in the reviewed code is unique to the call method as it's a critical error boundary where detailed error information is valuable for plugin debugging. Other error instances are primarily for control flow and specific error conditions where stack traces aren't as crucial.
Key observations:
- Most error constructions are for specific, known error conditions (e.g., "Service closed", "No host is bound")
- Test files appropriately use simple error messages
- The enhanced error reporting is correctly placed at the plugin execution boundary where stack traces are most valuable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar error handling patterns that might benefit from the same enhancement
# Search for Error message construction patterns
rg "new Error\(" -A 2
# Search for error handling in catch blocks
ast-grep --pattern 'catch (err) {
$$$
}'
Length of output: 9384
Continue from #428
The After shows that the original error was occcurred in
dummy_dispatcher_plugin.ts:19:13but Before. This stack-trace will help developers to find a reason of issues.Before
After
Summary by CodeRabbit
Summary by CodeRabbit
New Features
failmethod in the dispatcher to simulate error scenarios.Bug Fixes
Tests