nvim-neotest / neotest

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

Path maps for Docker #89

Open lukas-reineke opened 2 years ago

lukas-reineke commented 2 years ago

neotest uses the absolute path for files, but when the tests run in Docker, those paths don't match.

Would it be possible to add some kind of path mapping to fix this? Something like DAPs localRoot and remoteRoot.

rcarriga commented 2 years ago

I'm not opposed to the idea of this but also I'm not sure how this should be implemented. Off the top of my head I believe this would have to be implemented by adapters rather than by neotest itself, as the usage of paths is completely up to them. Of course it'd be better if there was one implementation that could be shared, There could be a translate_path function somewhere for adapters to use :thinking:

lukas-reineke commented 2 years ago

I haven't looked too deep into it, but from what I could tell, the adapters wouldn't need to change. The translation should be directly at the beginning from Neovim buffer to filepath. All adapter functions are called with the translated path, so adapters only ever look at things in Docker. And at the end when parsing the results.

rcarriga commented 2 years ago

Unfortunately that wouldn't work well. It would require adding translation logic to both consumers and the core code, and would also hide the translation from adapters which shouldn't be done in case they need to use the file path for some type of functionality (e.g. project root inference).

lukas-reineke commented 2 years ago

It would require adding translation logic to both consumers and the core code

I agree, consumers should not have to worry about this.

and would also hide the translation from adapters which shouldn't be done in case they need to use the file path for some type of functionality (e.g. project root inference).

IMO if the tests run inside docker, everything needed to run the tests should be inside docker too. Project root should resolve to the root inside docker, etc.

Maybe the translation can happen directly before calling the adapter, and directly after receiving results from the adapter.

rcarriga commented 2 years ago

Maybe I'm misunderstanding, but from what I understand the neovim process is outside of the container and the test commands are running docker exec ... commands (or something similar). That means that to read the files, an adapter needs to use the host OS paths, not the docker paths and so we can't hide mappings.

Unfortunately I believe the only route for this is to provide the mappings to the adapter when building commands/results and let them handle it manually

lukas-reineke commented 2 years ago

But that's only a problem if the adapter needs to read the file, would that ever be necessary outside of discover_positions? And discover_positions could be called with the original path (this does get a bit messy though I agree)

rcarriga commented 2 years ago

Yes unfortunately the adapters use the filesystem in both the root function and results

pBorak commented 2 years ago

If I were to use it solely for docker, would it be just matter of changing the root and results function (locally/forking), not to use absolute path, but rather the one matching the docker one?

lukas-reineke commented 2 years ago

yeah, it's not that easy.. root needs to run on the local file system as well, same as discover_positions.

You're probably right, the only way to not make this a total mess would be to provide translate functions that the adapter can use when they need to.

rcarriga commented 2 years ago

Yeah it wouldn't be any easier if you only used docker. The same changes would be needed unfortunately.

I'll get to working on this at some stage but I don't use docker for testing so I can't say it'll be a priority. If someone wants to work on this, I'd be happy to discuss :smile:

rcarriga commented 2 years ago

A thought that I've had with this is that even if we implement some sort of path mappings in the adapter interface, several adapters won't be able to run without some very gnarly mounting. For example neotest-python uses a custom wrapper script which would have to be mounted as well as a temp file that would also have to be mounted separately.

It's looking like it'd be better to solve this problem doing what VSCode does and just install Neovim in the container along with plugins and run from there. There is https://github.com/esensar/nvim-dev-container which looks like it might work but I've no clue how much neotest would have to be aware of it. Just noting ideas down

mmirus commented 1 year ago

:wave: For running rpec tests in a docker container, I suggested using a shell script as an intermediary over at https://github.com/olimorris/neotest-rspec/issues/28#issuecomment-1334550741.

  1. Create a shell script that invokes rspec in docker with the args passed to it
  2. Point neotest at the shell script as the rspec executable

It's easy to transform the paths as needed in the script.

#!/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 works; with this setup, neotest can run the tests in the container.

The next problem is that neotest reports all the tests as failures, regardless of the actual result. The likely explanation is that when the rspec output comes from the container / script, it's different from what neotest expects.

@rcarriga Do you have any ideas on what the problem might be, or suggestions for troubleshooting it?

rcarriga commented 1 year ago

I'm afraid you're going to find a lot of issues with that approach, hence why I don't it's worth pursuing this without a more holistic approach.

The issue you're probably running into is neotest-rspec instructs rspec to write the results to a file and then reads the results from that file https://github.com/rcarriga/neotest-rspec/blob/3dac4782b61a4d8fbc61d627b36f245326959b77/lua/neotest-rspec/init.lua#L143. That file won't be filled if it's run in docker without mounting it. If you manage to solve that issue, the paths in the results will all then need to be translated back to the host machine paths.

mmirus commented 1 year ago

Thanks for pointing me in the right direction. I got at least part of the way there.

  1. Disable rspec writing the output to a file (remove these two lines)
  2. Replace the output file path with a static path (/tmp/neotest-rspec or whatever)
  3. Update the script I provided as below (again, substitute your particulars). Make sure the file path matches what you put in neotest-rspec.
# Strip "/home/mmirus/git/repo/path/" from test paths sent to container
escaped_path="\/home\/mmirus\/git\/repo\/path\/"
args=("${@/$escaped_path/}")
docker compose exec web bundle exec rspec "${args[@]}" | tee /tmp/neotest-rspec | sed '/^{/d'
# Remove all lines except the JSON from the rspec output file
sed -i '/^{/!d' /tmp/neotest-rspec

I did not find it necessary to do anything with the paths in the results.

One thing to note is that neotest-rspec is trying to have rspec output two results: json test results to a file, and a human-readable test result (the rspec "progress" formatter) to stdout. Trying to preserve both of those is the trickiest bit. If we can change that to use only the json output without losing much user-facing functionality, that might simplify things.

This isn't intended to be a shippable solution as-is, but perhaps something can come of it. For now, just exploring what's possible.

Overall: image

Individual test result: image


Another option that would work for some cases would be to just have rspec write the output file to a location that is shared between the container and the host system. This is simpler and safer than messing about with filtering and redirecting test output, but of course has the downside of writing a results file within your project.

In that case, I believe this is all that's needed:

  1. Set the results path here to a location that's shared between the container and the host. Use the path on your local system, not the path inside the container.
  2. Filter that path in the command args passed to your script (substitute your particulars):
  # Strip "/home/mmirus/git/repo/path/" from test paths sent to container
  escaped_path="\/home\/mmirus\/git\/repo\/path\/"
  args=("${@/$escaped_path/}")
  docker compose exec web bundle exec rspec "${args[@]}"

And there you go:

image

You can see that the output in the panel fares better in this approach.

rcarriga commented 1 year ago

I did not find it necessary to do anything with the paths in the results.

Ah brilliant, it must just use relative paths :+1:

Another option that would work for some cases would be to just have rspec write the output file to a location that is shared between the container and the host system.

You could parse the path from the args in your script and then docker cp <container>:<path> <result_path> after the docker exec right?

mmirus commented 1 year ago

@rcarriga Ah brilliant, it must just use relative paths +1

Yes, it seemed like while the path originally provided to rspec by neotest is absolute, the paths in the rspec output are themselves relative.

@rcarriga You could parse the path from the args in your script and then docker cp : after the docker exec right?

Smart idea! This seems to do the trick, without requiring any changes to the neotest-rspec code. Here's the script users would modify and specify as their rspec exec:

#!/bin/bash

# Customize the following:
# - escaped_path: absolute local path to your project
# - container: name of your docker container
escaped_path="\/home\/<user>\/project\/path\/" # Be careful to properly escape this
container="container_name"

# WARN: This will break if flags other than -o and -f are added in neotest-rspec
while getopts o:f: flag; do
    # This deliberately does not handle all arguments
    # shellcheck disable=SC2220
    # shellcheck disable=SC2213
    case "${flag}" in
        o) output_path=${OPTARG} ;;
    esac
done

# Strip local path from test paths sent to container
args=("${@/$escaped_path/}")

# Run the tests
docker compose exec "$container" bundle exec rspec "${args[@]}"

# Copy the test output from the container to the host
docker compose cp "$container:$output_path" "$output_path"

As long as the /tmp/... path provided by neotest-spec is writable both in the container and on the host, this should work.

And then, as shown above:

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

Result:

image

You can see in the summary that neotest thinks some other tests in the suite have failed, which is not correct (you can see in the panel that only one failure occurred). I don't know if that error in the summary is due to this method, or is a neotest / neotest-rspec bug which would be happening regardless.

Sláinte!

rcarriga commented 1 year ago

Awesome that it works! :grin: Not sure why you're seeing the failed tests in the summary. Neotest does some result aggregation which can mark tests as failed if results are not returned for them so if the tests are present in the container then that would explain it. Otherwise the logs might contain some useful information with the trace level set.

meicale commented 1 year ago

It is wonderful to make it rspec, which is a test framework for Ruby. How about neotest-python, are there any counter part to "rspec_cmd", and how to set it. I just monkey-copy and replace runner whith the provided scripts, but it doestn't work . Thank you!

mmirus commented 1 year ago

Sorry, @meicale, I don't work in Python, so I can't help. You'll need to do some experimenting.

If memory serves, I used these debugging techniques:

  1. Run the script from the command line instead of with neotest and see what the output is
  2. Run the script using neotest and look at the results in the output panel

Hopefully between the output in those two locations you'll be able to figure out what's going wrong and adjust the script as needed.

If you get it working, please share your changed version here for others!

rcarriga commented 1 year ago

neotest-python would be more difficult to make work with docker. It uses a wrapper module that writes JSON results to a file that is read by the adapter (i.e. the neovim process). To make it work you'd have

  1. Mount the wrapper module into the container
  2. Mount the JSON results file so that the NeoVim process can access it or replace the path in the args (as the rspec script does)
  3. Translate paths passed to python (same as the rspec script does above)
  4. Translate the paths in the results back, because the paths are absolute in the results unlike rspec
camilledejoye commented 1 year ago

Hi, jumping into the conversation since I'm also mostly working with containers. I started to adapt the phpunit adapter and managed to make it work both on the host and remote.

For the issue of local vs remote path it's manageable only by the adapter, without the rest of neotest being aware of it. In build_spec make the position.path relative so that the test program works in both cases. In results if the test program returns full file paths they must be mapped to local paths. This is doable without too much issues by providing a path_mappings like DAP.

The real issue is to be able to read the file containing the test result since it's going to be created inside the container. The quickest solution is to use a wrapper script to identify when the test program is run through neotest and copy the file from the container to host. But that's really inefficient and requires constant wrapping of the test programs in each project with a similar configuration to not make the configuration of neotest too hard to maintain. We could provide a "container" configuration to the adapter so that it does the copy itself but that's not really any better: container names changes from project to project, etc. One could imagine handling a volume to share those files but that's still requiring file system operations, we can only mount directories from the project root so we need to be able to configure where the adapter is going to write this file and override the definition of the container for a personal use case (not easily done in all projects).

From my experience the most painless way to work with containers is to not rely on the file system but solely on stdin, stdout and stderr when it's possible. In the case of the phpunit adapter it would be possible to use the TeamCity format as output which would then enable to process it in real time and without knowing where the process is actually running. I saw that the lib.files provide a stream feature and I was wondering if it could be possible to put something similar in place for stdout instead of a file ? The main downside here is that we will loose the regular output, if we use docker compose exec ... then docker compose will merge stdout with stderr and if we use docker compose exec -T ... to disable the TTY and keep them separated then the container does not have access to a /dev/stderr anymore to write on :(

But it's probably acceptable, IMO you either want to have neotest handle test result and only showing you relevant information: success, failure, failure messages Or you want to have access to the output and analyze yourself, maybe a strategy to add there which will be a behavior like vim-test by running the test program in an emulated terminal.

Anyway, I digress a bit here, the main question is how can I access the output from within the adapter to process the results from it without going through a file ? It seems there is already a temporary file generated by neotest in order to show the output at any time. Would it be possible to return a callback from an adapter to process this output in real time ?

meicale commented 1 year ago

I think it is reasonable to add a layer like DAP. I use dap and debugpy to debug python code in the container and remote server, without too much config and efforts. LSP needs similar ability, too. If this works, it will be as good as VSCode to work with remote if not better.

rcarriga commented 1 year ago

@camilledejoye A custom strategy is an interesting idea. I don't use containers at all for testing but if someone is interested in opening a PR I'd be happy to help. It could work like the dap strategy where is requires some configuration from the adapter but abstracts the common needs of actually running the process.

It seems there is already a temporary file generated by neotest in order to show the output at any time. Would it be possible to return a callback from an adapter to process this output in real time ?

Adapters can already process the output of the process. It can provide a stream function in the RunSpec which gets an iterator of the process output. https://github.com/nvim-neotest/neotest/blob/bbbfa55d850f1aaa6707ea85fb5230ac866459c6/lua/neotest/types/init.lua#L43