olimorris / neotest-rspec

🧪 Neotest adapter for RSpec. Works in Docker containers too
MIT License
93 stars 27 forks source link

How to run with docker #28

Closed vinibispo closed 1 year ago

vinibispo commented 2 years ago

Hey, incredible work on this repository, and I think it would be better if we could run with docker, too, because the vast majority of companies use docker nowadays. I thought like we could run this, setting custom_command as docker Like:

require("neotest-rspec")(
{ custom_command = "docker exec -it mytestcontainer rspec" }
)
olimorris commented 2 years ago

@vinibispo - I've made it possible to have a custom rspec command in the latest commit. Please let me know if this works

pBorak commented 2 years ago

@vinibispo Have you been able to use neotest-rspec alongside docker with this command? I am having issues with paths - neotest uses absolute ones. https://github.com/nvim-neotest/neotest/issues/89

vinibispo commented 2 years ago

I'll test it, sorry for the delay @olimorris

olimorris commented 2 years ago

Yeah my hunch is that absolute paths are going to be the killer here...

evertonlopesc commented 2 years ago

@olimorris I did the following test:

local neo = require('neotest')
local rspec = require('neotest-rspec')

neo.setup({
  adapters = {
    rspec({
      rspec_cmd = function()
        return vim.tbl_flatten({
          "docker exec -it <container> bin/rspec ",
        })
      end
    }),
  }
})

I have this return:

image

I also did the following test:

neo.setup({
  adapters = {
    rspec({
      rspec_cmd = function()
        return vim.tbl_flatten({
          "docker",
          "exec -it",
          "container",
          "bin/rspec"
        })
      end
    }),
  }
})

I have this return:

image

Do you have any other test ideas?

pBorak commented 2 years ago

@evertonlopesc I have forked this repo in the past, tweaked the command to use docker, and had the same results as you. This is due to the absolute paths :/ the issue I mentioned here.

olimorris commented 2 years ago

Let's keep this open for now and monitor this issue as @pBorak has kindly highlighted

evertonlopesc commented 2 years ago

@pBorak I read this comment and also your issues on neotest.

@olimorris ok, I got it

Thanks for your attention!

mmirus commented 2 years ago

Hey, all! Short term, my solution is to use a shell script as my rspec executable, and mutate the paths as needed in that script.

Example below. You'll obviously need to substitute your own particulars.

#!/bin/bash

# Strip "/home/mmirus/git/repo/path/" from test paths sent to container
args="$(echo "$@" | sed 's/\/home\/mmirus\/git\/repo\/path\///g')"
docker compose exec web bundle exec rspec $args

Then you just do this:

require("neotest").setup({
  adapters = {
    require("neotest-rspec")({
      rspec_cmd = "my-script.sh",
    }),
  },
})

This is basically the formula I've used in the past with other plugins and editors that had trouble with running tests or tools in a container. Tell them to run a shell script as the executable, do any arg transformations needed in the script, then call the container with the final result.

It seems to work here, but it's barely tested.

Thanks for this adapter, Oli!

pBorak commented 2 years ago

@mmirus Hey, thank you for your comment, really smart move with this script ❤️
I tested your workaround and it seems to work for running specs in general. Unfortunately, I've noticed that neotest test results do not work correctly - all tests are reported as failed. Is that functionality working for you?

mmirus commented 2 years ago

I did see that too. I haven't had time to look into it yet. The most obvious explanation is that something about the output from docker (or the script) is not as expected. Perhaps we can capture "normal" output and output from the script and compare.

olimorris commented 2 years ago

@mmirus could be worth you posting this on the Neotest issue regarding Docker if you haven't done already. Your approacapproadh is brilliant and perhaps @rcarriga mayshave some thoughts regarding the output.

mmirus commented 2 years ago

Good suggestion! I'll do that and see if they have any ideas.

mmirus commented 2 years ago

FWIW, this version of the script might be a slightly more correct way of handling the arg transformation, but it doesn't seem like it makes any practical difference. :man_shrugging:

#!/bin/bash

# Strip "/home/mmirus/git/repo/path/" from test paths sent to container
args=("${@/\/home\/mmirus\/git\/repo\/path\//}")
docker compose exec web bundle exec rspec "${args[@]}"
mmirus commented 1 year ago

Some progress over on the neotest ticket. https://github.com/nvim-neotest/neotest/issues/89#issuecomment-1337633498

@olimorris Does any of this spark ideas for how a more shippable solution could be constructed?

pBorak commented 1 year ago

@mmirus Tested out your 2nd solution. Works wonderfully!

edit: not sure if it's problem with the workaround or the neotest architecture itself but when I put binding.pry and try to debug while testing I cannot get into REPL 😕

edit 2: Just learned about require("neotest").run.attach(). It works!

olimorris commented 1 year ago

Some progress over on the neotest ticket. nvim-neotest/neotest#89 (comment)

@olimorris Does any of this spark ideas for how a more shippable solution could be constructed?

Nice work! This is super exciting.

I am more than happy to accept a PR (looks like you've done all the hard work already) to change how we call Neotest based on RSpec being run inside a Docker container. We could always pass a config flag or have a global variable.

mmirus commented 1 year ago

@olimorris I think the approach outlined in my latest comment is the one I prefer of the two I suggested in my comments over there.

If you agree, then no change is needed to the neotest-rspec code, unless you want to add a note to the README about how to set up the adapter to work with docker.

olimorris commented 1 year ago

@mmirus sorry I didn't see it. Whilst there's not direct support for it in neotest yet, I'd prefer to add it as a note in the readme in the usage section if you'd like to PR it

mmirus commented 1 year ago

No need to apologize! I posted it after your last comment here. I'll send up a PR for the README sometime soon.

olimorris commented 1 year ago

Big shoutout to @ramblex for the latest PR which adds support for Docker