Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run more than a single test at once #45

Closed
saep opened this issue Feb 26, 2023 · 6 comments · Fixed by #48
Closed

Run more than a single test at once #45

saep opened this issue Feb 26, 2023 · 6 comments · Fixed by #48
Assignees
Labels
bug Something isn't working

Comments

@saep
Copy link
Contributor

saep commented Feb 26, 2023

Neovim version (nvim -v)

v0.8.3

Operating system/version

NixOS 22.11 / NixOS unstable with home-manager

How to reproduce the issue

Open a file with multiple tests (e.g. https://github.com/neovimhaskell/nvim-hs/blob/main/tests/EventSubscriptionSpec.hs).

Run all tests of a file with multiple tests:

require("neotest").run.run(vim.fn.expand("%"))

Expected behaviour

Tests execute without errors.

Actual behaviour

Some tests usually fail with a failure message such as this:

Build profile: -w ghc-9.0.2 -O1
In order, the following will be built (use -v for more details)
 - nvim-hs-2.3.2.2 (test:hspec) (first run)
ghc-pkg: cannot create: /home/saep/src/nvim-hs/dist-newstyle/bu
/hspec/package.conf.inplace already exists

The minimal config used to reproduce this issue.

Basically:

require('neotest').setup{
  adapters = require('neotest-haskell'),
}

I tried to use the minimal config from this repository with the lines above added, but it cannot find tests. I don't know why. The 2 lines above is the only neotest configuration I have.

nix-shell -p neovim

NVIM_DATA_MINIMAL=/tmp/minimal nvim --clean -u ~/src/neotest-haskell/tests/minimal.lua tests/EventSubscriptionSpec.hs

@saep saep added the bug Something isn't working label Feb 26, 2023
@saep
Copy link
Contributor Author

saep commented Feb 26, 2023

I think the error originates from spawning too many cabal test processes.

I've tried to fix this myself a couple months back, but I've gotten a bit frustrated with figuring out how to generate just one cabal test-command instead of one for each found test. Maybe I'll have a go at this again sometime in the future. I wouldn't mind some pointers to help me figure this out. ;-)

@mrcjkb
Copy link
Owner

mrcjkb commented Feb 26, 2023

Thanks for reporting this.

You're probably right about neotest spawning too many cabal processes.

I haven't used this on whole test files very often - I usually call neotest.run.run() with the cursor located at the node of the spec tree containing all the specs I want to run.

When running on the entire file, neotest will identify child and parent nodes as separate tests, leading to duplicate test runs of the child nodes.

I have also noticed some strange behaviour when the cursor position results in multiple nodes being identified, which indicates multiple processes are being started.

To fix this, we need a way to identify child nodes as such and remove them if their parent is in the list of tests to run.
This may require an upstream patch to neotest.

For running tests on whole files, it should be possible to use a different tree-sitter query for finding tests, based on whether or not a file was passed as an argument.
But this, too, might need a neotest patch.

I'm quite busy until next weekend, but maybe I'll be able to squeeze in some time tomorrow.

@saep
Copy link
Contributor Author

saep commented Feb 26, 2023

I'm quite busy until next weekend, but maybe I'll be able to squeeze in some time tomorrow.

Don't stress yourself. This isn't urgent. :-)

Since it might help, I share what I can still remember the last time I tried to fix this.

  1. The wording of the Collecting Results section of the README implies that neotest should be capable of running multiple tests within a single process. Therefore, I'm not sure that this is necessarily a problem upstream (although it might be) or simply requires a different implementation here.

  2. To fix this, we need a way to identify child nodes as such and remove them if their parent is in the list of tests to run.

    Hspec happily accepts arbitrarily many arguments.

    Here is an example for 2 Test Files:

    cabal test --test-show-details=streaming --test-options="--format=checks --match /Neovim.RPC.SocketReader/ --match /Neovim.RPC.Common/"

    So even if the children nodes were not squashed together with their parents, hspec would run the right tests. (I think the size of the command line has some limit, but squashing together the redundant arguments can be an optimization done later.)

@mrcjkb
Copy link
Owner

mrcjkb commented Feb 26, 2023

Thanks for the input.

neotest should be capable of running multiple tests within a single process.

The adapter interface and RunSpec type make me think otherwise.
It seems like build_spec is called repeatedly; once for each position that is found. But maybe this can be solved in discover_positions.

Hspec happily accepts arbitrarily many arguments.

This is probably the way to go if we can get the build_spec calls squashed into a single call.

@mrcjkb
Copy link
Owner

mrcjkb commented Feb 27, 2023

After having browsed the neotest sources a bit, I think the best way to solve this is a complete rework of how neotest-haskell parses positions and later constructs the test commands from the positions.

The current parse_positions implementation runs one big tree-sitter query that results in what is effectively a list of positions. And then build_spec runs queries on each position to build the hspec --match filters for each position.
This works well if there's only one position to run (the "nearest"), but not if there are more than one.

If I change parse_positions to run separate smaller tree-sitter queries and return a tree that is equivalent to the hspec tree (as far as is possible with the limited tree-sitter capabilities), neotest should only pass the respective parent nodes to build_spec.
For the entire file, this would likely be a single position, unless the Spec is constructed using multiple top-level functions.
Then I can construct the hspec --match filter in build_spec by simply iterating over each position's parent nodes.

It would still be nice to be able to merge multiple positions into a single command, but this will definitely need a patch to neotest.

@mrcjkb
Copy link
Owner

mrcjkb commented Feb 27, 2023

@saep I have implemented a fix and created a separate issue #49 for improving the logic to parse positions (which will need more time).

The fix in #48 attempts to find the top-level describe node and runs that.
This might not be completely fool-proof, since it doesn't work if a single module has multiple top-level specs.

I'll have to take some extra time to think about how to get that working. Adding a --match for each test could be an option, but as you mentioned, it could exceed the maximum command line size in large test files.
Although it should be okay if I limit it to describes that don't have a parent describe.

When #44 is merged, I will prepare a new release 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants