[WIP] Create a custom Logger to gather the data from the test run

[GeorchW]

See the proposal in #269 .

TODO:

• Add Data Collector project
• Add Data Collector arguments to command lines
• Add TCP listener code
• Send test results via TCP
• Make use of the received data
• Send stack traces / error messages
• See how --list-tests can be replaced
• Remove TRX parsing code
• Synchronize outcome enum
• Clean up TestResults API
• Make use of discovery events
• Update tree properly
• Make sure that watch works
• Make sure that the tree updates correctly when new tests are added on the fly
• Remove tests that don't have results after watching
• Fix go to
• Fix NUnit's DisplayNames
• Fix problems being reset
• Make XUnit/NUnit issue configurable
• Fix tests
• Make Travis work

[GeorchW]

This is coming along nicely. I'm currently refactoring the things I stumble upon, e.g. the Executor code, that's why the changes are getting bigger than required.

I've had to switch from .NET Standard 2.0 to .NET core 3.0 though, since the former does not have a JSON formatter and I got into DLL hell while trying to make Newtonsoft.Json work - vstest seems to screw around with the assembly loading logic. Maybe I'll try again tomorrow, or I'll file an issue with the vstest people.

If that doesn't work out: Are we trying to remain compatibility to the .NET Core 2.something SDK? This would still enable people who need their code to be targeting netcoreapp2.something to do so, just the SDK needs to be upgraded. Otherwise, we might want to keep the TRX parsing as a fallback for a while for the old users, before v2 becomes unsupported by MS anyway (August 2021). My impression is that VSCode users are usually quite willing to upgrade regularly. @stefanforsberg

[stefanforsberg]

Following this with great interest!

Does that data collector also have events for test discovery?

In general I would say that this change is so big that it might warrant a 1.0 release version. If it for various reasons is not compatible with netcore less than 3 we can always point people to a previous version of the extension (which the can choose to install in the vscode UI nowadays). It might not be ideal but I feel that it's hard enough to keep up with one moving target, having to keep track of the trx-side of things as well seems like a bit much too chew on imho.

On that note, .net 5 is around the corner. Will we end up in similar territory there? I'm just slightly scared of ending up with different versions of the datacollector that we need to keep up to date as well as figure out which to use depending on the environment the extension is run in.

Hope I'm not sounding too negative here, just trying to get as much information as possible 👍

[GeorchW]

Following this with great interest!

That's nice to hear!

Does that data collector also have events for test discovery?

Wondered so too. Haven't seen them yet :-/ I know, right now we're dependent on the human-readable output of dotnet test, but it seems like the DataCollector is not loaded when using --list-tests. I'll add that to the list.

In general I would say that this change is so big that it might warrant a 1.0 release version. If it for various reasons is not compatible with netcore less than 3 we can always point people to a previous version of the extension (which the can choose to install in the vscode UI nowadays). It might not be ideal but I feel that it's hard enough to keep up with one moving target, having to keep track of the trx-side of things as well seems like a bit much too chew on imho.

Agreed.

On that note, .net 5 is around the corner. Will we end up in similar territory there?

Let's be honest: The TRX file was pretty stable. The fact that XUnit was emitting the entire name in the name field was more of a bug that got fixed. I would expect the API to be similarly, if not more, stable. But the real deal of this change is the "streaming" and the fact that we don't have to write the results to disk.

Right now, the data collector has about 40 lines of code. It has literally one single dependency apart from the standard library: Microsoft.TestPlatform.ObjectModel. I think it's unlikely that this is changed, but if it is, it's not hard to adapt. But more generally, MS has a reputation for stable APIs (in .NET at least) because of their many business customers. There was the .NET Core API break, which they are trying to get people aboard with by supporting more of the old scenarios in .NET 5. I doubt that they are going to make a big API break again.

Hope I'm not sounding too negative here, just trying to get as much information as possible +1

Sure, transparency never hurts.

[GeorchW]

I almost wrote a long rant in this place, since the vstest system is really over-engineered. It turns out that we need a logger instead of a datacollector. This also allows us to gather the data from the test discovery.

[stefanforsberg]

This also allows us to gather the data from the test discovery.

That sounds great. Currently it's in addition not only parsing the cmd output but if the user is running nunit or mstests it has to resolve down to an undocument parameter you can send to vstest for it to create a file where we read the test namnes. This is odne because nunit and mstest do not report the FQN of the test but only the method name of the discovered test. I would be very glad to get rid of that code.

But the real deal of this change is the "streaming"

Does this mean that we are able to update the tree more in "real time" as tests are run rather than waiting for the whole test run to be completed before updating the tree?

[GeorchW]

Does this mean that we are able to update the tree more in "real time" as tests are run rather than waiting for the whole test run to be completed before updating the tree?

Exactly.

[GeorchW]

Okay, I'm almost done - all that's left is testing stuff. I'd be glad if you could pull the branch and try it out.

... This is odne because nunit and mstest do not report the FQN of the test but only the method name of the discovered test.

Yeah, I stumbled upon the same issue. There's now some code in the C# logger that covers this inconsistency.

While I was working on the project, I also stumbled upon a lot of thens, which I've replaced with async. They should now all be gone.

[stefanforsberg]

Great!

In general I love the direction of moving code from parsing cmd output in node to csharp code and the streaming of test results.

One thing that I'm curious about is that there is (at least when I try it on my machine) quite a significant difference in speed. When I run all tests for the NUnit project it takes ~40 seconds before the tree is completely updated with this branch compared to the ~4 seconds it takes if I run the same test project in the master branch. Is it something I'm missing on my end here?

[GeorchW]

Is that speed difference from the NUnit project from the repository?

Anyway, I already suspected that it would be a little inefficient, since the tests are currently not async to the TCP connection. That, and a new connection is made for each test. Both of these things could be trimmed down easily.

[stefanforsberg]

” Is that speed difference from the NUnit project from the repository?”

Yep!

[GeorchW]

Ok, weird - that one takes more like the 4 seconds you were talking about on my machine. I've changed the test results to be sent asynchronously, can you try again now?

[stefanforsberg]

Now it's more like 4 seconds =)

[GeorchW]

I tried aggregating messages before sending them to the extension, but it adds quite a bit of complexity and potentially subtle bugs. It would, however, probably improve performance by quite a bit.

It's on a branch of mine, if you want to have a look.

[stefanforsberg]

The current solution for adding test errors as problems in VSCode also is built around getting an aggregated test result. Before adding the problems it clear all previous problems which with the streaming solution means that the next test result clears the potential problem for the last result.

[stefanforsberg]

Also I'm getting errors when I try to run a specific test [WARNING] Could not find a matching test directory for test NunitTests.TestClass2.Fail

(that is either right clicking on a test in the tree and selecting run test or clicking the run icon at the far right of a test)

[GeorchW]

Wow, XUnit is terrible. For context:

• There are two kinds of names for a test case: DisplayName and FullyQualifiedName.
• MsTest and NUnit agree that the latter is a unique name for the test case, i.e. it includes the arguments. (DisplayName contains an abbreviated version without the namespace.)
• XUnit has the reflection name as the FullyQualifiedName, and the unique name of the test as the DisplayName.
• During test discovery, there is an additional property attached to the test case, identifying it as an XUnit test case. However, that property is missing during execution.

I'm seriously considering to use a proxy like testCase.DisplayName.Length > testCase.FullyQualifiedName.Length to distinguish between XUnit and NUnit tests. Otherwise, I'd have to detect the type of test during discovery, then save it somewhere to use it later when determining which name to use. And when we're detecting new tests during a run, we don't have a chance to know what kind of test this is.

[stefanforsberg]

Haha now you're starting to sound like me =)

But yes, if there was some sort of specification and the different test providers adhered to that specification that would have made things easier.

[GeorchW]

The different behaviour is causing me some headaches.

• When using DisplayName instead of FullyQualifiedName, we also have to adapt the filtering mechanism to work on DisplayName instead. I.e. the command line needs to change from dotnet test --filter FullyQualifiedName=... to dotnet test --filter DisplayName=... for the respective tests.
• There is no reliable way of always knowing which name to use (see above). If people are mixing test frameworks within a single project, and a new test appears during a run, we don't have a clue what to do.
• However, no one ever mixes frameworks within a single project. We can, instead, discover tests using our usual strategy, and assume that all new tests are XUnit tests if some are.

What do you think about the following:

• For each project, we separately track whether it uses DisplayName or FullyQualifiedName.
• We make a new setting, dotnet-test-explorer.testNameSource (or something along the lines), with the following possible values:
  • displayName: always use the display name of the test
  • fullyQualifiedName: always use the FQN of the test
  • detect: during discovery phase, look if there are XUnit tests - if there are, use the DisplayName, otherwise use FullyQualifiedName
  • a mapping of path glob => setting, for example:

    {
      "**/MyProject.NormalTests": "fullyQualifiedName",
      "**/MyProject.WeirdTests": "displayName"
    }

• We add some possibility to see both the display name and the fully qualified name of a test.

I think this should really cover everyone's use cases.

[stefanforsberg]

Ah so we don't really get around the problem of different test frameworks implementing the "spec" in slightly different ways by moving to the logger approach. We just need to handle them there instead if I follow you.

Do you still feel that getting away from processing files (which seems way more stable now that we don't rely on chokidar to watch files for us btw) and streaming test results is worth it in terms of the work needed (I realise that it's you that does the work)?

[GeorchW]

Since most of the work is already done and the remaining work is not that much, I'm rather sure that it is worth the work. We also get around the issue of detecting language-dependent strings in the dotnet process output.

The question to me is more whether we can get the building and testing to be as ergonomic as it should be. I'll have to have a look at different projects with a similar mixture of languages and how they set it up.

However, I'm rather busy right now, so I'm probably not going to work on this until the weekend.