Nvim test - #398
Conversation
… settings and keymaps
…itter, UI, window, and workspace functionalities
…mprehensive AI, completion, plugins, and telescope tests
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a public Makefile Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 significantly enhances the testing infrastructure for the Neovim configuration. It introduces a dedicated Highlights
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;DRAdds a comprehensive Neovim test runner and a new test suite to verify core APIs, editor settings, keymaps, and plugin configurations, making tests easily runnable via 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 comprehensive test suite for the Neovim configuration, which is a great step towards ensuring its stability and maintainability. The changes are well-structured, with tests organized by feature. My review focuses on improving the test code itself by reducing duplication and leveraging testing framework features to make the tests more concise and easier to maintain. I've noticed two common patterns that could be improved:
- Many test suites have repetitive setup and teardown code (like creating and deleting buffers) within each
itblock. This can be extracted intobefore_eachandafter_eachhooks to reduce boilerplate. - Some test suites contain multiple
itblocks that perform very similar checks. These can often be consolidated into a single, data-driven test using a loop, making the test suite more compact.
I've left specific suggestions on several of the new test files to illustrate these points. Applying these patterns across the new test files will significantly improve the test suite's quality.
| describe("content manipulation", function() | ||
| it("should set and get buffer lines", function() | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| local lines = { "line 1", "line 2", "line 3" } | ||
| vim.api.nvim_buf_set_lines(buf, 0, -1, false, lines) | ||
|
|
||
| local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false) | ||
| assert.are.same(lines, result) | ||
|
|
||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
|
|
||
| it("should append lines to buffer", function() | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "first" }) | ||
| vim.api.nvim_buf_set_lines(buf, -1, -1, false, { "second" }) | ||
|
|
||
| local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false) | ||
| assert.are.same({ "first", "second" }, result) | ||
|
|
||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
|
|
||
| it("should replace specific lines", function() | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "a", "b", "c" }) | ||
| vim.api.nvim_buf_set_lines(buf, 1, 2, false, { "replaced" }) | ||
|
|
||
| local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false) | ||
| assert.are.same({ "a", "replaced", "c" }, result) | ||
|
|
||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
There's a lot of repeated buffer creation and deletion in these tests. You can make the test suite cleaner and more maintainable by using before_each and after_each hooks to handle this setup and teardown logic. This avoids duplicating the same lines in every it block.
describe("content manipulation", function()
local buf
before_each(function()
buf = vim.api.nvim_create_buf(false, true)
end)
after_each(function()
if buf and vim.api.nvim_buf_is_valid(buf) then
vim.api.nvim_buf_delete(buf, { force = true })
end
end)
it("should set and get buffer lines", function()
local lines = { "line 1", "line 2", "line 3" }
vim.api.nvim_buf_set_lines(buf, 0, -1, false, lines)
local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false)
assert.are.same(lines, result)
end)
it("should append lines to buffer", function()
vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "first" })
vim.api.nvim_buf_set_lines(buf, -1, -1, false, { "second" })
local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false)
assert.are.same({ "first", "second" }, result)
end)
it("should replace specific lines", function()
vim.api.nvim_buf_set_lines(buf, 0, -1, false, { "a", "b", "c" })
vim.api.nvim_buf_set_lines(buf, 1, 2, false, { "replaced" })
local result = vim.api.nvim_buf_get_lines(buf, 0, -1, false)
assert.are.same({ "a", "replaced", "c" }, result)
end)
end)
| describe("augroup creation", function() | ||
| local function augroup_exists(name) | ||
| local ok, id = pcall(vim.api.nvim_get_autocmds, { group = name }) | ||
| return ok and id ~= nil | ||
| end | ||
|
|
||
| it("should create HighlightYank augroup", function() | ||
| assert.is_true(augroup_exists("HighlightYank")) | ||
| end) | ||
|
|
||
| it("should create ResizeSplits augroup", function() | ||
| assert.is_true(augroup_exists("ResizeSplits")) | ||
| end) | ||
|
|
||
| it("should create CheckTime augroup", function() | ||
| assert.is_true(augroup_exists("CheckTime")) | ||
| end) | ||
|
|
||
| it("should create GitCommit augroup", function() | ||
| assert.is_true(augroup_exists("GitCommit")) | ||
| end) | ||
|
|
||
| it("should create NewFile augroup", function() | ||
| assert.is_true(augroup_exists("NewFile")) | ||
| end) | ||
|
|
||
| it("should create Help augroup", function() | ||
| assert.is_true(augroup_exists("Help")) | ||
| end) | ||
|
|
||
| it("should create Git augroup", function() | ||
| assert.is_true(augroup_exists("Git")) | ||
| end) | ||
|
|
||
| it("should create Fugitive augroup", function() | ||
| assert.is_true(augroup_exists("Fugitive")) | ||
| end) | ||
|
|
||
| it("should create QuickfixHelp augroup", function() | ||
| assert.is_true(augroup_exists("QuickfixHelp")) | ||
| end) | ||
|
|
||
| it("should create Markdown augroup", function() | ||
| assert.is_true(augroup_exists("Markdown")) | ||
| end) | ||
|
|
||
| it("should create Terminal augroup", function() | ||
| assert.is_true(augroup_exists("Terminal")) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
This block of tests is quite repetitive. You can make it more concise and easier to extend by consolidating these checks into a single data-driven test that iterates over a list of augroup names.
describe("augroup creation", function()
local function augroup_exists(name)
local ok, id = pcall(vim.api.nvim_get_autocmds, { group = name })
return ok and id ~= nil
end)
it("should create all expected augroups", function()
local expected_augroups = {
"HighlightYank",
"ResizeSplits",
"CheckTime",
"GitCommit",
"NewFile",
"Help",
"Git",
"Fugitive",
"QuickfixHelp",
"Markdown",
"Terminal",
}
for _, name in ipairs(expected_augroups) do
assert.is_true(augroup_exists(name), "Expected augroup '" .. name .. "' to exist")
end
end)
end)
| describe("vim.lsp API", function() | ||
| it("should have vim.lsp.buf available", function() | ||
| assert.is_table(vim.lsp.buf) | ||
| end) | ||
|
|
||
| it("should have hover function", function() | ||
| assert.is_function(vim.lsp.buf.hover) | ||
| end) | ||
|
|
||
| it("should have definition function", function() | ||
| assert.is_function(vim.lsp.buf.definition) | ||
| end) | ||
|
|
||
| it("should have declaration function", function() | ||
| assert.is_function(vim.lsp.buf.declaration) | ||
| end) | ||
|
|
||
| it("should have implementation function", function() | ||
| assert.is_function(vim.lsp.buf.implementation) | ||
| end) | ||
|
|
||
| it("should have references function", function() | ||
| assert.is_function(vim.lsp.buf.references) | ||
| end) | ||
|
|
||
| it("should have type_definition function", function() | ||
| assert.is_function(vim.lsp.buf.type_definition) | ||
| end) | ||
|
|
||
| it("should have code_action function", function() | ||
| assert.is_function(vim.lsp.buf.code_action) | ||
| end) | ||
|
|
||
| it("should have rename function", function() | ||
| assert.is_function(vim.lsp.buf.rename) | ||
| end) | ||
|
|
||
| it("should have signature_help function", function() | ||
| assert.is_function(vim.lsp.buf.signature_help) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
These tests for the vim.lsp.buf API are very repetitive. To make the test suite more concise and maintainable, you can consolidate them into a single data-driven test that iterates through a list of function names to check for their existence.
describe("vim.lsp API", function()
it("should have vim.lsp.buf available", function()
assert.is_table(vim.lsp.buf)
end)
it("should have standard LSP buffer functions", function()
local functions = {
"hover",
"definition",
"declaration",
"implementation",
"references",
"type_definition",
"code_action",
"rename",
"signature_help",
}
for _, func_name in ipairs(functions) do
assert.is_function(vim.lsp.buf[func_name], "vim.lsp.buf." .. func_name .. " should be a function")
end
end)
end)
| describe("indentation", function() | ||
| it("should use spaces instead of tabs", function() | ||
| assert.is_true(vim.opt.expandtab:get()) | ||
| end) | ||
|
|
||
| it("should enable smart indentation", function() | ||
| assert.is_true(vim.opt.smartindent:get()) | ||
| end) | ||
|
|
||
| it("should set shiftwidth to 2", function() | ||
| assert.equals(2, vim.opt.shiftwidth:get()) | ||
| end) | ||
|
|
||
| it("should set softtabstop to 2", function() | ||
| assert.equals(2, vim.opt.softtabstop:get()) | ||
| end) | ||
|
|
||
| it("should set tabstop to 2", function() | ||
| assert.equals(2, vim.opt.tabstop:get()) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
The tests for indentation settings can be grouped into a single, more descriptive test case. This makes the intent clearer (testing the overall indentation setup) and reduces the number of individual tests, making the suite more concise.
describe("indentation", function()
it("should be configured for 2-space soft tabs", function()
assert.is_true(vim.opt.expandtab:get(), "should use spaces instead of tabs")
assert.is_true(vim.opt.smartindent:get(), "should enable smart indentation")
assert.equals(2, vim.opt.shiftwidth:get(), "shiftwidth should be 2")
assert.equals(2, vim.opt.softtabstop:get(), "softtabstop should be 2")
assert.equals(2, vim.opt.tabstop:get(), "tabstop should be 2")
end)
end)
| describe("indentation autocmd", function() | ||
| it("should set shiftwidth to 2 for new buffers", function() | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| vim.api.nvim_set_current_buf(buf) | ||
| vim.bo[buf].filetype = "lua" | ||
|
|
||
| vim.api.nvim_exec_autocmds("FileType", { pattern = "lua" }) | ||
|
|
||
| assert.equals(2, vim.opt_local.shiftwidth:get()) | ||
|
|
||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
|
|
||
| it("should set tabstop to 2 for new buffers", function() | ||
| local buf = vim.api.nvim_create_buf(false, true) | ||
| vim.api.nvim_set_current_buf(buf) | ||
| vim.bo[buf].filetype = "javascript" | ||
|
|
||
| vim.api.nvim_exec_autocmds("FileType", { pattern = "javascript" }) | ||
|
|
||
| assert.equals(2, vim.opt_local.tabstop:get()) | ||
|
|
||
| vim.api.nvim_buf_delete(buf, { force = true }) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
This test block contains duplicated buffer creation and deletion logic. By using before_each and after_each hooks, you can centralize this setup and teardown, making the tests cleaner and less repetitive.
describe("indentation autocmd", function()
local buf
before_each(function()
buf = vim.api.nvim_create_buf(false, true)
vim.api.nvim_set_current_buf(buf)
end)
after_each(function()
if buf and vim.api.nvim_buf_is_valid(buf) then
vim.api.nvim_buf_delete(buf, { force = true })
end
end)
it("should set shiftwidth to 2 for new buffers", function()
vim.bo[buf].filetype = "lua"
vim.api.nvim_exec_autocmds("FileType", { pattern = "lua" })
assert.equals(2, vim.opt_local.shiftwidth:get())
end)
it("should set tabstop to 2 for new buffers", function()
vim.bo[buf].filetype = "javascript"
vim.api.nvim_exec_autocmds("FileType", { pattern = "javascript" })
assert.equals(2, vim.opt_local.tabstop:get())
end)
end)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| describe("colorscheme", function() | ||
| it("should have dracula colorscheme available", function() | ||
| local colors = vim.fn.getcompletion("dracula", "color") | ||
| assert.is_true(#colors > 0 or vim.g.colors_name == "dracula") |
There was a problem hiding this comment.
The new UI spec asserts that the dracula colorscheme is available (vim.fn.getcompletion("dracula", "color") must be non-empty or vim.g.colors_name already set), but the bundled test runner only bootstraps plenary and launches Neovim with the minimal init (run_tests.sh lines 25‑41). In a clean checkout running make neovim-test, no colorscheme plugins are installed or loaded, so the assertion at lines 27‑29 fails every time. Either load lua/ui.lua/install the colorscheme in the test harness or relax the expectation so the suite can pass on a fresh environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for Neovim configuration using plenary.nvim as the testing framework. The tests cover various aspects of the Neovim setup including LSP, treesitter, completion, keymaps, autocmds, UI settings, and core API functionality.
Key changes:
- Moves
run_tests.shfromtests/to the neovim config root directory and updates related paths - Adds 15 new test specification files covering different aspects of Neovim configuration
- Updates
minimal_init.luato support environment variable for plenary directory - Adds a
testtarget to the Makefile for easier test execution
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds test target and updates neovim-test path to reflect script relocation |
home-manager/programs/neovim/run_tests.sh |
Updates NVIM_DIR path logic for new script location |
home-manager/programs/neovim/tests/minimal_init.lua |
Adds PLENARY_DIR environment variable support for flexible plenary.nvim location |
home-manager/programs/neovim/tests/workspace_spec.lua |
Adds tests for workspace-specific settings and commands |
home-manager/programs/neovim/tests/ui_spec.lua |
Adds tests for UI configurations including notify, colorscheme, and statusline |
home-manager/programs/neovim/tests/treesitter_spec.lua |
Adds tests for treesitter API and configuration |
home-manager/programs/neovim/tests/terminal_spec.lua |
Adds tests for terminal functionality and API basics |
home-manager/programs/neovim/tests/telescope_spec.lua |
Adds tests for telescope-related APIs and configuration patterns |
home-manager/programs/neovim/tests/settings_spec.lua |
Adds comprehensive tests for vim settings and options |
home-manager/programs/neovim/tests/plugins_spec.lua |
Adds tests for plugin management patterns and setup |
home-manager/programs/neovim/tests/lsp_spec.lua |
Adds tests for LSP configuration and diagnostics API |
home-manager/programs/neovim/tests/keymaps_spec.lua |
Adds tests for keymap functionality and vim.keymap API |
home-manager/programs/neovim/tests/completion_spec.lua |
Adds tests for completion-related APIs |
home-manager/programs/neovim/tests/autocmds_spec.lua |
Adds tests for autocommands configuration |
home-manager/programs/neovim/tests/api/window_spec.lua |
Adds tests for Neovim window API manipulation |
home-manager/programs/neovim/tests/api/init_spec.lua |
Updates test suite name and removes verbose comments |
home-manager/programs/neovim/tests/api/buffer_spec.lua |
Adds tests for Neovim buffer API operations |
home-manager/programs/neovim/tests/ai_spec.lua |
Adds placeholder tests for AI/sidekick integration |
Comments suppressed due to low confidence (2)
home-manager/programs/neovim/tests/api/init_spec.lua:2
- The description has been updated to "Tests for Neovim init/core API" which is more accurate than the old "Tests for Neovim configuration loading" since the test suite has been renamed from "config" to "init".
home-manager/programs/neovim/tests/api/init_spec.lua:54 - The removal of inline comments (like "-- Create a new buffer", "-- Set lines", "-- Clean up") makes the code less readable, especially for complex test scenarios. Consider keeping helpful comments that explain non-obvious test steps.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| it("should have get or get_query function", function() | ||
| -- API changed in newer Neovim versions | ||
| local has_get = vim.treesitter.query.get ~= nil or vim.treesitter.query.get_query ~= nil |
There was a problem hiding this comment.
The test checks for the existence of either get or get_query but doesn't verify that at least one of them is actually a function. Consider: assert.is_true((vim.treesitter.query.get and type(vim.treesitter.query.get) == "function") or (vim.treesitter.query.get_query and type(vim.treesitter.query.get_query) == "function"))
| local has_get = vim.treesitter.query.get ~= nil or vim.treesitter.query.get_query ~= nil | |
| local has_get = (vim.treesitter.query.get and type(vim.treesitter.query.get) == "function") or (vim.treesitter.query.get_query and type(vim.treesitter.query.get_query) == "function") |
| describe("getcwd", function() | ||
| it("should return current working directory", function() | ||
| local cwd = vim.fn.getcwd() | ||
| assert.is_string(cwd) | ||
| assert.is_true(#cwd > 0) | ||
| end) |
There was a problem hiding this comment.
The test "should return current working directory" only verifies that vim.fn.getcwd() returns a non-empty string, but it doesn't actually test any workspace.lua functionality. Consider either removing this test or modifying it to test actual workspace-related behavior if the workspace module provides any getcwd wrapper or related functionality.
| it("should support leader keymaps for pickers", function() | ||
| local leader_maps = { | ||
| "<leader>of", | ||
| "<leader>lg", | ||
| "<leader>fb", | ||
| "<leader>fh", | ||
| "<leader>fc", | ||
| "<leader>fr", | ||
| "<leader>fq", | ||
| "<leader>/", | ||
| } | ||
| for _, lhs in ipairs(leader_maps) do | ||
| vim.keymap.set("n", lhs, function() end, { noremap = true }) | ||
| local keymap = vim.fn.maparg(lhs, "n") | ||
| assert.is_true(keymap ~= "", "Failed for " .. lhs) | ||
| vim.keymap.del("n", lhs) | ||
| end | ||
| end) |
There was a problem hiding this comment.
[nitpick] The test name "should support leader keymaps for pickers" is somewhat generic. Consider being more specific about what's being tested, e.g., "should allow setting and verifying leader-based picker keymaps".
|
|
||
| it("should set undo directory", function() | ||
| local undodir = vim.opt.undodir:get() | ||
| assert.is_true(type(undodir) == "string" or type(undodir) == "table") |
There was a problem hiding this comment.
The type check allows both string and table types for undodir, but doesn't verify that a table contains valid values or that a string is non-empty. Consider adding more specific assertions to ensure the undodir is properly configured.
| assert.is_true(type(undodir) == "string" or type(undodir) == "table") | |
| if type(undodir) == "string" then | |
| assert.is_true(undodir ~= "") | |
| elseif type(undodir) == "table" then | |
| local has_nonempty = false | |
| for _, v in ipairs(undodir) do | |
| if type(v) == "string" and v ~= "" then | |
| has_nonempty = true | |
| break | |
| end | |
| end | |
| assert.is_true(has_nonempty) | |
| else | |
| assert.is_true(false, "undodir must be string or table") | |
| end |
| it("should set leader key to space", function() | ||
| assert.equals(" ", vim.g.mapleader) | ||
| end) |
There was a problem hiding this comment.
[nitpick] The test assumes the leader key is already set to space. Consider setting it explicitly in the test or in a before_each hook to ensure test isolation and avoid dependency on external configuration state.
| it("should support menu option", function() | ||
| vim.opt.completeopt:append("menu") | ||
| local completeopt = vim.opt.completeopt:get() | ||
| assert.is_true(vim.tbl_contains(completeopt, "menu")) | ||
| end) | ||
|
|
||
| it("should support menuone option", function() | ||
| vim.opt.completeopt:append("menuone") | ||
| local completeopt = vim.opt.completeopt:get() | ||
| assert.is_true(vim.tbl_contains(completeopt, "menuone")) | ||
| end) | ||
|
|
||
| it("should support noselect option", function() | ||
| vim.opt.completeopt:append("noselect") | ||
| local completeopt = vim.opt.completeopt:get() | ||
| assert.is_true(vim.tbl_contains(completeopt, "noselect")) | ||
| end) |
There was a problem hiding this comment.
The test modifies the global completeopt setting by appending "menu", which could affect subsequent tests. Consider using a before_each/after_each pattern to save and restore the original value, or create a fresh test buffer with local options.
| it("should have sidekick module structure expected", function() | ||
| -- In full config, sidekick would be loaded | ||
| -- Here we just verify the expected API pattern | ||
| assert.is_true(true) | ||
| end) |
There was a problem hiding this comment.
[nitpick] The test "should have sidekick module structure expected" has a placeholder comment but only asserts true without testing anything meaningful. Consider either implementing a real test for the sidekick API structure, or removing this placeholder test.
|
|
||
| it("should enable mouse support for all modes", function() | ||
| local mouse = vim.opt.mouse:get() | ||
| assert.is_true(mouse.a == true or mouse == "a") |
There was a problem hiding this comment.
The condition mouse.a == true or mouse == "a" is checking if either the table has an 'a' key set to true or if mouse equals the string "a". However, in Neovim, vim.opt.mouse:get() typically returns a string (e.g., "a"), not a table with boolean values. The first condition mouse.a == true would error if mouse is a string. Consider simplifying to just check: assert.equals("a", mouse) or handle the type check more carefully.
| assert.is_true(mouse.a == true or mouse == "a") | |
| assert.equals("a", mouse) |
| describe("plugin setup pattern", function() | ||
| it("should support setup with empty table", function() | ||
| -- Common pattern: require("plugin").setup({}) | ||
| -- We test the pattern works | ||
| local mock_setup_called = false | ||
| local mock_plugin = { | ||
| setup = function(opts) | ||
| mock_setup_called = true | ||
| assert.is_table(opts) | ||
| end, | ||
| } | ||
| mock_plugin.setup({}) | ||
| assert.is_true(mock_setup_called) | ||
| end) | ||
|
|
||
| it("should support setup with options", function() | ||
| local received_opts = nil | ||
| local mock_plugin = { | ||
| setup = function(opts) | ||
| received_opts = opts | ||
| end, | ||
| } | ||
| mock_plugin.setup({ enabled = true, timeout = 500 }) | ||
| assert.equals(true, received_opts.enabled) | ||
| assert.equals(500, received_opts.timeout) | ||
| end) | ||
| end) |
There was a problem hiding this comment.
[nitpick] The test "plugin setup pattern" doesn't actually test any real plugin functionality from the plugins.lua file. It only tests mock objects. Consider either testing actual plugin setup behavior or renaming to indicate it's testing the pattern/API, not the plugins.lua module.
| describe("augroup creation", function() | ||
| local function augroup_exists(name) | ||
| local ok, id = pcall(vim.api.nvim_get_autocmds, { group = name }) | ||
| return ok and id ~= nil |
There was a problem hiding this comment.
The augroup_exists function uses pcall to catch errors, but it checks id ~= nil which is always true when pcall returns ok=true. The function should check if the returned autocmds table has elements or use a different approach to verify the group exists. Consider: return ok and #id > 0 or check vim.api.nvim_create_augroup to get the group ID.
| return ok and id ~= nil | |
| return ok and #id > 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
home-manager/programs/neovim/tests/minimal_init.lua (1)
21-32: NilPLENARY_DIRbreaks fallback plenary path discoveryWith
local plenary_paths = { plenary_dir, ... }, ifPLENARY_DIRis unset (nil) thenplenary_paths[1]is nil andipairs(plenary_paths)stops immediately. This prevents any of the fallback paths from being checked, so plenary may never be added toruntimepathunless the env var is set.Guard the env var and insert it only when non‑empty, keeping the original fallbacks intact:
-local plenary_dir = os.getenv("PLENARY_DIR") -local plenary_paths = { - plenary_dir, -- Environment variable takes precedence - 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", -} +local plenary_dir = os.getenv("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", +} + +if plenary_dir ~= nil and plenary_dir ~= "" then + table.insert(plenary_paths, 1, plenary_dir) -- Environment variable takes precedence +end
🧹 Nitpick comments (6)
home-manager/programs/neovim/tests/completion_spec.lua (1)
19-42: Consider resetting completeopt between tests.The tests append values to
completeoptwithout resetting it, which could cause state pollution if tests run in different orders or if new tests are added. Consider storing the original value and restoring it after each test.Apply this pattern in a setup/teardown:
describe("completeopt", function() + local original_completeopt + + before_each(function() + original_completeopt = vim.opt.completeopt:get() + end) + + after_each(function() + vim.opt.completeopt = original_completeopt + end) + it("should be configurable", function() local completeopt = vim.opt.completeopt:get() assert.is_table(completeopt) end)home-manager/programs/neovim/tests/treesitter_spec.lua (1)
1-62: Treesitter API presence tests are fine; keymap assertion is intentionally looseThe Treesitter API/query checks look good and version‑tolerant. For the incremental selection keymap you currently only assert
maparg()returns a string, which is always true; if you ever want to enforce that<C-space>is actually mapped, you could tighten that tokeymap ~= ""or mark the test pending when not present.home-manager/programs/neovim/tests/ai_spec.lua (1)
1-22: AI/sidekick tests: keymap spec is useful; sidekick spec is just a stubThe keymap pattern test is a good minimal check and cleans up after itself. The “sidekick API” example is currently a no‑op stub; if that’s intentional scaffolding, you may want to either mark it pending or flesh it out once the sidekick module is loadable in the minimal env.
home-manager/programs/neovim/tests/ui_spec.lua (1)
1-60: UI tests are good; consider how hard you want to depend on DraculaThe
vim.notify,vim.ui, andstatuslinechecks are straightforward. The Dracula test (getcompletion("dracula", "color")orcolors_name == "dracula") will fail if that colorscheme isn’t installed or loaded in the minimal test env; if you ever run these tests without Dracula present, you may want to soften this to “any colorscheme works” or gate/mark it pending when not available.home-manager/programs/neovim/tests/settings_spec.lua (1)
112-120: Consider strengthening directory path assertions.The current tests only verify that
backupdiris non-empty andundodiris the correct type, but don't validate the actual directory paths. If the expected paths are predictable and consistent across test environments, consider asserting the specific values.Example:
it("should set backup directory", function() local backupdir = vim.opt.backupdir:get() - assert.is_true(#backupdir > 0) + -- More specific assertion if path is known: + assert.is_true(vim.tbl_contains(backupdir, vim.fn.expand("~/.local/state/nvim/backup//"))) end)home-manager/programs/neovim/tests/telescope_spec.lua (1)
1-3: Consider tracking integration tests as future work.While the current pattern validation tests are useful, they don't exercise the actual telescope module or verify that your telescope configuration loads correctly. When feasible, consider adding integration tests that load telescope in a fuller test environment to catch real configuration issues.
Would you like me to open an issue to track adding telescope integration tests in the future?
📜 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 (18)
Makefile(2 hunks)home-manager/programs/neovim/run_tests.sh(1 hunks)home-manager/programs/neovim/tests/ai_spec.lua(1 hunks)home-manager/programs/neovim/tests/api/buffer_spec.lua(1 hunks)home-manager/programs/neovim/tests/api/init_spec.lua(1 hunks)home-manager/programs/neovim/tests/api/window_spec.lua(1 hunks)home-manager/programs/neovim/tests/autocmds_spec.lua(1 hunks)home-manager/programs/neovim/tests/completion_spec.lua(1 hunks)home-manager/programs/neovim/tests/keymaps_spec.lua(1 hunks)home-manager/programs/neovim/tests/lsp_spec.lua(1 hunks)home-manager/programs/neovim/tests/minimal_init.lua(1 hunks)home-manager/programs/neovim/tests/plugins_spec.lua(1 hunks)home-manager/programs/neovim/tests/settings_spec.lua(1 hunks)home-manager/programs/neovim/tests/telescope_spec.lua(1 hunks)home-manager/programs/neovim/tests/terminal_spec.lua(1 hunks)home-manager/programs/neovim/tests/treesitter_spec.lua(1 hunks)home-manager/programs/neovim/tests/ui_spec.lua(1 hunks)home-manager/programs/neovim/tests/workspace_spec.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/run_tests.sh
🧠 Learnings (1)
📚 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
⏰ 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: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: nix-linux
- GitHub Check: nix-darwin
- GitHub Check: lua-neovim
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: nix-nixos
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
🔇 Additional comments (17)
home-manager/programs/neovim/tests/terminal_spec.lua (1)
1-46: LGTM! Well-structured terminal API tests.The test suite properly covers terminal functionality basics and window options. All tests include appropriate resource cleanup, preventing state pollution between test runs.
home-manager/programs/neovim/tests/api/buffer_spec.lua (1)
1-129: LGTM! Comprehensive buffer API test coverage.The test suite thoroughly covers buffer operations including creation, deletion, content manipulation, options, listing, and naming. All tests properly clean up resources.
home-manager/programs/neovim/tests/plugins_spec.lua (1)
1-75: LGTM! Appropriate plugin pattern testing.The test suite validates plugin management patterns and APIs using mocks, which is suitable for a minimal test environment where actual plugins aren't loaded.
home-manager/programs/neovim/tests/autocmds_spec.lua (2)
10-163: LGTM! Thorough autocmd testing.The test suite comprehensively covers augroup creation, event registration, API usage, and filetype-specific settings. All tests properly manage buffer lifecycle and clean up test augroups.
5-8: No action needed. Theautocmdsmodule exists athome-manager/programs/neovim/lua/autocmds.luaand properly initializes all expected augroups (HighlightYank, ResizeSplits, CheckTime, GitCommit, NewFile, Help, Git, Fugitive, QuickfixHelp, Markdown, and Terminal). The test's before_each hook correctly reloads the module for each test.home-manager/programs/neovim/tests/completion_spec.lua (1)
44-82: LGTM! Good cleanup practices for keymaps.The snippet expansion and insert mode mapping tests properly clean up created keymaps, preventing test pollution.
home-manager/programs/neovim/tests/api/window_spec.lua (1)
1-142: LGTM! Comprehensive window API test coverage.The test suite thoroughly validates window operations with proper resource cleanup throughout. Tests cover all major window API features including creation, deletion, buffer management, options, dimensions, and cursor positioning.
home-manager/programs/neovim/run_tests.sh (2)
9-51: LGTM! Well-structured test runner script.The script follows shell best practices with proper error handling, availability checks, clear output, and correct exit code propagation. Indentation uses 2 spaces as per coding guidelines.
7-8: The test structure is correctly aligned with the NVIM_DIR path. Thetests/directory exists athome-manager/programs/neovim/tests/withminimal_init.luaand all 16 test spec files (utils_spec.lua, ai_spec.lua, api/*.lua, autocmds_spec.lua, etc.) in place. SinceNVIM_DIRpoints to the script's own directory and the script changes to that directory on line 35, the relative paths totests/minimal_init.luaandtests/on lines 40-41 will resolve correctly. No action required.Likely an incorrect or invalid review comment.
Makefile (2)
140-141: LGTM! Public test target improves discoverability.The new
testtarget provides a clear entry point for running tests, aligning with the learning to "test Nix and home-manager configurations locally before pushing usingmake test."
498-501: LGTM! Test runner path correctly updated.The path update to
home-manager/programs/neovim/run_tests.shaligns with the test infrastructure reorganization and matches the run_tests.sh location in this PR.home-manager/programs/neovim/tests/api/init_spec.lua (1)
1-56: Init/core API tests look solid and self‑containedThe specs exercise core APIs (settings load, leader key, buffer/window validity, buffer ops, feedkeys) without leaving state behind; no issues from a correctness or flakiness standpoint.
home-manager/programs/neovim/tests/lsp_spec.lua (1)
1-104: LSP and diagnostic API surface tests look appropriateThis suite cleanly validates the expected vim.lsp and vim.diagnostic functions and severity constants without side effects (no server startup). Nothing concerning here.
home-manager/programs/neovim/tests/workspace_spec.lua (1)
1-56: Workspace command and autocmd tests are well‑structuredReloading
workspaceper test, explicitly creating buffers, triggeringFileTypeautocmds, and cleaning up buffers makes these specs clear and low‑risk.home-manager/programs/neovim/tests/keymaps_spec.lua (1)
1-139: Comprehensive keymap API coverage with proper cleanupThis spec gives nice coverage of keymap behaviors (options, buffer-local, callbacks, multi-mode, and execution) and consistently deletes mappings/buffers afterward. No changes needed.
home-manager/programs/neovim/tests/settings_spec.lua (1)
1-170: LGTM! Comprehensive settings test coverage.The test suite is well-structured with logical groupings, proper module reloading for isolation, and thorough coverage of Neovim settings. The assertions correctly validate expected option values using the vim.opt API.
home-manager/programs/neovim/tests/telescope_spec.lua (1)
1-99: LGTM! Pattern validation tests for telescope configuration.The test suite appropriately validates telescope configuration patterns and keymap capabilities without loading the actual telescope plugin. The "pattern" naming in describe blocks clearly signals this scope, and the comment on line 2 explains the constraint. These smoke tests ensure configuration structures are syntactically valid.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 18 files
Prompt for AI agents (all 5 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/programs/neovim/tests/ai_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/ai_spec.lua:9">
P2: Trivial assertion `assert.is_true(true)` always passes and provides no test coverage. Consider either implementing a meaningful test (e.g., checking module structure exists) or marking it as pending/skipped with a clear TODO.</violation>
</file>
<file name="home-manager/programs/neovim/tests/treesitter_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/treesitter_spec.lua:56">
P2: Test assertion doesn't verify what the test name claims. `vim.fn.maparg()` returns an empty string when no mapping exists, so `assert.is_string(keymap)` will always pass. Consider either:
1. Rename the test to reflect what it actually tests (e.g., 'should not error when checking C-space keymap')
2. Or add a meaningful assertion like `assert.is_truthy(keymap ~= "")` if the mapping should exist</violation>
</file>
<file name="home-manager/programs/neovim/tests/completion_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/completion_spec.lua:19">
P2: Tests modify `vim.opt.completeopt` but don't restore the original value, causing test pollution. Consider saving and restoring the original value, or add a `before_each`/`after_each` hook to reset state between tests.</violation>
</file>
<file name="home-manager/programs/neovim/tests/workspace_spec.lua">
<violation number="1" location="home-manager/programs/neovim/tests/workspace_spec.lua:49">
P2: This test doesn't actually test the workspace module - it only verifies Vim's built-in `vim.fn.getcwd()`. Consider testing workspace-specific getcwd behavior, or remove this test if the workspace module doesn't have related functionality.</violation>
</file>
<file name="home-manager/programs/neovim/tests/minimal_init.lua">
<violation number="1" location="home-manager/programs/neovim/tests/minimal_init.lua:24">
P1: Adding potentially `nil` value as first element breaks `ipairs` iteration. When `PLENARY_DIR` is not set, `os.getenv()` returns `nil`, causing `ipairs()` to stop immediately and skip all fallback paths. Consider filtering out nil or conditionally adding the path.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| it("should have sidekick module structure expected", function() | ||
| -- In full config, sidekick would be loaded | ||
| -- Here we just verify the expected API pattern | ||
| assert.is_true(true) |
There was a problem hiding this comment.
P2: Trivial assertion assert.is_true(true) always passes and provides no test coverage. Consider either implementing a meaningful test (e.g., checking module structure exists) or marking it as pending/skipped with a clear TODO.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/ai_spec.lua, line 9:
<comment>Trivial assertion `assert.is_true(true)` always passes and provides no test coverage. Consider either implementing a meaningful test (e.g., checking module structure exists) or marking it as pending/skipped with a clear TODO.</comment>
<file context>
@@ -0,0 +1,22 @@
+ it("should have sidekick module structure expected", function()
+ -- In full config, sidekick would be loaded
+ -- Here we just verify the expected API pattern
+ assert.is_true(true)
+ end)
+ end)
</file context>
| end) | ||
|
|
||
| describe("incremental selection keymaps", function() | ||
| it("should have C-space mapped for init_selection", function() |
There was a problem hiding this comment.
P2: Test assertion doesn't verify what the test name claims. vim.fn.maparg() returns an empty string when no mapping exists, so assert.is_string(keymap) will always pass. Consider either:
- Rename the test to reflect what it actually tests (e.g., 'should not error when checking C-space keymap')
- Or add a meaningful assertion like
assert.is_truthy(keymap ~= "")if the mapping should exist
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/treesitter_spec.lua, line 56:
<comment>Test assertion doesn't verify what the test name claims. `vim.fn.maparg()` returns an empty string when no mapping exists, so `assert.is_string(keymap)` will always pass. Consider either:
1. Rename the test to reflect what it actually tests (e.g., 'should not error when checking C-space keymap')
2. Or add a meaningful assertion like `assert.is_truthy(keymap ~= "")` if the mapping should exist</comment>
<file context>
@@ -0,0 +1,62 @@
+ end)
+
+ describe("incremental selection keymaps", function()
+ it("should have C-space mapped for init_selection", function()
+ local keymap = vim.fn.maparg("<C-space>", "n")
+ -- Just verify it doesn't error, may not be set in test env
</file context>
| it("should have C-space mapped for init_selection", function() | |
| it("should be able to check C-space keymap without error", function() |
| end) | ||
| end) | ||
|
|
||
| describe("completeopt", function() |
There was a problem hiding this comment.
P2: Tests modify vim.opt.completeopt but don't restore the original value, causing test pollution. Consider saving and restoring the original value, or add a before_each/after_each hook to reset state between tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/completion_spec.lua, line 19:
<comment>Tests modify `vim.opt.completeopt` but don't restore the original value, causing test pollution. Consider saving and restoring the original value, or add a `before_each`/`after_each` hook to reset state between tests.</comment>
<file context>
@@ -0,0 +1,82 @@
+ end)
+ end)
+
+ describe("completeopt", function()
+ it("should be configurable", function()
+ local completeopt = vim.opt.completeopt:get()
</file context>
| end) | ||
| end) | ||
|
|
||
| describe("getcwd", function() |
There was a problem hiding this comment.
P2: This test doesn't actually test the workspace module - it only verifies Vim's built-in vim.fn.getcwd(). Consider testing workspace-specific getcwd behavior, or remove this test if the workspace module doesn't have related functionality.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/workspace_spec.lua, line 49:
<comment>This test doesn't actually test the workspace module - it only verifies Vim's built-in `vim.fn.getcwd()`. Consider testing workspace-specific getcwd behavior, or remove this test if the workspace module doesn't have related functionality.</comment>
<file context>
@@ -0,0 +1,56 @@
+ end)
+ end)
+
+ describe("getcwd", function()
+ it("should return current working directory", function()
+ local cwd = vim.fn.getcwd()
</file context>
| -- Add plenary to runtimepath if installed via vim.pack or available | ||
| local plenary_dir = os.getenv("PLENARY_DIR") | ||
| local plenary_paths = { | ||
| plenary_dir, -- Environment variable takes precedence |
There was a problem hiding this comment.
P1: Adding potentially nil value as first element breaks ipairs iteration. When PLENARY_DIR is not set, os.getenv() returns nil, causing ipairs() to stop immediately and skip all fallback paths. Consider filtering out nil or conditionally adding the path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/programs/neovim/tests/minimal_init.lua, line 24:
<comment>Adding potentially `nil` value as first element breaks `ipairs` iteration. When `PLENARY_DIR` is not set, `os.getenv()` returns `nil`, causing `ipairs()` to stop immediately and skip all fallback paths. Consider filtering out nil or conditionally adding the path.</comment>
<file context>
@@ -19,7 +19,9 @@ vim.opt.runtimepath:append(".")
-- Add plenary to runtimepath if installed via vim.pack or available
+local plenary_dir = os.getenv("PLENARY_DIR")
local plenary_paths = {
+ plenary_dir, -- Environment variable takes precedence
vim.fn.stdpath("data") .. "/site/pack/plugins/start/plenary.nvim",
vim.fn.stdpath("data") .. "/site/pack/plugins/opt/plenary.nvim",
</file context>
![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 9406219...9799ede
Analysis
-
Test-Source Structure Mismatch - The file
tests/api/init_spec.luawas renamed fromtests/config_spec.luabut doesn't correspond to any source file atlua/api/init.lua. This breaks the mirror pattern established between test files and source files, reducing discoverability and creating confusion. -
Missing Test Helper Abstraction - Common patterns are repeated across test files without abstraction (buffer creation/cleanup, keymap verification, autocmd checking). This leads to code duplication and harder maintenance. A shared test utilities file would improve consistency and reduce boilerplate.
-
Inconsistent Test Cleanup - Some tests properly clean up resources (keymaps, buffers) while others don't. Files like
keymaps_spec.luausevim.keymap.del()but similar tests in other files (ai_spec, completion_spec) lack cleanup, potentially causing test pollution. -
Ambiguous Testing Approach for Plugin Dependencies - Tests acknowledge plugin dependencies won't be loaded in the minimal test environment, but are inconsistent about what they're actually validating. This creates ambiguity about whether tests validate correct behavior or just API availability.
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
18 files reviewed | 1 comments | Edit Agent Settings • Read Docs
| -- Tests basic configuration loading and core Neovim functionality | ||
|
|
||
| describe("config", function() | ||
| describe("init", function() |
There was a problem hiding this comment.
This file was renamed from config_spec.lua to api/init_spec.lua, but there's no corresponding source file at lua/api/init.lua. This breaks the test-to-source mirror pattern used throughout the test suite (e.g., tests/autocmds_spec.lua → lua/autocmds.lua). Consider either:
- Moving this back to
tests/config_spec.luawithdescribe("config", ...), OR - Adding a clear comment explaining why this deviates from the naming convention and what it's actually testing
The test content (settings loading, basic API operations) suggests this might be better as tests/init_spec.lua at the root level rather than under api/.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#398
File: home-manager/programs/neovim/tests/api/init_spec.lua#L4
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
This file was renamed from `config_spec.lua` to `api/init_spec.lua`, but there's no corresponding source file at `lua/api/init.lua`. This breaks the test-to-source mirror pattern used throughout the test suite (e.g., `tests/autocmds_spec.lua` → `lua/autocmds.lua`). Consider either:
1. Moving this back to `tests/config_spec.lua` with `describe("config", ...)`, OR
2. Adding a clear comment explaining why this deviates from the naming convention and what it's actually testing
The test content (settings loading, basic API operations) suggests this might be better as `tests/init_spec.lua` at the root level rather than under `api/`.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
home-manager/programs/neovim/tests/ui_spec.lua (1)
57-59: laststatus assertion always passes.The check
laststatus >= 0will always be true since valid values are 0, 1, 2, or 3. If you want to verify a specific UI configuration (e.g., always showing the statusline), consider checking for an exact value likeassert.equals(laststatus, 2)orassert.is_true(laststatus >= 2).Apply this diff if you want to verify the statusline is always shown:
- it("should have laststatus set", function() + it("should always show statusline", function() local laststatus = vim.opt.laststatus:get() - assert.is_true(laststatus >= 0) + assert.is_true(laststatus >= 2) end)
📜 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 (1)
home-manager/programs/neovim/tests/ui_spec.lua(1 hunks)
⏰ 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). (12)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: Agent
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim
- GitHub Check: lua-neovim-test
- GitHub Check: nix-nixos
- GitHub Check: nix-linux
- GitHub Check: nix-darwin
🔇 Additional comments (2)
home-manager/programs/neovim/tests/ui_spec.lua (2)
27-32: Colorscheme test is intentionally minimal.The test has been simplified to work in minimal environments (addressing past review feedback), but it only verifies that the colorscheme API exists, not any actual configuration. This is acceptable given the constraints, though the test name "should support colorscheme API" accurately reflects what it tests.
46-54: vim.ui tests verify API availability.The tests confirm
vim.ui.selectandvim.ui.inputexist, which is appropriate for a minimal environment. They don't verify if these have been enhanced by plugins (e.g., dressing.nvim), but this aligns with the test suite's minimal environment approach.
| it("should be overridden with nvim-notify", function() | ||
| assert.is_function(vim.notify) | ||
| end) |
There was a problem hiding this comment.
Test doesn't verify nvim-notify override.
The assertion only checks that vim.notify is a function, which is always true since vim.notify is a built-in Neovim API. This doesn't confirm that nvim-notify has overridden it. If verifying the override is important, consider checking for nvim-notify-specific behavior or metadata (e.g., checking if require("notify") exists). Alternatively, if the minimal test environment doesn't load nvim-notify, update the test description to reflect that it's only verifying the API exists.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by cubic
Adds a Neovim test runner and a comprehensive test suite to verify core APIs, editor settings, and keymaps. This improves reliability and makes running tests simple via make.
New Features
Migration
Written for commit 0652089. Summary will update automatically on new commits.