nvim-neotest / neotest

An extensible framework for interacting with tests within NeoVim.
MIT License
2.35k stars 115 forks source link

[BUG] Vim:E155: Unknown sign: neotest_unknown #130

Closed orjangj closed 2 years ago

orjangj commented 2 years ago

NeoVim Version Output of nvim --version

NVIM v0.8.0-1210-gd367ed9b2
Build type: Release
LuaJIT 2.1.0-beta3

Describe the bug The "unknown" status is missing in the list of statuses:

# In file: lua/neotest/consumers/status.lua
...
local function init(client)
  local statuses = {
    passed = { text = config.icons.passed, texthl = config.highlights.passed },
    skipped = { text = config.icons.skipped, texthl = config.highlights.skipped },
    failed = { text = config.icons.failed, texthl = config.highlights.failed },
    running = { text = config.icons.running, texthl = config.highlights.running },
    -- This is missing
    unknown = { text = config.icons.unknown, texthl = config.highlights.unknown },
  }
  for status, conf in pairs(statuses) do
    async.fn.sign_define("neotest_" .. status, conf)
  end
   ...
end
...

I've tested that adding the missing unknown entry fixes the issue.

To Reproduce

Minimal config (as per install instructions in the readme):

use {
  "nvim-neotest/neotest",
  requires = {
    "nvim-lua/plenary.nvim",
    "nvim-treesitter/nvim-treesitter",
    "antoinemadec/FixCursorHold.nvim"
  }
}

No need to produce the error exactly, just execute the following lua command in nvim:

:lua print(("%s"):format(vim.inspect(vim.fn.sign_getdefined("neotest_unknown"))))

It returns an empty {}

Here's a comparison with neotest_passed:

{ {
    name = "neotest_passed",
    text = " ",
    texthl = "NeotestPassed"
  } }

Expected behavior

This command:

:lua print(("%s"):format(vim.inspect(vim.fn.sign_getdefined("neotest_unknown"))))

should produce something like this:

{ {
    name = "neotest_unkown",
    text = "? ",
    texthl = "NeotestUnknown"
  } }

Let me know if you need more context.

orjangj commented 2 years ago

Created a PR to fix the issue: https://github.com/nvim-neotest/neotest/pull/131

rcarriga commented 2 years ago

There is no "unknown" result status (the values are defined here https://github.com/nvim-neotest/neotest/blob/7690967f599b15cd6bc93030693ed9a274e87aae/lua/neotest/types/init.lua#L9. If you are seeing an error around a neotest_unknown sign then your adapter is probably returning an invalid status for a test. Which adapter are you using?

orjangj commented 2 years ago

Ok. I see. I've misunderstood/overlooked what possible values the results function of an adapter could return then.

I'm currently writing my own adapter for ctest runner: https://github.com/orjangj/neotest-ctest

It's closely related to the neotest-gtest adapter (which is discussed here https://github.com/nvim-neotest/neotest/discussions/107), but my adapter is using ctest (which comes bundled with cmake) as the runner instead of make/binaries. One of the benefits of using ctest as the runner is that it handles parameterized tests as well. I initially thought extending neotest-gtest would be a good idea, but ctest doesn't necessarily depend on the google test framework (although that's the only framework my adapter is currently supporting).

I hope this adapter can be added to your list of supported adapters in the future. It's working quite well, but has a few bugs and edge-cases I need to sort out before I can mark it as ready-for-use.

Anyway... my reasoning for returning "unknown" status in my adapter is that C/C++ are compiled languages, so if one or more tests were not compiled before attempting to run the tests, then I would return the "unknown" status as these tests are not handled by ctest but are picked up by the treesitter parser.

If you think adding unknown to the list of statuses is not the right solution, then I can simply mark the tests as skipped instead. I just thought that marking them as unknown would be more useful.

Let me know what you think :)

orjangj commented 2 years ago

Actually... after giving it some more thought, I think I'll just mark non-compiled tests as skipped. It makes parts of my adapter a lot easier to implement as well. It should be the users responsibility to ensure tests are compiled, so I'll just mention that in the readme of my own adapter.

I'll close this ticket. Someone else can reopen it if they think it would be useful to include unknown as a possible status response. I'll close the linked PR as well.

Thank you for your time :)

rcarriga commented 2 years ago

Cool always great to see new adapters! :smile:

As you said,I think skipped makes sense here, functionally it fulfills the same role of "this didn't run". The complexity of having another status just for the specific case of something not being compiled would not be worth it. One thing you could do is set the short property of the the skipped result to a message something like Test not found, try re-compiling so users would be able to see it