Conversation
Update integration tests to properly use NeovimIpcGuard for process lifecycle management. This ensures Neovim instances are correctly terminated after test completion, preventing resource leaks in the test suite. Changes: - Replace direct guard assignment with explicit NeovimIpcGuard::new() - Apply fix to organize_imports and lua_tools test functions - Maintain consistent variable naming for IPC paths
Replace per-test cargo run with pre-compiled binary approach: - Add get_compiled_binary() function with OnceLock for single compilation - Create create_mcp_service! macro for consistent binary usage - Reduce test execution time by eliminating redundant compilations - Maintain same test functionality with improved performance
- Reduce LSP initialization wait times from 15-20s to 2-3s - Decrease save operation wait from 1000ms to 100ms - Reduce connection polling interval from 100ms to 50ms - Optimize Lua tools test wait from 2000ms to 500ms - Maintain test reliability while significantly improving speed
- Replace diagnostics_autocmd.lua with setup_autocmd.lua for enhanced autocmd management - Rename setup_diagnostics_changed_autocmd to setup_autocmd for cleaner API - Add NotificationTracker for async notification handling with timeout support - Add LspAttach and LspDetach autocmd handlers with detailed client information - Update all integration tests to use new method name - Enhance notification system with memory management and proper cleanup
Add wait_for_lsp_ready and wait_for_diagnostics methods to properly synchronize tests with LSP initialization. Implement notification cleanup to prevent memory leaks and ensure test stability. Replace arbitrary sleep calls with deterministic LSP readiness checks. Key improvements: - Add wait_for_lsp_ready() and wait_for_diagnostics() methods to NeovimClientTrait - Implement notification expiry and cleanup mechanisms in NotificationTracker - Add helper functions for common LSP setup patterns in integration tests - Replace sleep-based timing with event-driven synchronization - Improve test reliability by waiting for actual LSP attachment events
- Add NeovimClientConfig struct with configurable lsp_timeout_ms (default: 3000ms) - Replace hardcoded 1000ms timeouts with configurable timeout across all LSP operations - Add with_config() method for client configuration - Make unused debugging methods private with allow(dead_code) attributes
- Add wait_for_lsp_ready tool documentation across all files - Update tool count from 23 to 24 tools - Document configurable LSP timeout settings - Include notification tracking system improvements - Add enhanced test reliability information
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances LSP integration with configurable timeouts and improves test reliability through optimized performance and robust synchronization mechanisms.
- Adds configurable LSP timeout settings and comprehensive notification tracking for better LSP operation control
- Implements robust LSP synchronization with
wait_for_lsp_readytool and notification management system - Optimizes integration test performance through one-time binary compilation and improved timing controls
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test_utils.rs | Reduces polling interval from 100ms to 50ms for faster Neovim startup detection |
| src/server/tools.rs | Adds new wait_for_lsp_ready tool and updates autocmd setup method calls |
| src/server/integration_tests.rs | Implements one-time binary compilation optimization and replaces hardcoded timeouts with LSP readiness checks |
| src/server/core.rs | Updates autocmd setup method call to use new unified approach |
| src/neovim/lua/setup_autocmd.lua | New comprehensive autocmd setup handling diagnostics and LSP events |
| src/neovim/lua/diagnostics_autocmd.lua | Removes old diagnostics-only autocmd file |
| src/neovim/integration_tests.rs | Replaces fixed sleep delays with LSP readiness helper functions |
| src/neovim/client.rs | Adds notification tracking system, configurable timeouts, and LSP synchronization methods |
| docs/instructions.md | Documents new wait_for_lsp_ready tool |
| README.md | Updates tool count and documents new LSP readiness functionality |
| CLAUDE.md | Updates documentation with new features and tool count |
| CHANGELOG.md | Documents new features and improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
• Add configurable LSP timeout to NeovimClient for better control over LSP operations
• Implement robust LSP synchronization with notification tracking system
• Refactor autocmd setup with enhanced notification management
• Optimize integration test performance with one-time binary compilation
• Improve test reliability with better Neovim process cleanup
• Update documentation for new LSP readiness and configuration features