Skip to content

nvim api commands test #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

nvim api commands test #183

wants to merge 4 commits into from

Conversation

gigamaax
Copy link
Contributor

I started trying to fix the tests, case by case but I'm not sure if the direction I went for the first test is something you'd agree with. I also don't know enough about message pack, this codebase's implementation of messagepack, nor the internals of nvim_get_commands to know if this is dumb. so I figured I'd open up a pr and get some feedback early on.

very specifically, this pr addresses only two failing tests:

    --- FAIL: TestAPI/Command (0.00s)
        --- FAIL: TestAPI/Command/Commands (0.00s)
            --- FAIL: TestAPI/Command/Commands/Nvim (0.00s)
                api_test.go:2251: msgpack: cannot convert Nil to string
            --- FAIL: TestAPI/Command/Commands/Batch (0.00s)
                api_test.go:2267: msgpack: cannot convert Nil to string

as far as I can tell, what's happening is strings are sometimes nil. I'm not sure what the root case for that is, or if it's even really an issue. it appears to mostly only matter on the omitempty fields, which makes me feel like it should be alright if there's nothing there. so, rther than consume the error, I just said it was an empty string.

the other issue is the actual expect case. and this is the thing that I'm most worried is potentially wrong:
for v0.9, no commands are returned. for 10, three are returned, and they appear to be treesitter commands. for nightly, assuming 11 as well, but I'm still running 10 so I can't confirm, 4 are returned. it's the same three in v10, EditQuery, Inspect, and InspectTree. the last one is Open.

I'd imagine that { builtin = false } should filter those out, but maybe since they're not vim commands and more specific to neovim, they get special treatment? I coldn't find specific docs about any of the commands to figre it out.

let me know what you think. I'm happy to make any adjustments. also, let me know if you'd prefer a draft pr next time :)

for nvim minor verion 9, no commands are returned
for nvim minor version 10, three are returned
currently, for nightly builds, four are returend
Copy link

codecov bot commented Mar 29, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.1%. Comparing base (dd77a91) to head (96d7cca).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
msgpack/decode.go 0.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #183     +/-   ##
=======================================
- Coverage   83.3%   83.1%   -0.3%     
=======================================
  Files         14      14             
  Lines       3132    3134      +2     
=======================================
- Hits        2610    2605      -5     
- Misses       522     529      +7     
Flag Coverage Δ
linux-1.18-nightly ?
linux-1.18-stable 83.1% <0.0%> (-0.3%) ⬇️
linux-1.19-nightly ?
linux-1.19-stable ?
linux-1.20-nightly ?
linux-1.20-stable ?
macos-1.18-nightly ?
macos-1.18-stable ?
macos-1.19-nightly ?
macos-1.19-stable ?
macos-1.20-nightly ?
macos-1.20-stable ?
windows-1.18-nightly ?
windows-1.18-stable ?
windows-1.19-nightly ?
windows-1.19-stable 82.2% <0.0%> (-0.3%) ⬇️
windows-1.20-nightly ?
windows-1.20-stable 82.2% <0.0%> (-0.3%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@justinmk
Copy link
Member

Thank you! Yeah this looks fine to me. To see what the test is doing, run nvim -u NONE and then run:

lua =vim.api.nvim_get_commands({})

on Nvim 0.11, that indeed returns 4 commands. So this particular test is sensitive to the Nvim version.

neovim ships with default keybindings that may change per verison. the
tests, prior to this change, included these default key mappings which
make the tests fragile with each release. keeping a list of default
commands should be outside the scope of this library.

this change removes assertions against the entire set of keymaps
returned by `nvim_get_keymap` and only verifies the desired keymap has
been added and then removed.
@gigamaax
Copy link
Contributor Author

ok, cool. thank you!

I've updated the keymap test to be less specific about the version of neovim and focus a little bit more on the direct actions taken within the test. I think that should make it a lot less fragile with new verisons.

I'm going to close this for now while I work on the other failing tests and reopen when they pass

@gigamaax gigamaax closed this Mar 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants