Skip to content

Add comprehensive rename functionality tests for nimlsp#183

Merged
PMunch merged 2 commits into
PMunch:masterfrom
bung87:rename-verify
Jul 10, 2025
Merged

Add comprehensive rename functionality tests for nimlsp#183
PMunch merged 2 commits into
PMunch:masterfrom
bung87:rename-verify

Conversation

@bung87

@bung87 bung87 commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

Add comprehensive rename functionality tests for nimlsp

Summary

This PR adds a comprehensive test suite for the textDocument/rename LSP functionality, including cross-file rename testing and documentation of current limitations.

Changes Made

New Test Files

  • tests/trename.nim - Main test file for textDocument/rename functionality
  • tests/rename_test_project/ - Test project with multiple files for cross-file testing
    • rename_test.nim - Source file with exported variables, functions, and constants
    • other_file.nim - Importing file that references symbols from the main file
    • rename_test.nimble - Nimble project configuration
  • tests/run_rename_test.nim - Test runner script

Test Coverage

The test suite covers:

  • Variable renaming - Tests renaming exported variables
  • Function renaming - Tests renaming exported functions
  • Constant renaming - Tests renaming exported constants
  • Cross-file renaming - Tests renaming from both definition and usage files
  • Response validation - Verifies proper LSP response structure and text edits
  • Version warning handling - Gracefully handles nimlsp version mismatch warnings

Key Findings

During testing, we discovered an important limitation in the current nimlsp implementation:

Cross-file rename behavior:

  • Rename from usage/import file: Updates both definition and usage files
  • Rename from definition file: Only updates the definition file

This reveals a limitation in nimlsp/nimsuggest symbol resolution where cross-file references are only detected when the rename request originates from the file that imports/uses the symbol.

Test Output

The test includes explanatory output that documents this behavior:

[NOTE] Cross-file rename: Only renaming from the usage/import file updates both files.
Renaming from the definition file only updates the definition file.
This is a current limitation of nimlsp/nimsuggest symbol resolution.

bung87 added 2 commits July 9, 2025 23:19
- Add trename.nim test file that tests textDocument/rename requests
- Create rename_test_project with test files for cross-file renaming
- Test renaming variables, functions, and constants across multiple files
- Verify that LSP returns proper WorkspaceEdit responses with text edits
- Handle version warnings gracefully in test
- Follow existing test patterns from tnimlsp.nim
- Add test case for renaming from importing file perspective
- Update nimble file with proper project requirements
- Add explanatory note about cross-file rename behavior
- Document that rename from usage file works, but from definition file doesn't
- This reveals a limitation in nimlsp/nimsuggest symbol resolution
@bung87 bung87 changed the title Rename verify Add comprehensive rename functionality tests for nimlsp Jul 9, 2025
@PMunch

PMunch commented Jul 10, 2025

Copy link
Copy Markdown
Owner

Very nice to have some tests! I have noticed the "Rename from definition file" bug before. But haven't gotten around to fix it. It's probably just a matter of figuring out which nimsuggest commands would give the right list of symbols.

@PMunch PMunch merged commit 138680a into PMunch:master Jul 10, 2025
9 checks passed
@bung87

bung87 commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

yeah, I rarely use rename function in nim project, but likely it doesn't work well every time during these years, so I let AI help me write this test to see what's in the deep.

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.

2 participants