✨ Add support for user defined dap strategy

cpb commented 4 weeks ago

Why?

The default strategy configuration for dap does not work with current versions of nvim-dap or nvim-dap-ruby. It is missing the request field.

What?

I am proposing enabling users to provide their own strategy config implementation so they can customize it to their needs.

Alternatives

Change the existing get_strategy_config to include:

  request = "attach",
  error_on_failure = false,
  localfs = true,
  waiting = 1000,

Todo

[ ] Update documentation
[x] Suppress exit code 1 warning for test failures
- error_on_failure = false,
[x] ~Fix inconsistent population of test output~
- dap-ruby appends stdout to dap.repl instead of the process output tempfile
- dap-ruby is configured as a type = "server" and it'll be challenging to capture the output stream as is
[x] ~Fix inconsistent coloring of test output~
- When dap-ruby runs the specs, it doesn't have a TTY -- so the .rspec configured --color is ignored.
- If --force-color is set, then the dap.repl includes the color codes

Demo

https://github.com/user-attachments/assets/2d86ca90-69e5-4a74-84d0-0d259cab5a3e

olimorris commented 4 weeks ago

Thank you. I'll add @elken to this who authored the original PR

elken commented 4 weeks ago

I can confirm it does work as I've been using it, unless you mean something changed in the week or so since?

EDIT: Seems something has changed, okay but I don't see this PR adding it

cpb commented 4 weeks ago

I can confirm it does work as I've been using it, unless you mean something changed in the week or so since?

EDIT: Seems something has changed, okay but I don't see this PR adding it

I get an error message:

Config needs the request property which must be one of attach or launch

https://github.com/user-attachments/assets/2c24dd53-6f5b-4e77-94e5-a910139d603f

This PR makes the strategy configuration user editable.

An alternative would be to include these lines in the default strategy configuration:

  request = "attach",
  options = { source_filetype = "ruby" },
  error_on_failure = false,
  localfs = true,
  waiting = 1000,

At least request = "attach" is needed … I put all these in my user defined implementation of strategy_config but I suspect some may not be needed.

cpb commented 4 weeks ago

@elken I interpreted your statement:

okay but I don't see this PR adding it

As desiring a default configuration which worked!

This PR should now work without requiring users to override the default strategy_config. It does so by adding these values to the default get_strategy_config:

  request = "attach",
  error_on_failure = false,
  localfs = true,

elken commented 4 weeks ago

diff --git a/lua/neotest-rspec/init.lua b/lua/neotest-rspec/init.lua
index c449491..75aa9db 100644
--- a/lua/neotest-rspec/init.lua
+++ b/lua/neotest-rspec/init.lua
@@ -123,8 +123,13 @@ function NeotestAdapter.build_spec(args)
   local function get_strategy_config(strategy, command, cwd)
     local strategy_config = {
       dap = function()
+        vim.fn.setenv("RUBYOPT", "-rdebug/open")
+        vim.fn.setenv("RUBY_DEBUG_COMMANDS", "c ;;")
         return {
           name = "Debug RSpec Tests",
+          request = "attach",
+          localfs = true,
+          waiting = 1000,
           type = "ruby",
           args = { unpack(command, 2) },
           command = command[1],

The above seems to work consistently now.

The env vars are needed for "reasons". Also probably worth noting that I'm calling bin/rspec in my config.

The application I was testing it on must have been setting up debug to load correctly.

cpb commented 4 weeks ago

@elken indeed, a simpler contribution!

The setenv stuff could be simplified a bit.

nvim-dap-ruby https://github.com/suketa/nvim-dap-ruby/blob/main/lua/dap-ruby.lua#L81 configures debug to open (if required) so one change to your diff could be to simply:

diff --git a/lua/neotest-rspec/init.lua b/lua/neotest-rspec/init.lua
index c449491..75aa9db 100644
--- a/lua/neotest-rspec/init.lua
+++ b/lua/neotest-rspec/init.lua
@@ -123,8 +123,13 @@ function NeotestAdapter.build_spec(args)
   local function get_strategy_config(strategy, command, cwd)
     local strategy_config = {
       dap = function()
+        vim.fn.setenv("RUBYOPT", "-rdebug")
+        vim.fn.setenv("RUBY_DEBUG_NONSTOP", true)
         return {
           name = "Debug RSpec Tests",
+          request = "attach",
+          localfs = true,
+          waiting = 1000,
+          error_on_failure = false,
           type = "ruby",
           args = { unpack(command, 2) },
           command = command[1],

Would you prefer to not make the strategy configurable by users, and instead simply update get_strategy_config ?

I still believe neotest-rspec will need to include error_on_failure = false, in the strategy, so as to not notify the user with a message about an exit code of 1.

My rspec_cmd is configured like so, which works well with the nvim-ruby-dap existing environment variables, and is an alterative to using RUBYOPTS

rspec_cmd = function()
  return vim.tbl_flatten({
    "bundle",
    "exec",
    "rdbg",
    "--nonstop",
    "-c",
    "--",
    "rspec",
  })
end,

elken commented 3 weeks ago

I'm not beholden to my code :smile: whichever change is least destructive and simplest for the user.

RE: making it configurable, I would see what the other adapters do. I don't know if it needs to be, but it's obviously not a massive deal if it is :smile:

cpb commented 3 weeks ago

Closing in favour of:

https://github.com/olimorris/neotest-rspec/pull/72

Pending further discovery in trends of adapter's exposing dap strategy configuration.

olimorris / neotest-rspec