neotest icon indicating copy to clipboard operation
neotest copied to clipboard

Support multiple runners for same test file

Open adrigzr opened this issue 2 years ago • 4 comments

Hello there!

I'm trying to implement a new runner for javascript/typescript for the test framework Mocha. Currently, I use the neotest-jest for other projects and when I try to run my runner, it runs first the jest one (it is first in the config adapters list) and fails.

Are you planning to support other runners for the same test file?

Edit: here is the runner neotest-mocha.

adrigzr avatar Sep 04 '22 08:09 adrigzr

I am planning on enabling this however for his case, really the adapters should check if their runner is installed

rcarriga avatar Sep 06 '22 18:09 rcarriga

I agree, but should the adapters check it in the is_test_file hook or in the build_spec one? I'm inclined towards doing it in the latest, but I think it needs a change in neotest for it to work.

adrigzr avatar Sep 07 '22 06:09 adrigzr

Working on neotest-vitest I tryed to follow @rcarriga suggestion and I've added a package.json check with following lines of code. It seems to work correctly. I am not experienced with Lua, and I guess it could be improved but it could be a good starting point

local function hasVitestDependency(path)
  local rootPath = util.find_package_json_ancestor(path)

  if not lib.files.exists(rootPath .. "/package.json") then
    print("package.json not found")
    return false
  end

  local packageJsonContent = lib.files.read(rootPath .. "/package.json")
  local parsedPackageJson = vim.json.decode(packageJsonContent)

  if parsedPackageJson["dependencies"] ~= nil then
    for key, _ in pairs(parsedPackageJson["dependencies"]) do
      if key == "vitest" then
        return true
      end
    end
  end

  if parsedPackageJson["devDependencies"] ~= nil then
    for key, _ in pairs(parsedPackageJson["devDependencies"]) do
      if key == "vitest" then
        return true
      end
    end
  end

  return false
end

adapter.root = function(path)
  if not hasVitestDependency(path) then
    return
  end

  return lib.files.match_root_pattern("package.json")(path)
end

adapter.root = getRoot

marilari88 avatar Sep 08 '22 07:09 marilari88

Yep that's exactly what I had in mind! It should be done in root detection, is_test_file and build_spec already mean that the adapter is active which for this case it shouldn't be.

Since you mentioned you're new to lua, just FYI it's not far from JS in many ways :smile: parsedPackageJson["dependencies"] == parsedPackageJson.dependencies and if thing ~= nil then is the same as if thing (unless it's false). Purely style based changes though, functionally I see nothing wrong. The only thing I can think of is if in a monorepo with multiple test runners but I don't how likely/niche of a use case that is, I think this would work for 99% of people

rcarriga avatar Sep 09 '22 07:09 rcarriga

Hello @marilari88,

Thanks for the suggestion. The problem with this approach is that the adapter.root method is always executed given the current cwd. This means that if you have a monorepo with several packages with different test runners, you cannot select the proper runner for each file. The only way I see to solve this issue is by selecting the adapter based on the current file.

.
└── packages
    ├── bar
    │   ├── package.json <- jest based package
    │   └── test
    │       └── index.js
    └── foo
        ├── package.json <- mocha based package
        └── test
            └── index.js

Regards,

adrigzr avatar Feb 07 '23 12:02 adrigzr

hi @adrigzr. How to select the adapter based on current file?

I tried the monorepo with https://github.com/marilari88/turbo-example with neotest-vitest and neotest-jest, and I realized they interfere with each other. I think our adapters should tell neotest which test runner each file refers to. And we can do this through Adapter.is_test_file. I opened this PR in neotest-vitest: https://github.com/marilari88/neotest-vitest/pull/19 I'm going to test it (even with mocha) and eventually propose you the change

image

marilari88 avatar Feb 08 '23 00:02 marilari88

Hi @marilari88, thats exactly what I meant. Neotest should decide on the adapter to use based on a file and its context and not on root folder detection. Your suggested approach sounds very promising. 👍

adrigzr avatar Feb 08 '23 06:02 adrigzr

I would like to volunteer working on this issue (if no one else is) since I'm involved in projects that either use mocha or jest. I'll try to summarise the approach that should be taken as described by the previous comments.

  • The check should happen in the is_test_file function or similar to support both ordinary projects with a single package.json in the root and monorepos. One could search upwards from the current file's path to the root for a package.json which should handle both cases.
  • Otherwise, it should be done in the root detection method but each test file might need its own root to be set to support monorepos. I don't know if neotest supports this.
  • @marilari88's code looks good to me too so I'll use this as a foundation.

I think it would make sense to either expose utility functions for finding package.json files in neotest itself so other test adapters can use it or add them to the few js test adapters that need it.

Hopefully, I'm on the right track but please provide any feedback you might have 🙂

MisanthropicBit avatar Jun 03 '23 15:06 MisanthropicBit

I've had some time to look into this more and neotest-mocha already has a PR open for adding this functionality while neotest-jest implements this already.

In conclusion, I don't think it is necessary for me to start working on this issue as it is already underway, at least for jest and mocha.

MisanthropicBit avatar Jun 18 '23 10:06 MisanthropicBit