👍 rejects if the plugin name is invalid #418
Conversation
WalkthroughThe changes encompass the addition of validation for plugin names across several files in the Denops framework. A new function, Changes
Possibly related PRs
Poem
Tip OpenAI O1 model for chat
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
+ Coverage 95.90% 95.96% +0.05%
==========================================
Files 24 25 +1
Lines 1393 1412 +19
Branches 174 179 +5
==========================================
+ Hits 1336 1355 +19
Misses 55 55
Partials 2 2 ☔ 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- autoload/denops/_internal/plugin.vim (1 hunks)
- denops/@denops-private/service.ts (5 hunks)
- denops/@denops-private/service_test.ts (9 hunks)
- tests/denops/runtime/functions/denops/notify_test.ts (1 hunks)
- tests/denops/runtime/functions/denops/request_async_test.ts (1 hunks)
- tests/denops/runtime/functions/denops/request_test.ts (1 hunks)
- tests/denops/runtime/functions/plugin/check_type_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/is_loaded_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/load_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/reload_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/unload_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/wait_async_test.ts (2 hunks)
- tests/denops/runtime/functions/plugin/wait_test.ts (2 hunks)
- tests/denops/testdata/invalid_plugin_names.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- autoload/denops/_internal/plugin.vim
Additional comments not posted (32)
tests/denops/testdata/invalid_plugin_names.ts (1)
1-6: LGTM!The
INVALID_PLUGIN_NAMESconstant provides a clear and concise reference for identifying invalid plugin names. The use of tuples with descriptive labels enhances the readability and maintainability of the test data.The use of
readonlyis a good practice to ensure the integrity of the test data.Overall, this addition aligns well with the PR objective of improving the validation of plugin names.
tests/denops/runtime/functions/denops/request_test.ts (3)
1-25: LGTM!The test setup is comprehensive and follows best practices:
- Imports necessary dependencies
- Sets up the test environment using
testHost- Initializes variables to capture error outputs
- Waits for the Denops server to be running
- Sets up an autocommand to capture plugin events
26-36: LGTM!The test case for invalid plugin names is well-structured and comprehensive:
- Iterates over a predefined list of invalid plugin names
- Asserts that calling
denops#requestwith each invalid name throws an appropriate error- Ensures comprehensive coverage of invalid plugin name scenarios
38-55: LGTM!The test case for a valid plugin is well-structured and comprehensive:
- Loads a valid plugin and waits for it to be initialized
- Verifies that the plugin is loaded by checking the captured plugin events
- Calls
denops#requestwith valid parameters once the plugin is loaded- Asserts that the output includes the expected confirmation message, indicating successful execution of the request
tests/denops/runtime/functions/denops/notify_test.ts (1)
12-61: LGTM!The
testHostfunction is well-structured and follows best practices for testing Denops plugins. It sets up a test environment using thetestHostutility, tests the behavior ofdenops#notify()with invalid plugin names using theINVALID_PLUGIN_NAMESconstant, resolves the path to a valid plugin script using theresolveTestDataPathutility, waits for asynchronous operations to complete using thewaitutility, introduces a delay in the tests using thedelayutility, and makes assertions using theassertEqualsandassertStringIncludesutilities.The function is testing the behavior of
denops#notify()under various conditions, including invalid plugin names and a valid plugin, which is a good practice for ensuring the robustness of the code.Overall, the function is well-written and enhances the reliability of the Denops framework by ensuring that the notification system behaves correctly under both valid and invalid conditions.
tests/denops/runtime/functions/denops/request_async_test.ts (3)
1-9: Imports are well-structured and promote code reusability.The imports are organized and follow a clear naming convention. The use of separate test data and utility files enhances code reusability and maintainability.
13-116: Comprehensive test suite with clear structure and assertions.The test suite for
denops#request_async()is well-designed and covers both valid and invalid scenarios. The tests are structured logically and use clear assertions to verify the expected behavior. The use of async/await and utility functions enhances the readability and reliability of the tests.
36-71: Thorough testing of invalid plugin name handling.The tests comprehensively cover the handling of invalid plugin names by iterating over a list of invalid names and verifying the expected behavior. The use of a loop reduces code duplication and improves maintainability. The assertions are clear and accurately verify the invocation of the failure callback with the expected error message.
tests/denops/runtime/functions/plugin/check_type_test.ts (1)
80-90: LGTM!The test case is well-structured, comprehensive, and follows the existing test case structure. It covers all the invalid plugin names provided in the
INVALID_PLUGIN_NAMESarray and uses theassertRejectsfunction to check if thedenops#plugin#check_typefunction throws an error with the appropriate message.denops/@denops-private/service.ts (5)
48-48: LGTM!The addition of plugin name validation using the
assertValidPluginNamefunction is a good improvement. It ensures that only valid plugin names are loaded, enhancing the robustness of the system.
83-83: LGTM!The addition of plugin name validation using the
assertValidPluginNamefunction is a good improvement. It ensures that only valid plugin names are unloaded, enhancing the robustness of the system.
88-88: LGTM!The addition of plugin name validation using the
assertValidPluginNamefunction is a good improvement. It ensures that only valid plugin names are reloaded, enhancing the robustness of the system.
95-100: LGTM!The addition of plugin name validation using the
assertValidPluginNamefunction is a good improvement. It ensures that only valid plugin names are waited for, enhancing the robustness of the system.The modification to use
async/awaitsyntax also improves readability and consistency in handling asynchronous operations.
118-118: LGTM!The addition of plugin name validation using the
assertValidPluginNamefunction is a good improvement. It ensures that only valid plugin names are dispatched to, enhancing the robustness of the system.tests/denops/runtime/functions/plugin/is_loaded_test.ts (2)
3-3: LGTM!The import statement is correctly formatted and follows the existing import style in the file.
34-44: Excellent test case!The new test case is well-structured, follows the existing test style, and improves the robustness of the tests by ensuring that multiple invalid inputs are validated. The use of
assertRejects,for...ofloop, and template literals is spot-on.tests/denops/runtime/functions/plugin/wait_async_test.ts (2)
8-8: LGTM!Importing the invalid plugin names from an external file is a good practice for maintainability and reusability.
33-46: LGTM!The test case correctly loops over the array of invalid plugin names and asserts that calling
denops#plugin#wait_async()with each invalid name throws an error with the expected message. Including the invalid plugin name in the error message is helpful for debugging.tests/denops/runtime/functions/plugin/unload_test.ts (2)
7-7: LGTM!Importing the invalid plugin names from an external file is a good practice for maintainability and reusability of test data.
36-46: Great job enhancing the test coverage!The new test case iterates over multiple invalid plugin names and verifies that the
unloadmethod throws an error with the expected message for each invalid name. This improves the robustness of the test suite by ensuring that the error handling is tested for a broader range of invalid inputs.The test case is well-structured and follows the existing test suite pattern, making it easy to understand and maintain.
tests/denops/runtime/functions/plugin/reload_test.ts (2)
7-7: LGTM!The import statement is correctly written and the imported data is used in the tests.
36-46: Great job enhancing the test coverage!The changes introduce a loop that iterates over multiple invalid plugin names imported from an external file. For each invalid plugin name, the test dynamically generates a step that checks if calling
denops#plugin#reloadwith that name throws an error with the expected message. This improves the robustness of the tests by covering multiple invalid cases rather than a single hardcoded instance.tests/denops/runtime/functions/plugin/load_test.ts (2)
7-7: LGTM!The import statement is correct and the imported data is likely used to enhance the tests for invalid plugin names.
35-45: Excellent enhancement to the invalid plugin name test!The introduction of the test loop that iterates over
INVALID_PLUGIN_NAMESsignificantly improves the robustness and coverage of the test. By dynamically checking each invalid plugin name and generating a specific error message, the test ensures that multiple invalid cases are covered and provides clear feedback on failures.This change broadens the scope of the test and eliminates the reliance on a single hardcoded case, making it more comprehensive and maintainable.
tests/denops/runtime/functions/plugin/wait_test.ts (2)
10-10: LGTM!Importing test data from a separate file is a good practice for maintainability and reusability.
39-49: LGTM!The loop is correctly implemented and improves the test coverage by testing the
denops#plugin#waitfunction with multiple invalid plugin names. Using theassertRejectsfunction to check if the function call throws an error with the expected error message is a good practice.denops/@denops-private/service_test.ts (6)
446-464: LGTM!The test case for
.load()with invalid plugin names is implemented correctly. It properly asserts thatservice.load()rejects with aTypeErrorcontaining the invalid plugin name and verifies that no calls are made to the host.
787-805: LGTM!The test case for
.unload()with invalid plugin names is implemented correctly. It properly asserts thatservice.unload()rejects with aTypeErrorcontaining the invalid plugin name and verifies that no calls are made to the host.
1106-1124: LGTM!The test case for
.reload()with invalid plugin names is implemented correctly. It properly asserts thatservice.reload()rejects with aTypeErrorcontaining the invalid plugin name and verifies that no calls are made to the host.
1222-1235: LGTM!The test case for
.waitLoaded()with invalid plugin names is implemented correctly. It properly asserts thatservice.waitLoaded()rejects with aTypeErrorcontaining the invalid plugin name.
1379-1392: LGTM!The test case for
.dispatch()with invalid plugin names is implemented correctly. It properly asserts thatservice.dispatch()rejects with a string error containing the invalid plugin name.
1620-1652: LGTM!The test case for
.dispatchAsync()with invalid plugin names is implemented correctly. It properly asserts thatservice.dispatchAsync()resolves but calls the failure callback with an error object containing the invalid plugin name.
Fixes part of #415.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation