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

👍 denops#plugin#check_type() shows 'no plugins are loaded' message #407

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

Milly
Copy link
Contributor

@Milly Milly commented Aug 7, 2024

Fixes #406

Include to split functions/plugin tests. But test codes only changed for denops#plugin#check_type().
Why is a split test included? Because splitting it revealed this bug.

Summary by CodeRabbit

  • New Features

    • Enhanced plugin management with improved feedback for plugin loading and error handling.
    • Added a documentation update clarifying type checks for loaded plugins.
  • Bug Fixes

    • Improved handling of invalid plugin names and error scenarios during plugin operations.
  • Tests

    • Introduced comprehensive test suites for various Denops plugin functions, ensuring reliable behavior across loading, unloading, and reloading operations.

Copy link

coderabbitai bot commented Aug 7, 2024

Walkthrough

The recent changes enhance the denops#plugin#check_type() function by improving plugin retrieval and user feedback. Key updates include separating plugin list handling from script processing, providing user-friendly messages when no plugins are loaded, and introducing comprehensive unit tests for various scenarios, ensuring robust functionality.

Changes

Files Change Summary
autoload/denops/plugin.vim, doc/denops.txt Refined the check_type function to improve plugin handling and user feedback. Updated documentation to clarify that checks apply to loaded plugins.
tests/denops/runtime/functions/plugin/*.test.ts Introduced comprehensive test suites for multiple plugin functions (check_type, discover, is_loaded, load, reload, unload, wait_async, wait), enhancing test coverage and reliability.

Assessment against linked issues

Objective Addressed Explanation
denops#plugin#check_type() shows user-friendly messages for unloaded plugins (#406)
Provide clarity in documentation regarding loaded plugins (#406)

🐰 In a world of plugins, hopping so bright,
A rabbit brings changes, oh what a delight!
No more confusion when things aren't found,
User-friendly messages now abound.
With tests all around, our code's sure to thrive,
In Denops' garden, our plugins come alive! 🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Milly Milly force-pushed the plugin-type-check-no-loaded branch from b205ab5 to 33ff712 Compare August 7, 2024 16:25
@Milly Milly requested a review from lambdalisue August 7, 2024 16:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (3)
tests/denops/runtime/functions/plugin/reload_test.ts (1)

11-11: Clarify the unit of MESSAGE_DELAY.

The comment indicates msc, which seems to be a typo. It should be ms for milliseconds.

- const MESSAGE_DELAY = 200; // msc
+ const MESSAGE_DELAY = 200; // ms
tests/denops/runtime/functions/plugin/load_test.ts (1)

11-11: Clarify the unit of MESSAGE_DELAY.

The comment indicates msc, which seems to be a typo. It should be ms for milliseconds.

- const MESSAGE_DELAY = 200; // msc
+ const MESSAGE_DELAY = 200; // ms
tests/denops/runtime/functions/plugin/wait_test.ts (1)

14-14: Clarify the unit of MESSAGE_DELAY.

The comment indicates msc, which seems to be a typo. It should be ms for milliseconds.

- const MESSAGE_DELAY = 200; // msc
+ const MESSAGE_DELAY = 200; // ms
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 95f076b and 33ff712.

Files selected for processing (10)
  • autoload/denops/plugin.vim (1 hunks)
  • doc/denops.txt (1 hunks)
  • tests/denops/runtime/functions/plugin/check_type_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/discover_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/is_loaded_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/load_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/reload_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/unload_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/wait_async_test.ts (1 hunks)
  • tests/denops/runtime/functions/plugin/wait_test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
  • doc/denops.txt
Additional comments not posted (90)
tests/denops/runtime/functions/plugin/discover_test.ts (7)

1-9: LGTM!

The imports and constants are correctly set up for the test case.


15-20: Approve the test host setup.

The test host setup is comprehensive and correctly initializes the necessary components.


38-48: Approve the plugin loading verification.

The step correctly verifies that the denops plugins are loaded by checking the events.


50-60: Approve the event firing verification.

The step correctly verifies that the DenopsPlugin* events are fired as expected.


62-71: Approve the invalid plugin name verification.

The step correctly verifies that invalid plugin names are not loaded.


73-75: Approve the plugin entrypoint verification.

The step correctly verifies that the plugin entrypoint is called.


77-83: Approve the error message verification.

The step correctly verifies that an error message is output after a delay.

autoload/denops/plugin.vim (8)

Line range hint 1-3:
Approve the denops#plugin#is_loaded function.

The function correctly checks if a plugin is loaded or failed.


Line range hint 5-21:
Approve the denops#plugin#wait function.

The function correctly waits for a plugin to be loaded with proper error handling and timeout management.


Line range hint 23-31:
Approve the denops#plugin#wait_async function.

The function correctly waits for a plugin to be loaded asynchronously with proper callback handling.


Line range hint 33-35:
Approve the denops#plugin#load function.

The function correctly loads a plugin with the given name and script.


Line range hint 37-39:
Approve the denops#plugin#unload function.

The function correctly unloads a plugin with the given name.


Line range hint 41-43:
Approve the denops#plugin#reload function.

The function correctly reloads a plugin with the given name.


Line range hint 45-55:
Approve the denops#plugin#discover function.

The function correctly discovers plugins in the runtime path with proper handling of valid plugin names and file readability.


Line range hint 57-88:
Approve the denops#plugin#check_type function.

The function correctly checks the type of plugins with improved readability and user feedback.

However, ensure that the function usage matches the updated implementation.

Verification successful

No other usages of denops#plugin#check_type found.

The function denops#plugin#check_type is correctly implemented and there are no other usages of this function in the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `denops#plugin#check_type` match the updated implementation.

# Test: Search for the function usage. Expect: Only occurrences of the updated implementation.
rg --type vim -A 5 $'denops#plugin#check_type'

Length of output: 387

tests/denops/runtime/functions/plugin/check_type_test.ts (11)

1-9: LGTM!

The imports and constants are correctly set up for the test case.


12-27: Approve the test host setup.

The test host setup is comprehensive and correctly initializes the necessary components.


29-40: Approve the no plugins loaded scenario.

The step correctly verifies that an info message is output when no plugins are loaded.


42-56: Approve the some plugins loaded scenario.

The step correctly verifies that a type check message is output when some plugins are loaded.


58-76: Approve the invalid plugins scenario.

The step correctly verifies that an error message is output when invalid plugins are tried to load.


79-87: Approve the invalid plugin name scenario.

The step correctly verifies that an error is thrown when an invalid plugin name is specified.


90-104: Approve the not yet loaded plugin scenario.

The step correctly verifies that an info message is output when a plugin is not yet loaded.


106-124: Approve the not exists plugin scenario.

The step correctly verifies that an error message is output when a plugin does not exist.


126-147: Approve the loaded plugin scenario.

The step correctly verifies that a type check message is output when a plugin is loaded.


149-179: Approve the unloaded plugin scenario.

The step correctly verifies that a type check message is output when a plugin is unloaded.


181-211: Approve the reloaded plugin scenario.

The step correctly verifies that a type check message is output when a plugin is reloaded.

tests/denops/runtime/functions/plugin/is_loaded_test.ts (8)

31-39: Test case for invalid plugin name looks good.

The test correctly verifies that an error is thrown for an invalid plugin name.


42-50: Test case for unloaded plugin looks good.

The test correctly verifies that the function returns 0 when the plugin is not loaded.


52-89: Comprehensive test case for plugin entrypoint errors looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


91-137: Comprehensive test case for plugin dispose method errors looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


139-183: Comprehensive test case for plugin loading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


185-237: Comprehensive test case for plugin unloading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


240-309: Comprehensive test case for plugin reloading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


313-316: resolve function implementation looks good.

The function correctly constructs the path to test data scripts.

tests/denops/runtime/functions/plugin/wait_async_test.ts (8)

32-43: Test case for invalid plugin name looks good.

The test correctly verifies that an error is thrown for an invalid plugin name.


46-84: Comprehensive test case for asynchronous plugin loading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


86-119: Comprehensive test case for plugin loading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


122-152: Comprehensive test case for already loaded plugin looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


155-197: Comprehensive test case for plugin reloading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


201-241: Comprehensive test case for already reloaded plugin looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


244-273: Comprehensive test case for plugin loading failures looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


275-308: Comprehensive test case for plugin load failures looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.

tests/denops/runtime/functions/plugin/unload_test.ts (8)

33-42: Test case for invalid plugin name looks good.

The test correctly verifies that an error is thrown for an invalid plugin name.


44-67: Comprehensive test case for unloaded plugin looks good.

The test covers various scenarios and ensures correct return values.


69-106: Comprehensive test case for plugin dispose method errors looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


108-135: Comprehensive test case for plugin loading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


137-171: Comprehensive test case for already loaded plugin looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


173-219: Comprehensive test case for plugin unloading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


221-263: Comprehensive test case for already unloaded plugin looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.


265-313: Comprehensive test case for plugin reloading looks good.

The test covers various stages of the plugin lifecycle and ensures correct return values.

tests/denops/runtime/functions/plugin/reload_test.ts (12)

1-7: Ensure import paths are correct and dependencies are up-to-date.

The imports from jsr:@std and /denops-testutil should be verified to ensure they are correct and up-to-date. If these paths are incorrect, the tests will fail to run.


13-14: Verify the script paths.

Ensure that the paths to dummy_valid_plugin.ts and dummy_invalid_dispose_plugin.ts are correct and the files exist in the specified location.


16-21: Good structure for test setup.

The testHost function is well-structured, setting up the test environment and specifying the postlude script.


33-42: Test case for invalid plugin name is well-implemented.

The test case correctly checks for an error when an invalid plugin name is provided. The usage of assertRejects with the expected error message is appropriate.


44-67: Test case for non-loaded plugin is comprehensive.

The test case covers multiple scenarios for a plugin that is not yet loaded, including checking for no reload, no events, and no output messages. The use of assertRejects and assertEquals is appropriate.


69-107: Test case for plugin with throwing dispose method is thorough.

The test case handles a plugin whose dispose method throws an error. It checks for reloading, event firing, and error message output. The use of assertMatch to check the error message is appropriate.


110-140: Test case for loading plugin is well-structured.

The test case covers scenarios for a plugin that is currently loading, including reloading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


142-178: Test case for loaded plugin is comprehensive.

The test case covers scenarios for a plugin that is already loaded, including reloading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


180-219: Test case for unloading plugin is thorough.

The test case handles scenarios for a plugin that is currently unloading, including checking for no reload, event firing, and output messages. The use of assertMatch to check the output is appropriate.


221-293: Test case for unloaded plugin is comprehensive.

The test case covers scenarios for a plugin that is already unloaded, including reloading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


315-361: Test case for reloaded plugin is comprehensive.

The test case covers scenarios for a plugin that is already reloaded, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertMatch to check the output is appropriate.


364-367: Resolve function is well-implemented.

The resolve function correctly constructs the path to the test data scripts. Ensure that the paths are correct and the files exist.

tests/denops/runtime/functions/plugin/load_test.ts (14)

1-7: Ensure import paths are correct and dependencies are up-to-date.

The imports from jsr:@std and /denops-testutil should be verified to ensure they are correct and up-to-date. If these paths are incorrect, the tests will fail to run.


13-15: Verify the script paths.

Ensure that the paths to dummy_valid_plugin.ts, dummy_invalid_plugin.ts, and dummy_valid_dispose_plugin.ts are correct and the files exist in the specified location.


17-22: Good structure for test setup.

The testHost function is well-structured, setting up the test environment and specifying the postlude script.


34-43: Test case for invalid plugin name is well-implemented.

The test case correctly checks for an error when an invalid plugin name is provided. The usage of assertRejects with the expected error message is appropriate.


45-69: Test case for non-loaded plugin is comprehensive.

The test case covers multiple scenarios for a plugin that is not yet loaded, including checking for loading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


71-99: Test case for plugin with throwing entrypoint is thorough.

The test case handles a plugin whose entrypoint throws an error. It checks for loading failure, event firing, and error message output. The use of assertMatch to check the error message is appropriate.


101-127: Test case for same script with different name is well-structured.

The test case covers scenarios for loading the same script with a different name, including event firing and entry point calls. The use of assertMatch to check the output is appropriate.


130-166: Test case for loading plugin is comprehensive.

The test case covers scenarios for a plugin that is currently loading, including checking for no double loading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


168-201: Test case for loaded plugin is thorough.

The test case handles scenarios for a plugin that is already loaded, including checking for no double loading, event firing, and output messages. The use of assertMatch to check the output is appropriate.


203-248: Test case for unloading plugin is comprehensive.

The test case covers scenarios for a plugin that is currently unloading, including unloading, event firing, and dispose method calls. The use of assertMatch to check the output is appropriate.


250-293: Test case for unloaded plugin is well-structured.

The test case covers scenarios for a plugin that is already unloaded, including reloading, event firing, and entry point calls. The use of assertMatch to check the output is appropriate.


295-343: Test case for reloading plugin is comprehensive.

The test case covers scenarios for a plugin that is currently reloading, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertMatch to check the output is appropriate.


345-387: Test case for reloaded plugin is thorough.

The test case handles scenarios for a plugin that is already reloaded, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertMatch to check the output is appropriate.


391-394: Resolve function is well-implemented.

The resolve function correctly constructs the path to the test data scripts. Ensure that the paths are correct and the files exist.

tests/denops/runtime/functions/plugin/wait_test.ts (14)

1-11: Ensure import paths are correct and dependencies are up-to-date.

The imports from jsr:@std and /denops-testutil should be verified to ensure they are correct and up-to-date. If these paths are incorrect, the tests will fail to run.


16-19: Verify the script paths.

Ensure that the paths to dummy_valid_plugin.ts, dummy_invalid_plugin.ts, dummy_valid_wait_plugin.ts, and dummy_invalid_wait_plugin.ts are correct and the files exist in the specified location.


21-26: Good structure for test setup.

The testHost function is well-structured, setting up the test environment and specifying the postlude script.


38-47: Test case for invalid plugin name is well-implemented.

The test case correctly checks for an error when an invalid plugin name is provided. The usage of assertRejects with the expected error message is appropriate.


49-81: Test case for loading plugin is comprehensive.

The test case covers scenarios for a plugin that is currently loading, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertArrayIncludes to check the events is appropriate.


83-115: Test case for loaded plugin is thorough.

The test case handles scenarios for a plugin that is already loaded, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertLess to check the elapsed time is appropriate.


117-159: Test case for reloading plugin is comprehensive.

The test case covers scenarios for a plugin that is currently reloading, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertArrayIncludes to check the events is appropriate.


161-201: Test case for reloaded plugin is well-structured.

The test case covers scenarios for a plugin that is already reloaded, including waiting for the plugin to load, returning the correct result, and event firing. The use of assertLess to check the elapsed time is appropriate.


204-236: Test case for loading and failing plugin is thorough.

The test case handles scenarios for a plugin that is loading and fails, including waiting for the plugin to fail, returning the correct result, and event firing. The use of assertArrayIncludes to check the events is appropriate.


238-272: Test case for failed plugin is comprehensive.

The test case covers scenarios for a plugin that has failed to load, including waiting for the plugin to fail, returning the correct result, and event firing. The use of assertLess to check the elapsed time is appropriate.


274-337: Test case for timeout scenarios is well-implemented.

The test case handles scenarios where the plugin wait times out, including checking for the correct result and output messages. The use of assertStringIncludes to check the error message is appropriate.


341-355: Test case for internal timeout is thorough.

The test case covers scenarios where the internal timeout expires, including checking for the correct result and warning messages. The use of assertStringIncludes to check the warning message is appropriate.


357-425: Test case for stopped server is comprehensive.

The test case handles scenarios where the Denops server is stopped, including checking for the correct result and output messages. The use of assertStringIncludes to check the error message is appropriate.


431-433: Resolve function is well-implemented.

The resolve function correctly constructs the path to the test data scripts. Ensure that the paths are correct and the files exist.

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.44%. Comparing base (95f076b) to head (33ff712).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
- Coverage   95.52%   95.44%   -0.08%     
==========================================
  Files          23       23              
  Lines        1384     1384              
  Branches      174      174              
==========================================
- Hits         1322     1321       -1     
- Misses         59       60       +1     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lambdalisue lambdalisue merged commit eb9ef6a into main Aug 7, 2024
8 of 9 checks passed
@lambdalisue lambdalisue deleted the plugin-type-check-no-loaded branch August 7, 2024 17:50
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.

denops#plugin#check_type() shows a usage help message of deno check
2 participants