orjangj / neotest-ctest

A neotest adapter for C/C++ using CTest as a test runner. Supports GoogleTest, Catch2 and doctest test frameworks.
MIT License
8 stars 4 forks source link

nio.process.run not working on my setup #24

Closed pcorbineau closed 5 months ago

pcorbineau commented 5 months ago

Setup : Windows 11 Neovim : 0.10.0

Hey, I test your plugin today, and start by debugging it.

For unknown reason the nio.process.run dont look to works on my setup. So I replace it with neotest.lib one to fix it.

Here is the new ctest:run function in ctest.lua :

function ctest:run(args)
    local cmd = { "ctest", unpack(args) }
    local _, result = lib.process.run(cmd,
        { stdout = true, cwd = self._test_dir })

    return result.stdout
end
orjangj commented 5 months ago

Thank you for reporting. Nice to see someone trying out my plugin.

I don't have access to a Windows setup at the moment, so I won't be able to look into why nio.process.run is not working for you any time soon. Sounds like an upstream/nio issue though considering it's working on Linux. What version/commit of nio are you using? Are you getting any kind of error printed in neovim with regards to nio? neotest-ctest is bit lacking in error handling/reporting at the moment, but could you try the following and post the results from the print statement here (run :messages to see the whole print output)?

function ctest:run(args)
  local runner, err = nio.process.run({
    cmd = "ctest",
    cwd = self._test_dir,
    args = args,
  })

  if not runner then
    print(vim.inspect(err))
    return
  end

  local output = runner.stdout.read()
  runner.close()

  return output
end

Anyway... the fix you provided looks wrong. lib.process.run doesn't have a cwd option. Which will cause ctest to look for tests in the project root directory when it's supposed to look for tests in i.e. build/Debug/.. or similar.

This should work however:

function ctest:run(args)
  local cmd = { "ctest", "--test-dir", self._test_dir, unpack(args) }
  local code, result = lib.process.run(cmd, { stdout = true })

  return result.stdout
end

I'm inclined to believe that #25 is due to passing cwd to lib.process.run as part of its argument list. Could you verify this and close #25 if this is the case?

pcorbineau commented 5 months ago

I'm a beginner in lua and neovim conf. I try my best to debug it. I had a lot of print on each line :

function ctest:run(args)
  local runner = nio.process.run({
    cmd = "ctest",
    cwd = self._test_dir,
    args = args,
  })

  if not runner then
    return -- I dont pass here
  end

  local output = runner.stdout.read()
  -- output = ""
  runner.close()

  return output
end
pcorbineau commented 5 months ago

For now, I have a functional plugin with my two fixes from #24 and #25. I can test the whole test suite, per file, or per test. BTW, thanks to wrote this plugin.

pcorbineau commented 5 months ago

Anyway... the fix you provided looks wrong. lib.process.run doesn't have a cwd option. Which will cause ctest to look for tests in the project root directory when it's supposed to look for tests in i.e. build/Debug/.. or similar.

This should work however:

function ctest:run(args)
  local cmd = { "ctest", "--test-dir", self._test_dir, unpack(args) }
  local code, result = lib.process.run(cmd, { stdout = true })

  return result.stdout
end

I'm inclined to believe that #25 is due to passing cwd to lib.process.run as part of its argument list. Could you verify this and close #25 if this is the case?

It works on my side because function ctest:command(args) pass --test-dir, self._test_dir as parameters. Indeed I remove the cwd from lib.process.run ( sad github copilot suggestion on something I dont master ).

pcorbineau commented 5 months ago

Here you can find my working setup, this is probably not a good fix, but it works on my setup : https://github.com/pcorbineau/neotest-ctest/tree/fix_windows_run

orjangj commented 5 months ago

I see :) good to know you have something that works for you temporarily.

I'm currently working on implementing some integration tests that will run on all major platforms (Linux, MacOS and Windows) in Github Actions. Hopefully, I will be able to reproduce the problem with nio there. But it will take a few or more days to get there depending on how much time I have available to work on it.

If the problem occurs in integration testing, then I'll apply the fix discussed in this issue. I'll let you know once I have something to report.

pcorbineau commented 5 months ago

I see you found fixes from my two bugs on windows on your intergration test branch. I confirm it works on my side with this branch 🙏

orjangj commented 5 months ago

Good to hear. Sorry for the long wait though. I've been having trouble with catch_discover_tests and doctest_discover_tests in CI (some dll's not being found). Works fine in a Windows VM I have locally, so not sure what causes the problem. Something about the GH windows runner environment I suppose and how these cmake functions work. Anyway.. I have some platform integration coverage with GTest now, so catch2 and doctest will have to wait.

I was not able to figure out why nio.process.run didn't work on Windows, and I'm not sure I'm willing to spend more time on it either :P Your suggested fix, with some minor modifications as discussed in this tread seem to do the trick.

I will merge the fix any minute now. If you find other issues, please feel free to open a new issue.

orjangj commented 5 months ago

Thank you for the issue report and effort on finding the root cause @pcorbineau. You might want to change your setup to use the 0.1.0 tag or latest master branch now. I'm going to delete the 27-integration-test branch to keep the project clean.

GingerbreadFred commented 3 months ago

I'm finding this issue occurs on macos with the new code. When running a ctest --show-only=json-v1 via lib.process.run the process launches but the completion callback from inside the neotest call never seems to return.

It looks like an issue with vim.loop.spawn I think not correctly handling the way that ctest is outputting to stdout but thats deep within libuv and I haven't been able to debug it.

Reverting to the previous version works for me. I'm interested as to how the Mac integration tests are passing (or if anyone else running on mac has this issue) the same project setup works fine on windows. I can push a fork that branches how the process is launched based on the OS but that doesn't seem like the right fix :/