-
Notifications
You must be signed in to change notification settings - Fork 30
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
Parameterized Tests #57
base: main
Are you sure you want to change the base?
Conversation
Main reason I went with this approach instead of the custom parsing of |
I looked into this again and added the first approach (i.e. calling
I think it make sense to let the user choose a "strategy" of test case discovery. I included an option now in the setup args or WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really impressive! Unfortunately I'm leaving for a vacation in a couple days so I don't have a ton of time to review at the moment. To make reviewing easier when I get a chance, can you fix the existing tests and add new ones?
Thanks =) Sure, I'll have a look at the tests in the next days. |
ba0aa02
to
394bdd0
Compare
Hi @rouge8 I added tests for the new functionality. I also had to install |
Wow, thanks! I'm currently on vacation so it might take me a while to get to this, but I'm excited! |
What's the latest on this? I started implementing this myself before noticing this PR. Is the plan still to merge this eventually? |
Yes, I plan to review it. Unfortunately I don’t write much Rust these days so it’s not super pressing… |
Fair enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting an error with the cargo
strategy when in one of my projects:
|| Error executing luv callback:
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:95: Async task failed without callback: The coroutine failed with this message:
|| ...ndy/projects/neotest-rust/lua/neotest-rust/discovery.lua:317: bad argument #1 to 'ipairs' (table expected, got nil)
|| stack traceback:
|| [C]: in function 'ipairs'
|| ...ndy/projects/neotest-rust/lua/neotest-rust/discovery.lua:317: in function 'cargo'
|| /Users/andy/projects/neotest-rust/lua/neotest-rust/init.lua:330: in function 'discover_positions'
|| /Users/andy/projects/neotest-rust/lua/neotest-rust/init.lua:91: in function 'is_test_file'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:348: in function '_get_adapter'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:205: in function 'get_position'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:152: in function 'get_nearest'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:485: in function <...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:484>
|| stack traceback:
|| [C]: in function 'error'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:95: in function 'close_task'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:117: in function 'cb'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:181: in function 'waiter'
|| ...dy/.local/share/nvim/plugged/neotest/lua/nio/control.lua:82: in function 'wake'
|| ...dy/.local/share/nvim/plugged/neotest/lua/nio/control.lua:92: in function 'set'
|| ...re/nvim/plugged/neotest/lua/neotest/lib/process/init.lua:69: in function <...re/nvim/plugged/neotest/lua/neotest/lib/process/init.lua:64>
|| Error executing luv callback:
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:95: Async task failed without callback: The coroutine failed with this message:
|| ...ndy/projects/neotest-rust/lua/neotest-rust/discovery.lua:317: bad argument #1 to 'ipairs' (table expected, got nil)
|| stack traceback:
|| [C]: in function 'ipairs'
|| ...ndy/projects/neotest-rust/lua/neotest-rust/discovery.lua:317: in function 'cargo'
|| /Users/andy/projects/neotest-rust/lua/neotest-rust/init.lua:330: in function 'discover_positions'
|| /Users/andy/projects/neotest-rust/lua/neotest-rust/init.lua:91: in function 'is_test_file'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:348: in function 'get_adapter'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:358: in function '_set_focused_file'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:497: in function '_start'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:188: in function '_ensure_started'
|| ...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:200: in function <...l/share/nvim/plugged/neotest/lua/neotest/client/init.lua:199>
|| ...share/nvim/plugged/neotest/lua/neotest/consumers/run.lua:38: in function 'get_tree_from_args'
|| ...share/nvim/plugged/neotest/lua/neotest/consumers/run.lua:68: in function 'func'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:168: in function <...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:167>
|| stack traceback:
|| [C]: in function 'error'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:95: in function 'close_task'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:117: in function 'cb'
|| ...andy/.local/share/nvim/plugged/neotest/lua/nio/tasks.lua:181: in function 'waiter'
|| ...dy/.local/share/nvim/plugged/neotest/lua/nio/control.lua:82: in function 'wake'
|| ...dy/.local/share/nvim/plugged/neotest/lua/nio/control.lua:92: in function 'set'
|| ...re/nvim/plugged/neotest/lua/neotest/lib/process/init.lua:69: in function <...re/nvim/plugged/neotest/lua/neotest/lib/process/init.lua:64>
Hmm, can you give some more info please?
|
-- Neotest
local neotest = require("neotest")
neotest.setup({
discovery = { enabled = true },
adapters = {
require("neotest-python")({
args = { "-v" },
}),
require("neotest-rust")({}),
require("neotest-plenary")({}),
},
})
local bufopts = { noremap = true, silent = true }
-- Run the nearest test
vim.keymap.set("n", "<leader>t", neotest.run.run, bufopts)
-- Run all tests in the file
vim.keymap.set("n", "<leader>T", function()
neotest.run.run(vim.fn.expand("%"))
end, bufopts)
-- View the test output
vim.keymap.set("n", "<leader>to", neotest.output.open, bufopts)
-- View the test summary
vim.keymap.set("n", "<leader>ts", neotest.summary.open, bufopts)
{"rust-build-meta":{"target-directory":"/Users/andy/tmp/rust-builds","base-output-directories":["debug"],"non-test-binaries":{},"build-script-out-dirs":{},"linked-paths":["debug/build/zstd-sys-f82c3314ab16f949/out"],"target-platforms":[{"triple":"aarch64-apple-darwin","target-features":["aes","crc","dit","dotprod","dpb","dpb2","fcma","fhm","flagm","fp16","frintts","jsconv","lor","lse","neon","paca","pacg","pan","pmuv3","ras","rcpc","rcpc2","rdm","sb","sha2","sha3","ssbs","vh"]}],"target-platform":null},"test-count":29,"rust-suites":{"wanikani-apprentice::bin/wanikani-apprentice":{"package-name":"wanikani-apprentice","binary-id":"wanikani-apprentice::bin/wanikani-apprentice","binary-name":"wanikani-apprentice","package-id":"wanikani-apprentice 0.1.0 (path+file:///Users/andy/Dropbox/Projects/wanikani-apprentice)","kind":"bin","binary-path":"/Users/andy/tmp/rust-builds/debug/deps/wanikani_apprentice-048373458715c33b","build-platform":"target","cwd":"/Users/andy/Dropbox/Projects/wanikani-apprentice","status":"listed","testcases":{"tests::assignments::logged_out_redirect":{"ignored":false,"filter-match":{"status":"matches"}},"tests::index::logged_in":{"ignored":false,"filter-match":{"status":"matches"}},"tests::index::logged_out":{"ignored":false,"filter-match":{"status":"matches"}},"tests::lb_heartbeat::ignores_host_header":{"ignored":false,"filter-match":{"status":"matches"}},"tests::lb_heartbeat::ok":{"ignored":false,"filter-match":{"status":"matches"}},"tests::login::already_logged_in":{"ignored":false,"filter-match":{"status":"matches"}},"tests::login::invalid_api_key":{"ignored":false,"filter-match":{"status":"matches"}},"tests::login::valid_api_key":{"ignored":false,"filter-match":{"status":"matches"}},"tests::logout":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_500":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_1":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_2":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_3":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_4":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_5":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_display_time_remaining::case_6":{"ignored":false,"filter-match":{"status":"matches"}},"tests::test_radical_svg":{"ignored":false,"filter-match":{"status":"matches"}},"tests::trusted_host_header":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_assignments":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_assignments_unknown_subject::case_1":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_assignments_unknown_subject::case_2":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_assignments_unknown_subject::case_3":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_assignments_unknown_subject::case_4":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_kana_vocabulary":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_kanji":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_radicals":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_radicals_with_character_images":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_username":{"ignored":false,"filter-match":{"status":"matches"}},"wanikani::tests::test_vocabulary":{"ignored":false,"filter-match":{"status":"matches"}}}}}} |
Interesting, its seems like the test package name that M._binary_path() spits out is somehow unknown to cargo nextest, i.e. it is not part of Can you reproduce the issue, with any of these packages? |
I can't reproduce it with the test package unfortunately. I decided to open source the repo I was testing with: https://github.com/rouge8/wanikani-apprentice/blob/main/src/main.rs#L793 |
Imaginge you have the following tests: ```rust #[test] fn foo_bar() {} #[test_case(1 ; "one")] #[test_case(2 ; "two")] fn foo() {} ``` Selecting `foo` in the summary and running it would also run `foo_bar` since we only match the _prefix_ of the test. To exactly match this test we have to include "end of line" ($) char in the match, but optionally also match `::.*` so we include all subtests as well
This is needed for the new `param_discovery_mode="cargo"` in the tests
for more information, see https://pre-commit.ci
Hey @rouge8, thanks for sharing your project. You were onto something, it actually was a problem with the |
Now I'm getting "No tests found" in that file and a bunch of errors when I try to run the tests in https://github.com/rouge8/wanikani-apprentice/blob/main/src/wanikani.rs. It also looks like CI is broken... |
Cargo nextest will identify the tests of translation unit `src/main.rs` as `<package>::bin/<package>`, not as unit tests.
for more information, see https://pre-commit.ci
Okay took me a bit to figure out that there was a regression with treesitter between nvim 0.9.0 & nightly. Refactored the treesitter logic slightly to also work for 0.9.0. This also fixes the CI again. Can you please retry and see if it works now for you? Regarding the "No tests found" I sometimes experience the same behavior, but this must be sth. inside |
I'm still seeing errors in https://github.com/rouge8/wanikani-apprentice/blob/main/src/wanikani.rs:
And getting "No tests found" with Cargo in https://github.com/rouge8/wanikani-apprentice/blob/main/src/main.rs#L793 Would you be open to removing the Cargo test discovery from this PR and re-opening that in a separate PR once this is merged? I still need to make sure I understand the code, but the Treesitter discovery seems much more reliable and I'd feel comfortable merging that. |
Hmm, I tried in an Ubuntu 22:04 docker container, with nvim 0.9.0 (the same as in the CI) starting like:
In theory yes, but both strategies are rather intermixed now (especially in the README). Would be alot of effort for me TBH... We could mark the "cargo" strategy "experimental" in the README maybe? The default is "none" anyway right now, so this the whole feature would be opt-in anyway. Then we could get feedback from users, see how it sticks.
The treesitter approach is actually more complicated 😅 With the cargo strategy I just call neotest-rust/lua/neotest-rust/discovery.lua Lines 292 to 295 in 8cc3af5
...interprete the resulting output... neotest-rust/lua/neotest-rust/discovery.lua Line 304 in 8cc3af5
...transform some special syntax for neotest-rust/lua/neotest-rust/discovery.lua Lines 137 to 148 in 8cc3af5
... and finally just go through all the (parent) tests detected in the first pass (in neotest-rust/lua/neotest-rust/discovery.lua Lines 320 to 342 in 8cc3af5
|
With |
??? |
Can you create a Dockerfile that installs the appropriate nvim, cargo nextest, etc.? If you do I'll try that.
I'm not convinced this is inside Neotest because it happens for me consistently with the cargo discovery but not the treesitter discovery. I have the neotest log output from trying to use cargo discovery on https://github.com/rouge8/wanikani-apprentice/blob/main/src/main.rs#L793:
|
I did some more testing... The |
Sorry @rouge8 for the late reply, was busy with work. I quickly threw together a Dockerfile: FROM ubuntu:22.04
RUN apt-get update && apt-get -y install git build-essential curl libssl-dev pkg-config && rm -rf /var/lib/apt/lists/*
# Neovim
RUN mkdir -p _neovim
RUN curl -sL https://github.com/neovim/neovim/releases/download/v0.9.5/nvim-linux64.tar.gz | tar xzf - --strip-components=1 -C "${PWD}/_neovim"
RUN git clone https://github.com/LazyVim/starter ~/.config/nvim
ENV PATH="${PWD}/_neovim/bin:${PATH}"
# Rust
RUN curl https://sh.rustup.rs -sSf | bash -s -- -y
ENV PATH="/root/.cargo/bin:${PATH}"
RUN cargo install cargo-nextest
# Treesitter
RUN echo 'return { {"gollth/neotest-rust", branch = "param"}, { "nvim-neotest/neotest", config = function() require("neotest").setup({ adapters = { require("neotest-rust")({ dap_adapter = "codelldb", parameterized_test_discovery = "cargo", }), } }) end } }' > ~/.config/nvim/lua/plugins/neotest.lua
RUN nvim --headless -c "TSInstallSync rust | quit"
# Project
RUN git clone https://github.com/rouge8/wanikani-apprentice.git ~/wanikani-apprentice
RUN cargo nextest --manifest-path ~/wanikani-apprentice/Cargo.toml list
CMD nvim When you run it it opens nvim with the necessary config and test strategy already set to |
@rouge8 were you able to sort out your config? |
As a |
Hey @NobbZ , yeah that was my original intention behind this PR as well (= I don't know what is left from the Authors perspective regarding this. Personally I switched to https://github.com/mrcjkb/rustaceanvim, which comes with a pretty good Neotest integration as well. Maybe that is something for you as well |
I have seen that it is listed as supported, but sadly was unable to find documentation about how to integrate rustacean, also I fear that it's existence will interfere with |
This was the main blocker the last time I looked at this PR. I'm not willing to merge something that I can't get to work! Now there's also #79... |
Sorry to hear that you can’t get your config sorted out @rouge8 :/ But in the meantime people can try rustaceanvim, until there is a solution |
This is necessary for tests using the `cargo` discovery strategy since they have to build the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I've fixed the test runner, I tried looking at this PR again.
-
Cargo discovery works for me without crashing!
-
Treesitter discovery works, but it doesn't seem to actually run the tests successfully. For example, I opened the test summary on https://github.com/rouge8/wanikani-apprentice/blob/d11c844fd4426bc8debb87744dedd10631c550bc/src/main.rs#L793-L809 and hit
r
on the various cases to run them, and they were all marked as failed and "Test didn't produce any output" is displayed in nvim.
@gollth thoughts?
Hi @rouge8
first thanks for this adapter for the great neotest! I'm using it quite intensively. In our projects though, we make heavy use of parameterized tests, so I'm hoping for a solution to #1
After some research how other adapters handle this (e.g. neotest-jest & neotest-python) I tried my luck and implemented a possible way. According to the disucssion in Capture and interpolate dynamic tests there are two preferred ways of how to discover nested/parameterized tests:
cargo nextest list
in our case) and somehow relate the output back to positions in the test tree. This is howneotest-jest
andneotest-python
are doing it.I tried now the first approach and got quite some milage out of it:
test-case
rstest
This is my first ever Lua development, so there is probably a ton wrong with my style. But I wanted to discuss with you first if this approach generally makes sense. Let me know what you think
Discussion
Current Idea:
neotest
internally will place subtests only into their parents if theirrange
is a subset of their parentsrange
, which is impossible if the nested test's range is above the actual function.neotest-python
was doing, i.e. a two pass query:parameterized
if the test is parameterized at all (i.e. has an#[test_case(...)]
or a#[rstest] #[case(...)]
macro before it.parameterized
test, shoot another tree sitter query, which detects each test case (potentially with its name). For each case, add a new subtest below the parent.async
tests as well with both#[tokio::test]
and#[async_std::test]
runnersThere are many edge cases though, which are hard (impossible?) to overcome with plain treesitter:
test-case
supports two "modes": named and unnamed. The named one (e.g.#[test_case(some arg ; "test_name")]
is relatively easy to parse in tree sitter. However when you don't have a unique case name,test_case
builds the name out of the parameters. Also there is no way to distinguish the two cases because in tree sitter at this level everything is only a(token_tree)
rstest
value lists .... yeah... this one is straight impossible to parse with tree-sitter alone...