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

vstest integration #124

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Conversation

Nsidorenco
Copy link

@Nsidorenco Nsidorenco commented Nov 10, 2024

Implementing the queries for F# hit so many edge-cases with the different test running and their naming of test that it became easier to just implement the ideas of #90.

Rough draft of a vsconsole-based implementation, where a vstest instance is responsible for test discovery and running tests. Seems to work pretty well and reasonably snappy.

The test discovery logic should probably be cached in some way to avoid discovering test cases for the same dll for every source file.

Still a bit of work to be done as well; for now, the path to both the vstest dll and the test project dlls are (relatively) hard coded and overall test are entirely missing.

Will eventually close #90 and close #82

@Nsidorenco Nsidorenco force-pushed the test-runner branch 2 times, most recently from 643e37d to ea8750e Compare November 13, 2024 18:34
@Issafalcon
Copy link
Owner

Hi @Nsidorenco - Great work on this so far! I'm very limited on free time at the moment (essentially I have none!) so this is a big help towards improving this adapter. When I get chance, I'll take a good look at the changes, but at first glance, it looks like it's going in the right direction.

@Nsidorenco
Copy link
Author

@Issafalcon just take your time 😄 Outside of testing it is relatively feature complete now and should be ready for a review

@Nsidorenco Nsidorenco force-pushed the test-runner branch 2 times, most recently from e6cb2fb to d619ab1 Compare November 25, 2024 20:28
@waynebowie99
Copy link

I'm willing to help out with this. I went down a similar rabbit hole with my test project being too large causing me to create this PR #120

I swapped over to your branch and I'm getting the following error.

image

Seems to be due to this line here. https://github.com/Nsidorenco/neotest-dotnet/blob/6a1329bf0f5bdcb2b897645c5c804448a94817e2/lua/neotest-dotnet/vstest_wrapper.lua#L84C5-L84C70

When I get rid of some of those debug lines on my end, the test seem to attempt to load based on the debug messages.

It seems like it's failing to parse and keeps retrying? It's a little unclear.

I'm using C# on Windows 11

Nsidorenco and others added 4 commits December 4, 2024 17:02
* improve position detection

* add tests

* update workflow

* add treesitter cli to workflow

* bump lower bound nvim version

* test on all os

* set dotnet version
@Nsidorenco
Copy link
Author

Hi @waynebowie99,
Thank you for helping look into this! I don't use windows so this is a big help.

The first step of the adapter is that it tries to locate the run_tests.fsx file in the runtime path. In your case it fails to find the file on the system and therefore are unable to detect any tests.

I have just pushed a commit to better handle file separators on windows, maybe this fixes it for you?

Curiously, the CI for windows does not seem to have an issue detecting the file

@waynebowie99
Copy link

I just gave it a go and it looks like I'm still getting the same error. I can do some debugging or provide some logs that would be helpful

image

@Nsidorenco
Copy link
Author

Could you try to run vim.api.nvim_get_runtime_file("scripts", true) and check if neotest-dotnet is path of the results.

If so, can you also check what vim.api.nvim_get_runtime_file("scripts/run_tests.fsx", true) returns?

@waynebowie99
Copy link

neotest-dotnet is in vim.api.nvim_get_runtime_file("scripts", true)

This vim.api.nvim_get_runtime_file("scripts/run_tests.fsx", true) also exists too

image

@Nsidorenco
Copy link
Author

@waynebowie99 could you try the latest commit again?

@waynebowie99
Copy link

New error!!

image

@Nsidorenco
Copy link
Author

That's progress!

Can you check theneotest logs? It's a file located in the same directory as :LspLog.
It should mention something about "detecting sdk path"

@Nsidorenco
Copy link
Author

Alternatively, try the latest commit again - vstest.console.dll might not exists in a windows installation of dotnet

@waynebowie99
Copy link

waynebowie99 commented Dec 4, 2024

vstest.console.dll exists on windows and there is no vstest.console.exe. It is saying the detected sdk path is nil in the logs though

This is my dotnet sdk path that contains vstest.console.dll: C:\Program Files\dotnet\sdk\8.0.404

@waynebowie99
Copy link

I manually set the dotnet_sdk path and it was able to find vstest.console.dll but then errored on this
image

The last log was Waiting for result file to populated...

@Nsidorenco
Copy link
Author

Seems like something goes wrong when it tries to read the content of the file containing the discovered tests.

I just pushed a commit that changes the code a little.

If that does not work look for the following lines in the log:

DEBUG | 2024-12-04T21:30:03Z+0100 | ...ode/neotest-dotnet/lua/neotest-dotnet/vstest_wrapper.lua:302 | Discovering tests using:
DEBUG | 2024-12-04T21:30:03Z+0100 | ...ode/neotest-dotnet/lua/neotest-dotnet/vstest_wrapper.lua:303 | discover file_1 file_2  ...

file_1 and file_2 should be files located on your system, which contains the discovered tests.

@Issafalcon
Copy link
Owner

Hi @Nsidorenco - I'm just catching up with PR's on this plugin and took the liberty to run the checks before and after resolving merge conflicts on this branch.

I also tried your branch to see the feature parity with how the current plugin adapter works and noticed that a few things are missing. Take this single sample file for parameterized tests in C#:

image

Then compare with how parameterized tests used to look on main:

image

Just to point a few things out:

  • Parameterized tests have their own unique range in the neotest tree, so they accurately reflect each test case (this is tricky to replicate, but if there could be a slightly different range for each nested test so they have their own location in the test file, that would reach parity with how it works currently)
  • The temp test file output seems to not exist when checking the output of the test in the summary panel
  • The parameterized tests don't show up in the summary pane as individual tests as they did before.

I'm going to run checks on a few other test case files with different variations to check other functionality later, but for now, it's something to look into maybe?

Also, not to step on toes, but I have managed to implement a quick POC of https://github.com/microsoft/vstest/blob/main/docs/RFCs/0007-Editors-API-Specification.md, using vim.uv to allow the vstest process to communicate over sockets with the neovim process, and send JSON messages over the same socket connection to the test runner, with a good degree of success. I'm planning on creating a new plugin as a sort of API / interface to the vstest platform for general consumtption.

While I don't expect this to be anywhere near finished any time soon, I forsee that it could be a very useful interface for this adapter and any other plugins that want to make use of it, so I'll keep creating this editor interface for it, which will allow tighter control of the test flow than the single TestPlatformWrapper scripts. However, it shouldn't change what yoiu're doing currently. I would just replace the scripts with calls to that API in the future.

@Nsidorenco
Copy link
Author

Hi @Issafalcon, Thanks for giving this a look!

  • Parameterized tests have their own unique range in the neotest tree, so they accurately reflect each test case (this is tricky to replicate, but if there could be a slightly different range for each nested test so they have their own location in the test file, that would reach parity with how it works currently)

Good point, having the parameterized test cases un-nested is probably the more intuitive way to represent them.
To avoid having the manipulate the tree sitter ranges I ended up building the neotest tree manually to control the nesting of parameterized test cases.

  • The temp test file output seems to not exist when checking the output of the test in the summary panel
  • The parameterized tests don't show up in the summary pane as individual tests as they did before.

I think file waiting logic using tail got broken at some point. But I also think this was a bit of a hack to make neotest wait for the vstest process using a systems command. To make it more reliable I've changed it to use a custom strategy avoid all the shell interactions.

Also, not to step on toes, but I have managed to implement a quick POC of https://github.com/microsoft/vstest/blob/main/docs/RFCs/0007-Editors-API-Specification.md, using vim.uv to allow the vstest process to communicate over sockets with the neovim process, and send JSON messages over the same socket connection to the test runner, with a good degree of success. I'm planning on creating a new plugin as a sort of API / interface to the vstest platform for general consumtption.

I think this sounds like a great idea! I believe it should fit pretty well into the existing implementation, where we can just replace the file-based communication with the socket-based one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate with vstest Test Platform Protocols to enhance performance and consolidate code Add support for F#
3 participants