Better Test Discovery

[Issafalcon]

👋 Hello!

Please use the discussion below to add any ideas to improve the test discovery / iron out any bugs with the Test Discovery mechanism of the adapter:

• Neotest core improvements should be discussed in that repo
• Treesitter is the mechanism used. Omnisharp LSP requests were tried originally, but had too many issues

[Issafalcon]

Linking in #38 as one approach that was discussed via a PR for reference

[refractalize]

Hi @Issafalcon, I'm running into some issues with parameterized tests with NUnit (although I suspect the issues apply to all test frameworks). In particular, it has to do with the mismatch between the parameters as parsed by tree-sittter, and the parameters as returned in the results file - I'm sure you're familiar with this issue already! I'll give you an example, this code:

[TestCase(1, 1)]
[TestCase(2, 1)]
public void ParamTest(int a, int b)
{
    Assert.AreEqual(a, b);
}

One of these tests will pass, the other will fail, but neotest shows both of them as having passed. (I've spotted similar issues where both of them show as having failed, probably due to some previous state.) This is to the extent that I don't trust what I'm see in Vim and head back to the command line to see what's really going on with my tests.

Let's have a look at the results file:

<TestMethod codeBase=".../A.Test/bin/Debug/net7.0/A.Test.dll" adapterTypeName="executor://nunit3testexecutor/" className="A" name="ParamTest(2,1)" />
...
<TestMethod codeBase=".../A.Test/bin/Debug/net7.0/A.Test.dll" adapterTypeName="executor://nunit3testexecutor/" className="A" name="ParamTest(1,1)" />

You'll have noticed that there is a space in the parameters in the code (TestCase(1, 1)), but no space in the results name attribute (ParamTest(1,1)). This seems to be significant - and indeed if I remove the space between the arguments from the source code everything seems to work correctly. Another similar issue is with boolean values which are written in code as true and false, yet come through the results file as True and False. The problems are compounded if we have more complex expressions such as TestCase(new [] {1, 2, 3}) which becomes ParamTest([1, 2, 3]).

Some of these issues could be fixed by simply normalising the whitespace, or normalising the case on boolean values, but the more complex expressions (arrays) seem to be intractable, or at least hard enough to make you think if this is the right approach.

All this leads me to think that short of trying a very different approach altogether (for e.g. not using tree-sitter) that we should keep it simple and treat parameterized tests as a single test, that is, it has a single entry in the test list and a single result. Looking at the output would show output for all parameterised test runs, and should be enough to understand which of them had failed. I know this isn't ideal, but I'd prefer that it was limited in this way but reliable and understandable, than it supporting parameterized tests but only in very specific ways, and being broken and misleading in other cases. Indeed it's not obvious which cases are which until you start digging around in the code...

What are your thoughts, then, to removing support for parameterized tests? Or at least making support for them optional?

BTW otherwise I'm a very happy user of your plugin, especially the support for debugging with dap!

[refractalize]

Another approach that might help here is to use the following command to discover parameterized test names:

dotnet test -t -- NUnit.DisplayName=FullName

(Obviously NUnit specific but bare with me...)

So the following code:

[TestCase(1, 1)]
[TestCase(2, 1)]
public void ParamTest(int a, int b)
{
    Assert.AreEqual(a, b);
}

Will produce the following output:

The following Tests are available:
    Test.ParamTest(1,1)
    Test.ParamTest(2,1)

This returns the parameterised test names in what seems to be lexical order, so it's enough (perhaps) to match the lexical index of the TestCase() attribute from tree-sitter against it's corresponding index in the test name output to get it's canonical name.

Thoughts?

[Issafalcon]

Hi @refractalize - I'm glad you like the adapter!

I appreciate you taking the time to analyze this, and if I'm honest, I had overlooked the simple case of normalizing the inline parameters, so thanks for raising that point.

With regards to your other, more generalized point about parameterized tests - I have had the same thoughts, and I in fact did try to scrap using tree-sitter altogether and rely on the dotnet test -t command to discover the tests. From memory, this was why I didn't end up going with this approach:

1. The main reason was because, while it might seem fine with a few simple tests, with larger numbers, and also multiple test files, this command will bring back all the tests in the project, in non-specific order. This gets very messy when you then need to parse the output, and have tests that have the same names / similar names. Not to mention, each test frameworks runners all behave in slightly different ways. I believe it's either xUnit or MSTest that just plain ignores any sort of full qualification on the test name. So you might have a test with the same name in another file, and you would have duplicate lines with no possible way to tell which one was in which file.
2. It ended up being a real pain to actually parse this data, and get it to play nicely with neotest base, because the plugin is geared around treesitter outputs and positioning (which is also, again, something that the dotnet test method doesn't give you)

So really, point 1 made it a non-starter, or at least for me it did. The most robust solution for this would be to have some sort of hybrid approach to account for all cases where tree-sitter wouldn't work (anything requiring compilation to determine the parameters), that would then try to fish out the parameters from the compiled test list somehow. Given my limited free time, this isn't a solution I was willing to try to figure out.

Finally, I think you make a good case for simplifying parameterized tests. It's something I almost went with, but thought that having at least some parameterized tests with accurate indicators would be better than none. In hindsight, the confusion this has caused is probably a good reason to reconsider.

[refractalize]

Hi @Issafalcon,

I noticed that the types of things we can pass to c# attributes are quite limited, see here: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/attributes#2224-attribute-parameter-types. Which makes me hopeful that there are only a few different cases we need to cover to normalise the source code into what we get back from the test results, allowing us to support parameterised tests properly. Sorry I haven't been able to spend more time on this!

[Issafalcon]

Hi @refractalize - I haven't forgotten about this discussion (just FYI!). I'm currently planning out a way to make test discovery more robust using the combination of treesitter and the output from dotnet test -t. I think I have a way of doing it but only for xUnit and NUnit.

xUnit will allow dontnet test -t to output a list of fully qualified tests, which is great.

NUnit has https://docs.nunit.org/articles/vs-test-adapter/Tips-And-Tricks.html, which can also be used to leverage a .runsettings file to qualify the test names.

MSTest, however, is a lost cause when it comes to trying to extract the fully qualified test names.

Overall, it means that for xUnit and NUnit at least, parameterized test discovery can be made more robust by using dotnet test -t and treesitter. MStest will have to fall back to the limited test discovery mechanism.

Once time permits, I'll start working on this.