TreeView support for NUnit and MSTest - getting FQ test names

[jmansar]

Problem

The root cause of tree view not working for NUnit and MSTest is the fact that dotnet test -t command, that is called to discover tests in the solution doesn't return fully qualified names of tests (with namespace and class). It's coded to use DisplayName of a test that is provided by a test adapter.
XUnit test adapter returns FQ name as DisplayName and both NUnit and MSTest test adapters return just a method name. That's why it works for XUnit.

Solution

This PR adds the fallback mechanism for discovering tests. If dotnet test output doesn't include FQ names, the code runs dotnet vstest with /ListFullyQualifiedTests flag which always returns FQ names.

Drawbacks

The drawback of this approach is that it requires to execute both dotnet test and dotnet vstest for NUnit and MSTest. dotnet test cannot be simple swapped for dotnet vstest as dotnet vstest is just a wrapper around vstest.console which runs on the built assemblies. dotnet test runs on csproj/sln and also restores packages and performs a build (if --no-restore, --no-build flags are not specified).
So currently, the code parses the dotnet test output to get the list of assembly paths which are then passed to dotnet vstest.

[stefanforsberg]

Thanks for the PR! We've been talking about how to support mstest/nunit and we were tinkering with the idea of running the tests and using the resulting trx result file to generate the tree (since all of the test frameworks contain the FQN in there). But this seems like a better approach.

(I'll fix the conflict in test/index.ts in the main repo soonish)

[stefanforsberg]

@jmansar I couldn't get it to work at first. Turns out the sdk version I was using (2.0.3) didn't have the switch ListFullyQualifiedTests. I'm not sure at which version that became available but it would be nice to give the user some sort of clue as to why it didn't work. Also perhaps to fallback to the current behavior if the command is not available. Thoughts?

[jmansar]

Right. I'll find out what is the lowest version of sdk that supports it. It's quite well-hidden feature, not documented anywhere.
I believe that's the PR which adds it to vstest: Vstest PR1075

And yes, it makes sense to have a fallback. I guess I can just return test names from dotnet test in that case. I'll make the changes when I have some time.

Do you have some standard way of logging error / warnings? I haven't seen anything relevant in the vscode-dotnet-test-explorer code. Ideally, I'd log a warning if dotnet sdk version is too low.

[stefanforsberg]

There isn't really a standard way of logging errors / warnings as of now. The only place I think we show something to the user is when tree can't be populated

or when the go to test function can't figure out where to navigate too

.

Maybe alternative 2 would make more sense in this case (it's using the built in message box) (

vscode-dotnet-test-explorer/src/gotoTest.ts

Line 33 in 2b56eca

vscode.window.showWarningMessage(r.message);

)

When it comes to what the message should say my thoughts were that that although it would not perhaps make super sense to the user it would help us determine where things went wrong. In this case a message about a sdk version would be pretty self explanatory though.

@janaka @formulahendry Any other input?

[janaka]

I've only just skimmed so far.

Re: error pop up
Is alternative two the new notifications UI in VS Code that pops up from the right bottom corner?

[jmansar]

So, the lowest version of SDK that has /ListFullyQualifiedTests is 2.1.2

[teamdynamiq]

@janaka Yep! (I'm not sure if it's new, I think i'ts been around but it now opens in the lower right corner instead of top center... think it might depend on your vscode version)

[jmansar]

IMO in addition to this user friendly warning popup, it would be nice to have a proper diagnostic log written to the output channel similar to:

As currently some of the errors are swallowed. But, that's another issue.

[stefanforsberg]

@jmansar Yes, that sounds like a great idea. I'll start working on it now.

[stefanforsberg]

Output logger is added now

https://github.com/formulahendry/vscode-dotnet-test-explorer/blob/master/src/extension.ts#L22

[jmansar]

@stefanforsberg
I've added error logging and the warning message when dotnet sdk version is too low. I thought that this warning popup might be annoying for users that for some reason cannot / don't want to update the dotnet sdk. So, I made it possible to suppress this message by clicking "Don't show again", which updates the relevant section in the user settings.

[formulahendry]

Would better to use https://code.visualstudio.com/docs/extensionAPI/vscode-api#Memento api (refer to https://github.com/formulahendry/vscode-mysql/blob/177396b240b9760963e7447f84becf4bd1d891b0/src/mysqlTreeDataProvider.ts#L74) Since this kind of config is designed for user to update in User Setting, while in our case, we do not want to expose it to end user.

[jmansar]

Done. Though, I don't think there is an easy way to reset these now.

[formulahendry]

Sorry to come here late. The overall approach looks good to me! Thanks @jmansar ! One little concern is that whether it would take longer time to discover the tests? Since we may run dotnet test and then dotnet vstest. Or the dotnet vstest will take very shot time?

[stefanforsberg]

On my machine running the additional vstest command adds around 1.5 seconds to the total discovery time (which is around 5 seconds). So yes, in case of mstest and nunit (or xunit with DisplayName as in #56) it will take slightly longer to discover the tests.

I wonder it it would be viable to skip the dotnet test command and go straight for the dotnet vstest one?

[jmansar]

Regarding skipping dotnet test. To run vstest you need to pass the list of assemblies containing tests. How do we get these? Running dotnet test and parsing the stdout was one option. You could get these from dotnet build output as well. You could probably also construct paths to assemblies in the code based on TargetFramework and AssemblyName of the projects, but that's a bit shaky IMO.

This information could be cached, but that would only make a difference to users which have dotnet-test-explorer.build set to false (which is true by default). So, the extension will still be running either build or test to build the solution / project in most of the cases.

[stefanforsberg]

If we lack a reliable way to trigger the vstest command on a "post build" type of event and had to construct something on our own (and I can think of many scenarios just at the top of my head that would make that task a bit hairy) then the current model of first triggering dotnet test and then vstest sounds like the best one.

@formulahendry How do you feel about merging this?

[formulahendry]

Agree! Current solution seems the best.