feat(neovim): add plenary.nvim-based testing with CI/CD - #395
Conversation
- Add tests/ directory with minimal_init.lua for isolated test environment - Create utils_spec.lua tests for cycle_buffer, copen, and cclear functions - Create config_spec.lua tests for config loading and nvim API verification - Add run_tests.sh script that handles plenary setup and test execution - Add Makefile targets: neovim-test and neovim-test-dev - Update lua.yml workflow with lua-neovim-test job Tests can be run locally with `make neovim-test` or via Nix dev shell with `make neovim-test-dev`. CI runs tests on every push/PR.
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughA comprehensive test infrastructure for Neovim configuration is introduced. New CI workflow job, Makefile targets, test runner script, minimal initialization file, and two test suites are added to verify Neovim setup and utility functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust testing framework for the Neovim configuration, leveraging Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRAdded Plenary.nvim-based testing for Neovim configurations with CI/CD integration. What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a testing framework for the Neovim configuration using plenary.nvim, which is a great step towards ensuring configuration stability. The changes include new Makefile targets, a test runner script, a minimal init file for isolated testing, and initial test suites for configuration and utility functions.
My review focuses on improving the robustness and maintainability of the new testing infrastructure. I've suggested:
- Using
after_eachblocks for reliable test cleanup to prevent state leakage between tests. - Strengthening test assertions to verify behavior more thoroughly, not just the absence of errors.
- Making the test environment setup more flexible by respecting environment variables.
- A minor consistency improvement in the Makefile.
Overall, this is a solid foundation for testing, and these changes will help make the test suite more reliable.
| .PHONY: neovim-test | ||
| neovim-test: ## Run Neovim tests using plenary.nvim. | ||
| @echo "🧪 Running Neovim tests..." | ||
| @$(PWD)/home-manager/programs/neovim/tests/run_tests.sh |
There was a problem hiding this comment.
For consistency with other parts of the Makefile (e.g., line 503) and for better portability, it's recommended to use $(CURDIR) instead of $(PWD) to refer to the current directory.
@$(CURDIR)/home-manager/programs/neovim/tests/run_tests.sh
| describe("nvim API", function() | ||
| it("should have working buffer API", function() | ||
| local buf = vim.api.nvim_get_current_buf() | ||
| assert.is_number(buf) | ||
| assert.is_true(vim.api.nvim_buf_is_valid(buf)) | ||
| end) | ||
|
|
||
| it("should have working window API", function() | ||
| local win = vim.api.nvim_get_current_win() | ||
| assert.is_number(win) | ||
| assert.is_true(vim.api.nvim_win_is_valid(win)) | ||
| end) | ||
|
|
||
| it("should be able to create and manipulate buffers", function() | ||
| -- Create a new buffer | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| assert.is_true(vim.api.nvim_buf_is_valid(buf)) | ||
|
|
||
| -- Set lines | ||
| vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "hello", "world" }) | ||
| local lines = vim.api.nvim_buf_get_lines(buf, 0, -1, false) | ||
| assert.are.same({ "hello", "world" }, lines) | ||
|
|
||
| -- Clean up | ||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
|
|
||
| it("should handle feedkeys for keymap testing", function() | ||
| -- Create a scratch buffer for testing | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| vim.api.nvim_set_current_buf(buf) | ||
|
|
||
| -- Enter insert mode and type | ||
| vim.api.nvim_feedkeys("itest", "x", false) | ||
| vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes("<Esc>", true, false, true), "x", false) | ||
|
|
||
| local line = vim.api.nvim_get_current_line() | ||
| assert.equals("test", line) | ||
|
|
||
| -- Clean up | ||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
The cleanup logic (e.g., vim.api.nvim_buf_delete) in these tests will not execute if an assertion fails. This can lead to leftover buffers that might interfere with subsequent tests. It's more robust to handle cleanup in an after_each block to ensure it always runs.
describe("nvim API", function()
local bufs_to_clean = {}
after_each(function()
for _, b in ipairs(bufs_to_clean) do
if vim.api.nvim_buf_is_valid(b) then
vim.api.nvim_buf_delete(b, { force = true })
end
end
bufs_to_clean = {}
end)
it("should have working buffer API", function()
local buf = vim.api.nvim_get_current_buf()
assert.is_number(buf)
assert.is_true(vim.api.nvim_buf_is_valid(buf))
end)
it("should have working window API", function()
local win = vim.api.nvim_get_current_win()
assert.is_number(win)
assert.is_true(vim.api.nvim_win_is_valid(win))
end)
it("should be able to create and manipulate buffers", function()
-- Create a new buffer
local buf = vim.api.nvim_create_buf(false, true)
table.insert(bufs_to_clean, buf)
assert.is_true(vim.api.nvim_buf_is_valid(buf))
-- Set lines
vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "hello", "world" })
local lines = vim.api.nvim_buf_get_lines(buf, 0, -1, false)
assert.are.same({ "hello", "world" }, lines)
end)
it("should handle feedkeys for keymap testing", function()
-- Create a scratch buffer for testing
local buf = vim.api.nvim_create_buf(false, true)
table.insert(bufs_to_clean, buf)
vim.api.nvim_set_current_buf(buf)
-- Enter insert mode and type
vim.api.nvim_feedkeys("itest", "x", false)
vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes("<Esc>", true, false, true), "x", false)
local line = vim.api.nvim_get_current_line()
assert.equals("test", line)
end)
end)
| local plenary_paths = { | ||
| vim.fn.stdpath("data") .. "/site/pack/plugins/start/plenary.nvim", | ||
| vim.fn.stdpath("data") .. "/site/pack/plugins/opt/plenary.nvim", | ||
| vim.fn.expand("~/.local/share/nvim/site/pack/plugins/start/plenary.nvim"), | ||
| vim.fn.expand("~/.local/share/nvim/site/pack/plugins/opt/plenary.nvim"), | ||
| -- CI/test environment paths | ||
| "/tmp/plenary.nvim", | ||
| tests_dir .. "/plenary.nvim", | ||
| } | ||
|
|
||
| for _, path in ipairs(plenary_paths) do | ||
| if vim.fn.isdirectory(path) == 1 then | ||
| vim.opt.runtimepath:append(path) | ||
| break | ||
| end | ||
| end |
There was a problem hiding this comment.
The run_tests.sh script exports a PLENARY_DIR environment variable, but this file doesn't use it, relying on a hardcoded path /tmp/plenary.nvim. To make the test setup more flexible and honor the environment variable, you can read it using os.getenv(). This also requires a check to ensure the path is not nil before using it.
local plenary_env_path = os.getenv("PLENARY_DIR")
local plenary_paths = {
-- CI/test environment paths
plenary_env_path,
"/tmp/plenary.nvim",
tests_dir .. "/plenary.nvim",
-- User installation paths
vim.fn.stdpath("data") .. "/site/pack/plugins/start/plenary.nvim",
vim.fn.stdpath("data") .. "/site/pack/plugins/opt/plenary.nvim",
vim.fn.expand("~/.local/share/nvim/site/pack/plugins/start/plenary.nvim"),
vim.fn.expand("~/.local/share/nvim/site/pack/plugins/opt/plenary.nvim"),
}
for _, path in ipairs(plenary_paths) do
if path and vim.fn.isdirectory(path) == 1 then
vim.opt.runtimepath:append(path)
break
end
end
| describe("cycle_buffer", function() | ||
| it("should be a function", function() | ||
| assert.is_function(utils.cycle_buffer) | ||
| end) | ||
|
|
||
| it("should not error with no buffers", function() | ||
| -- With only one buffer (current), should not error | ||
| assert.has_no.errors(function() | ||
| utils.cycle_buffer("next") | ||
| end) | ||
| end) | ||
|
|
||
| it("should not error when cycling previous", function() | ||
| assert.has_no.errors(function() | ||
| utils.cycle_buffer("prev") | ||
| end) | ||
| end) | ||
|
|
||
| it("should handle multiple buffers", function() | ||
| -- Create a second buffer | ||
| local buf1 = vim.api.nvim_get_current_buf() | ||
| vim.cmd("enew") | ||
| local buf2 = vim.api.nvim_get_current_buf() | ||
|
|
||
| -- Should be able to cycle between them | ||
| assert.has_no.errors(function() | ||
| utils.cycle_buffer("next") | ||
| end) | ||
|
|
||
| -- Clean up | ||
| vim.api.nvim_buf_delete(buf2, { force = true }) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
The test for multiple buffers only checks for errors and doesn't verify that the buffer actually changed. Additionally, the cleanup logic will be skipped if an assertion fails. It's better to use after_each for robust cleanup and add assertions to confirm the buffer cycling behavior.
describe("cycle_buffer", function()
local bufs_to_clean = {}
after_each(function()
for _, b in ipairs(bufs_to_clean) do
if vim.api.nvim_buf_is_valid(b) then
vim.api.nvim_buf_delete(b, { force = true })
end
end
bufs_to_clean = {}
end)
it("should be a function", function()
assert.is_function(utils.cycle_buffer)
end)
it("should not error with only one buffer", function()
assert.has_no.errors(function()
utils.cycle_buffer("next")
end)
assert.has_no.errors(function()
utils.cycle_buffer("prev")
end)
end)
it("should cycle between multiple buffers", function()
local orig_buf = vim.api.nvim_get_current_buf()
-- Create a new buffer
vim.cmd("enew")
local new_buf = vim.api.nvim_get_current_buf()
table.insert(bufs_to_clean, new_buf)
assert.are_not.equal(orig_buf, new_buf, "New buffer should be different")
-- We are on new_buf. Cycling next should go to orig_buf.
utils.cycle_buffer("next")
assert.are.equal(orig_buf, vim.api.nvim_get_current_buf(), "Should cycle to original buffer")
-- Cycling next again should go back to new_buf.
utils.cycle_buffer("next")
assert.are.equal(new_buf, vim.api.nvim_get_current_buf(), "Should cycle back to new buffer")
-- Cycling prev should go back to orig_buf.
utils.cycle_buffer("prev")
assert.are.equal(orig_buf, vim.api.nvim_get_current_buf(), "Should cycle prev to original buffer")
end)
end)
| describe("copen", function() | ||
| it("should be a function", function() | ||
| assert.is_function(utils.copen) | ||
| end) | ||
|
|
||
| it("should handle empty quickfix list", function() | ||
| -- Clear quickfix list first | ||
| vim.fn.setqflist({}, "r") | ||
|
|
||
| assert.has_no.errors(function() | ||
| utils.copen() | ||
| end) | ||
| end) | ||
|
|
||
| it("should open quickfix window when items exist", function() | ||
| -- Add items to quickfix list | ||
| vim.fn.setqflist({ | ||
| { filename = "test.lua", lnum = 1, text = "Test item 1" }, | ||
| { filename = "test.lua", lnum = 2, text = "Test item 2" }, | ||
| }, "r") | ||
|
|
||
| utils.copen() | ||
|
|
||
| -- Check if quickfix window is open | ||
| local qf_open = false | ||
| for _, win in ipairs(vim.api.nvim_list_wins()) do | ||
| local buf = vim.api.nvim_win_get_buf(win) | ||
| if vim.bo[buf].buftype == "quickfix" then | ||
| qf_open = true | ||
| break | ||
| end | ||
| end | ||
|
|
||
| assert.is_true(qf_open) | ||
|
|
||
| -- Clean up | ||
| vim.cmd("cclose") | ||
| vim.fn.setqflist({}, "r") | ||
| end) | ||
| end) |
There was a problem hiding this comment.
The cleanup logic here (cclose, setqflist) will not run if an assertion fails. Using an after_each block ensures that the quickfix list is cleared and the window is closed after each test in this suite, making the tests more robust and independent. Using pcall for cclose prevents errors if the window is already closed.
describe("copen", function()
after_each(function()
pcall(vim.cmd, "cclose")
vim.fn.setqflist({}, "r")
end)
it("should be a function", function()
assert.is_function(utils.copen)
end)
it("should handle empty quickfix list", function()
-- Clear quickfix list first
vim.fn.setqflist({}, "r")
assert.has_no.errors(function()
utils.copen()
end)
end)
it("should open quickfix window when items exist", function()
-- Add items to quickfix list
vim.fn.setqflist({
{ filename = "test.lua", lnum = 1, text = "Test item 1" },
{ filename = "test.lua", lnum = 2, text = "Test item 2" },
}, "r")
utils.copen()
-- Check if quickfix window is open
local qf_open = false
for _, win in ipairs(vim.api.nvim_list_wins()) do
local buf = vim.api.nvim_win_get_buf(win)
if vim.bo[buf].buftype == "quickfix" then
qf_open = true
break
end
end
assert.is_true(qf_open)
end)
end)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
home-manager/programs/neovim/tests/run_tests.sh (2)
5-5: Enhance error handling with additional shell options.While
set -eis present, consider addingset -u(error on undefined variables) andset -o pipefail(catch failures in pipes) for more robust error handling, per shell scripting best practices.Apply this diff:
-set -e +set -euo pipefail
39-41: Consider documenting the complex nvim command.The headless nvim invocation with plenary test harness is complex. Adding an inline comment would help maintainers understand the test execution flow, per the guideline to "document complex commands in shell scripts."
Apply this diff:
# Run tests echo -e "${YELLOW}Executing tests...${NC}" +# Run nvim in headless mode with minimal_init.lua and plenary test harness +# Sequential execution ensures tests don't interfere with each other nvim --headless \ -u tests/minimal_init.lua \ -c "lua require('plenary.test_harness').test_directory('tests/', { minimal_init = 'tests/minimal_init.lua', sequential = true })"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/lua.yml(2 hunks)Makefile(1 hunks)home-manager/programs/neovim/tests/config_spec.lua(1 hunks)home-manager/programs/neovim/tests/minimal_init.lua(1 hunks)home-manager/programs/neovim/tests/run_tests.sh(1 hunks)home-manager/programs/neovim/tests/utils_spec.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
home-manager/programs/neovim/tests/run_tests.sh
.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
GitHub Actions workflows must be properly configured in .github/workflows/ directory
Files:
.github/workflows/lua.yml
.github/workflows/*.yml
📄 CodeRabbit inference engine (.cursor/rules/github-workflows.mdc)
.github/workflows/*.yml: CI Pipeline must run on all pull requests and main branch pushes, validating Nix flake checks, home-manager builds, nix-darwin builds, and system configurations
Run Biome for code formatting and validate Nix expressions, commit message format, and documentation updates in code quality checks
Use specific action versions (not @main or @master) in GitHub Actions workflows
Cache Nix store and build artifacts in GitHub Actions workflows to improve performance
Set appropriate timeout limits in GitHub Actions workflow jobs
Use concise job and step names in GitHub Actions workflows and add helpful annotations and comments
Use GITHUB_TOKEN when possible, secure sensitive data in repository secrets, and limit permissions to minimum required in GitHub Actions workflows
Review third-party actions before use in GitHub Actions workflows
Set appropriate failure conditions, add helpful error messages, configure notifications for failures, and archive build artifacts for debugging in GitHub Actions workflows
Files:
.github/workflows/lua.yml
{.github/workflows/*.yml,renovate.json}
📄 CodeRabbit inference engine (.cursor/rules/github-workflows.mdc)
Use Renovate for automated dependency updates, configure update schedule in renovate.json, group related updates together, and auto-merge minor and patch updates
Files:
.github/workflows/lua.yml
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Test Nix and home-manager configurations locally before pushing using `make test`
📚 Learning: 2025-11-25T09:34:47.854Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/github-workflows.mdc:0-0
Timestamp: 2025-11-25T09:34:47.854Z
Learning: Applies to .github/workflows/*.yml : Run Biome for code formatting and validate Nix expressions, commit message format, and documentation updates in code quality checks
Applied to files:
.github/workflows/lua.yml
📚 Learning: 2025-11-25T09:34:47.854Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/github-workflows.mdc:0-0
Timestamp: 2025-11-25T09:34:47.854Z
Learning: Applies to .github/workflows/*.yml : CI Pipeline must run on all pull requests and main branch pushes, validating Nix flake checks, home-manager builds, nix-darwin builds, and system configurations
Applied to files:
.github/workflows/lua.yml
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Test Nix and home-manager configurations locally before pushing using `make test`
Applied to files:
Makefile
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Test configurations before committing using `nix flake check` and `home-manager build --show-trace`
Applied to files:
Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Mesa Review
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: nix-nixos
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
- GitHub Check: nix-linux
- GitHub Check: lua-neovim
- GitHub Check: nix-darwin
🔇 Additional comments (7)
home-manager/programs/neovim/tests/minimal_init.lua (1)
1-49: Well-designed test initialization.The minimal init file properly isolates the test environment from the full config. Key highlights:
- Path derivation using
debug.getinfois correct.- Plenary search across multiple locations with early exit is efficient.
- Disabling shada, swap, backup, and undo prevents test conflicts.
- Setting
updatetimeensures async operations complete in tests.home-manager/programs/neovim/tests/config_spec.lua (1)
1-63: LGTM! Well-structured test suite.The config tests properly verify:
- Module loading with cache clearing
- Leader key configuration
- Neovim API functionality
- Buffer manipulation with proper cleanup
Makefile (1)
494-503: LGTM! Test targets follow Makefile conventions.The new targets are properly documented and follow the existing pattern of providing both direct and dev-shell variants. The neovim-test-dev target correctly mirrors the CI environment.
.github/workflows/lua.yml (2)
25-36: LGTM! New test job follows workflow best practices.The lua-neovim-test job correctly:
- Uses specific action versions (not @main)
- Sets an appropriate 30-minute timeout
- Runs tests in the dev shell to mirror local development
- Uses concise, descriptive step names
49-54: Proper job dependency ordering.The lua-check job correctly includes lua-neovim-test in its needs array, ensuring test results are evaluated before the final status check.
home-manager/programs/neovim/tests/utils_spec.lua (1)
1-109: LGTM! Comprehensive utils test coverage.The test suite demonstrates excellent practices:
- Module cache clearing in
before_eachensures test isolation.- Proper cleanup of buffers and quickfix lists prevents resource leaks.
- Tests verify function existence, error handling, and behavior.
- Multi-buffer scenarios are tested with appropriate cleanup.
home-manager/programs/neovim/tests/run_tests.sh (1)
1-51: The script already passes shellcheck with no violations detected. No action required.
| if [ ! -d "$PLENARY_DIR" ]; then | ||
| echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}" | ||
| git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify git clone success before proceeding.
The script clones plenary.nvim but doesn't verify the clone succeeded. If the clone fails, subsequent test execution may fail with unclear errors.
Apply this diff:
if [ ! -d "$PLENARY_DIR" ]; then
echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}"
- git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR"
+ if ! git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR"; then
+ echo -e "${RED}Error: Failed to clone plenary.nvim${NC}"
+ exit 1
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -d "$PLENARY_DIR" ]; then | |
| echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}" | |
| git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR" | |
| fi | |
| if [ ! -d "$PLENARY_DIR" ]; then | |
| echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}" | |
| if ! git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR"; then | |
| echo -e "${RED}Error: Failed to clone plenary.nvim${NC}" | |
| exit 1 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In home-manager/programs/neovim/tests/run_tests.sh around lines 26 to 29, the
script clones plenary.nvim but does not check whether git clone succeeded;
update the block to verify the clone command returned success and if it failed
print a clear error (including the target dir and git error context) to stderr
and exit with a non-zero status so tests don't continue on a failed clone (e.g.,
run git clone and immediately check its exit code or test that the directory now
exists and on failure echo an error to >&2 and exit 1).
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 1fc1f60...74ba76b
Analysis
-
Dependency Management Violation: The test framework clones plenary.nvim from GitHub on every run instead of using Nix for dependency management. This creates network dependencies, lacks version pinning, and violates the repository's declarative philosophy.
-
Missing CI/CD Caching: No caching strategy is implemented for CI runs, leading to slow execution times, redundant downloads, and poor developer experience.
-
Race Conditions in Tests: The use of asynchronous
nvim_feedkeys()without proper synchronization introduces potential flakiness, especially in buffer tests where timing-dependent operations aren't guaranteed to complete. -
Path Resolution Fragility: The minimal_init.lua uses runtime path detection with debug.getinfo() which may break with different sourcing methods, symlinks, or other edge cases common in home-manager environments.
-
Inadequate Error Handling: The test scripts use
set -ebut lack proper error recovery mechanisms for partial failures, validation of successful operations, or cleanup of stale artifacts.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
6 files reviewed | 4 comments | Edit Agent Settings • Read Docs
| fi | ||
|
|
||
| # Ensure plenary.nvim is available | ||
| if [ ! -d "$PLENARY_DIR" ]; then |
There was a problem hiding this comment.
The plenary.nvim dependency is cloned imperatively without version pinning, creating several issues:
- Non-deterministic: Using
--depth 1without a version/tag means tests could break if plenary updates - Network dependency: Tests fail if GitHub is unreachable
- Inefficient: Re-clones on every CI run instead of caching
- Architectural inconsistency: This Nix-based dotfiles repo should manage dependencies declaratively
Recommendations:
- Pin to a specific tag:
git clone --depth 1 --branch v0.1.4 https://... - Add validation after clone to detect partial failures:
if [ ! -f "$PLENARY_DIR/lua/plenary/test_harness.lua" ]; then echo "Error: plenary.nvim clone incomplete" rm -rf "$PLENARY_DIR" exit 1 fi
- Consider adding plenary.nvim to the Nix devshell in
devenv.nixfor proper declarative dependency management
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#395
File: home-manager/programs/neovim/tests/run_tests.sh#L26
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The plenary.nvim dependency is cloned imperatively without version pinning, creating several issues:
1. **Non-deterministic**: Using `--depth 1` without a version/tag means tests could break if plenary updates
2. **Network dependency**: Tests fail if GitHub is unreachable
3. **Inefficient**: Re-clones on every CI run instead of caching
4. **Architectural inconsistency**: This Nix-based dotfiles repo should manage dependencies declaratively
Recommendations:
- Pin to a specific tag: `git clone --depth 1 --branch v0.1.4 https://...`
- Add validation after clone to detect partial failures:
```bash
if [ ! -f "$PLENARY_DIR/lua/plenary/test_harness.lua" ]; then
echo "Error: plenary.nvim clone incomplete"
rm -rf "$PLENARY_DIR"
exit 1
fi
- Consider adding plenary.nvim to the Nix devshell in
devenv.nixfor proper declarative dependency management
</details>
| vim.api.nvim_set_current_buf(buf) | ||
|
|
||
| -- Enter insert mode and type | ||
| vim.api.nvim_feedkeys("itest", "x", false) |
There was a problem hiding this comment.
Race condition: nvim_feedkeys() with the "x" flag processes keys asynchronously, but line 56 immediately checks the result without synchronization. This test will be flaky.
The feedkeys are queued but may not be processed before nvim_get_current_line() executes. Use synchronous buffer operations instead:
local buf = vim.api.nvim_create_buf(false, true)
vim.api.nvim_set_current_buf(buf)
vim.api.nvim_buf_set_lines(buf, 0, -1, false, {"test"})
local line = vim.api.nvim_buf_get_lines(buf, 0, 1, false)[1]
assert.equals("test", line)
vim.api.nvim_buf_delete(buf, { force = true })Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#395
File: home-manager/programs/neovim/tests/config_spec.lua#L53
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Race condition: `nvim_feedkeys()` with the `"x"` flag processes keys asynchronously, but line 56 immediately checks the result without synchronization. This test will be flaky.
The feedkeys are queued but may not be processed before `nvim_get_current_line()` executes. Use synchronous buffer operations instead:
```lua
local buf = vim.api.nvim_create_buf(false, true)
vim.api.nvim_set_current_buf(buf)
vim.api.nvim_buf_set_lines(buf, 0, -1, false, {"test"})
local line = vim.api.nvim_buf_get_lines(buf, 0, 1, false)[1]
assert.equals("test", line)
vim.api.nvim_buf_delete(buf, { force = true })
</details>
| vim.g.maplocalleader = " " | ||
|
|
||
| -- Get the directory of this init file | ||
| local init_path = debug.getinfo(1, "S").source:sub(2) |
There was a problem hiding this comment.
Using debug.getinfo(1, "S").source:sub(2) for path detection is fragile:
- May break if file is sourced differently (e.g., via
:source,-uflag variations) - Symlinks could cause issues (relevant since home-manager uses symlinks heavily)
- Hard to debug when it fails
Consider a more robust approach:
local tests_dir = vim.fn.expand("<sfile>:p:h")Or set an environment variable in run_tests.sh and use vim.env.TEST_DIR.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#395
File: home-manager/programs/neovim/tests/minimal_init.lua#L9
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Using `debug.getinfo(1, "S").source:sub(2)` for path detection is fragile:
- May break if file is sourced differently (e.g., via `:source`, `-u` flag variations)
- Symlinks could cause issues (relevant since home-manager uses symlinks heavily)
- Hard to debug when it fails
Consider a more robust approach:
```lua
local tests_dir = vim.fn.expand("<sfile>:p:h")
Or set an environment variable in run_tests.sh and use vim.env.TEST_DIR.
</details>
| { filename = "test.lua", lnum = 2, text = "Test item 2" }, | ||
| }, "r") | ||
|
|
||
| utils.copen() |
There was a problem hiding this comment.
Tests that interact with Neovim window APIs lack timeout protection. A hung operation could cause the test suite to wait until the CI timeout (30 minutes).
Consider adding timeout protection for tests that open windows or manipulate buffers. While plenary handles some timeouts internally, explicit guards for long-running operations would improve test reliability:
local timeout_ms = 5000
local start = vim.loop.now()
utils.copen()
assert.is_true(vim.loop.now() - start < timeout_ms, "Test operation timed out")Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#395
File: home-manager/programs/neovim/tests/utils_spec.lua#L68
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Tests that interact with Neovim window APIs lack timeout protection. A hung operation could cause the test suite to wait until the CI timeout (30 minutes).
Consider adding timeout protection for tests that open windows or manipulate buffers. While plenary handles some timeouts internally, explicit guards for long-running operations would improve test reliability:
```lua
local timeout_ms = 5000
local start = vim.loop.now()
utils.copen()
assert.is_true(vim.loop.now() - start < timeout_ms, "Test operation timed out")
</details>
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive testing infrastructure for the Neovim configuration using plenary.nvim as the test harness. It enables automated testing of Neovim Lua modules both locally and in CI/CD, ensuring code quality and preventing regressions. The testing setup is properly integrated with the existing Nix-based development workflow and GitHub Actions pipeline.
- Adds plenary.nvim-based test framework with isolated test environment
- Creates test coverage for utility functions and configuration loading
- Integrates testing into CI/CD pipeline with new GitHub Actions job
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| home-manager/programs/neovim/tests/utils_spec.lua | Implements unit tests for cycle_buffer, copen, and cclear utility functions |
| home-manager/programs/neovim/tests/config_spec.lua | Adds tests for config loading and Neovim API verification |
| home-manager/programs/neovim/tests/minimal_init.lua | Provides isolated test environment configuration with plenary setup |
| home-manager/programs/neovim/tests/run_tests.sh | Implements test runner script that handles plenary installation and test execution |
| Makefile | Adds neovim-test and neovim-test-dev targets for local and CI testing |
| .github/workflows/lua.yml | Adds lua-neovim-test job and integrates it into the existing Lua validation workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,109 @@ | |||
| -- Tests for lua/utils.lua | |||
| -- Run with: nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedDirectory tests/ { minimal_init = './tests/minimal_init.lua' }" | |||
There was a problem hiding this comment.
The comment suggests running tests with PlenaryBustedDirectory, but the actual test runner script uses plenary.test_harness.test_directory(). Consider updating the comment to reflect the actual command used in run_tests.sh or removing it to avoid confusion.
| -- Run with: nvim --headless -u tests/minimal_init.lua -c "PlenaryBustedDirectory tests/ { minimal_init = './tests/minimal_init.lua' }" | |
| -- Run with: nvim --headless -u tests/minimal_init.lua -c "lua require('plenary.test_harness').test_directory('tests', {minimal_init = './tests/minimal_init.lua'})" |
| echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}" | ||
| git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR" |
There was a problem hiding this comment.
The script clones plenary.nvim from GitHub without verifying the repository's authenticity or pinning to a specific commit/tag. Consider adding a version pin (e.g., --branch v0.1.4) to ensure reproducible test runs and avoid potential issues from upstream changes.
| echo -e "${YELLOW}Cloning plenary.nvim to $PLENARY_DIR...${NC}" | |
| git clone --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR" | |
| echo -e "${YELLOW}Cloning plenary.nvim (tag v0.1.4) to $PLENARY_DIR...${NC}" | |
| git clone --branch v0.1.4 --depth 1 https://github.com/nvim-lua/plenary.nvim "$PLENARY_DIR" |
|
|
||
| it("should handle multiple buffers", function() | ||
| -- Create a second buffer | ||
| local buf1 = vim.api.nvim_get_current_buf() |
There was a problem hiding this comment.
The variable buf1 is assigned but never used. Consider either using it for assertions or removing it to keep the code clean.
| local buf1 = vim.api.nvim_get_current_buf() |
| it("should open quickfix window when items exist", function() | ||
| -- Add items to quickfix list | ||
| vim.fn.setqflist({ | ||
| { filename = "test.lua", lnum = 1, text = "Test item 1" }, | ||
| { filename = "test.lua", lnum = 2, text = "Test item 2" }, | ||
| }, "r") | ||
|
|
||
| utils.copen() | ||
|
|
||
| -- Check if quickfix window is open | ||
| local qf_open = false | ||
| for _, win in ipairs(vim.api.nvim_list_wins()) do | ||
| local buf = vim.api.nvim_win_get_buf(win) | ||
| if vim.bo[buf].buftype == "quickfix" then | ||
| qf_open = true | ||
| break | ||
| end | ||
| end | ||
|
|
||
| assert.is_true(qf_open) | ||
|
|
||
| -- Clean up | ||
| vim.cmd("cclose") | ||
| vim.fn.setqflist({}, "r") | ||
| end) |
There was a problem hiding this comment.
The copen function checks if quickfix size is greater than 1, but the test creates a quickfix list with 2 items. This means the test correctly validates the copen behavior when size > 1. However, consider adding a test for the edge case where size == 1 to verify that cclose is called as expected by the implementation logic.
| fi | ||
|
|
||
| # Export plenary directory for minimal_init.lua | ||
| export PLENARY_DIR |
There was a problem hiding this comment.
The PLENARY_DIR environment variable is exported but not actually used by the minimal_init.lua script. The minimal_init.lua searches for plenary in hardcoded paths including /tmp/plenary.nvim, but doesn't check the PLENARY_DIR environment variable. Consider either removing this export or updating minimal_init.lua to use it via vim.fn.getenv("PLENARY_DIR") for better configurability.
| export PLENARY_DIR | |
| # export PLENARY_DIR |
| local plenary_paths = { | ||
| vim.fn.stdpath("data") .. "/site/pack/plugins/start/plenary.nvim", | ||
| vim.fn.stdpath("data") .. "/site/pack/plugins/opt/plenary.nvim", | ||
| vim.fn.expand("~/.local/share/nvim/site/pack/plugins/start/plenary.nvim"), | ||
| vim.fn.expand("~/.local/share/nvim/site/pack/plugins/opt/plenary.nvim"), | ||
| -- CI/test environment paths | ||
| "/tmp/plenary.nvim", | ||
| tests_dir .. "/plenary.nvim", | ||
| } |
There was a problem hiding this comment.
The plenary path list includes hardcoded paths like ~/.local/share/nvim/site/pack/plugins/start/plenary.nvim which duplicate the logic of vim.fn.stdpath("data") paths on lines 23-24. Consider simplifying by removing the hardcoded home directory paths since stdpath("data") already resolves to the appropriate location.
| @$(PWD)/home-manager/programs/neovim/tests/run_tests.sh | ||
| @echo "✅ Neovim tests completed" |
There was a problem hiding this comment.
The success message is printed even if the test script fails. Move the success message into the conditional check based on the exit code, or remove it since the script itself already prints success/failure messages.
| @$(PWD)/home-manager/programs/neovim/tests/run_tests.sh | |
| @echo "✅ Neovim tests completed" | |
| @$(PWD)/home-manager/programs/neovim/tests/run_tests.sh && echo "✅ Neovim tests completed" |
| it("should handle multiple buffers", function() | ||
| -- Create a second buffer | ||
| local buf1 = vim.api.nvim_get_current_buf() | ||
| vim.cmd("enew") | ||
| local buf2 = vim.api.nvim_get_current_buf() | ||
|
|
||
| -- Should be able to cycle between them | ||
| assert.has_no.errors(function() | ||
| utils.cycle_buffer("next") | ||
| end) | ||
|
|
||
| -- Clean up | ||
| vim.api.nvim_buf_delete(buf2, { force = true }) | ||
| end) |
There was a problem hiding this comment.
The test creates two buffers but doesn't verify that cycle_buffer actually switches between them. Consider adding assertions to check that the current buffer changes after calling cycle_buffer("next"), for example by comparing vim.api.nvim_get_current_buf() before and after the call.
| "/tmp/plenary.nvim", | ||
| tests_dir .. "/plenary.nvim", | ||
| } | ||
|
|
||
| for _, path in ipairs(plenary_paths) do | ||
| if vim.fn.isdirectory(path) == 1 then | ||
| vim.opt.runtimepath:append(path) | ||
| break | ||
| end |
There was a problem hiding this comment.
This test setup adds /tmp/plenary.nvim to runtimepath and will execute Lua code from there if the directory exists, which allows arbitrary code execution from a world-writable location. A local attacker can pre-create /tmp/plenary.nvim with a malicious plenary plugin so that when run_tests.sh invokes Neovim and require('plenary.test_harness'), their code runs under the test user's privileges. To fix this, avoid loading plugins from /tmp (e.g., use a repo-local tests/plenary.nvim or a per-user stdpath('data') directory) and ensure any test-only plugin directory is created in a location not writable by other users.
Tests can be run locally with
make neovim-testor via Nix dev shellwith
make neovim-test-dev. CI runs tests on every push/PR.Summary by cubic
Adds a plenary.nvim-based test suite for the Neovim config and hooks it into CI to run on every push/PR. Provides simple Makefile targets to run tests locally or in the Nix dev shell.
New Features
Migration
Written for commit 74ba76b. Summary will update automatically on new commits.