xunit / visualstudio.xunit

VSTest runner for xUnit.net (for Visual Studio Test Explorer and dotnet test)
https://xunit.net/
Other
144 stars 81 forks source link

Add initial support for Microsoft.Testing.Platform #403

Closed Evangelink closed 2 months ago

Evangelink commented 6 months ago

Fixes #402

bradwilson commented 6 months ago

@Evangelink When building the sample, I get a compile failure that looks like some kind of missing assembly/package reference:

1>------ Build started: Project: xunit-runner-sample, Configuration: Debug Any CPU ------
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(12,27,12,34): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(12,92,12,117): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(13,9,13,26): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(14,9,14,26): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(15,9,15,75): error CS0012: The type 'ITestApplicationBuilder' is defined in an assembly that is not referenced. You must add a reference to assembly 'Microsoft.Testing.Platform, Version=1.0.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net462\TestPlatformEntryPoint.cs(16,34,16,41): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>Done building project "xunit-runner-sample.csproj" -- FAILED.
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(12,27,12,34): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(12,92,12,117): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(13,9,13,26): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(14,9,14,26): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(15,9,15,75): error CS0012: The type 'ITestApplicationBuilder' is defined in an assembly that is not referenced. You must add a reference to assembly 'Microsoft.Testing.Platform, Version=1.0.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
1>C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\obj\Debug\net6.0\TestPlatformEntryPoint.cs(16,34,16,41): error CS0234: The type or namespace name 'Testing' does not exist in the namespace 'Microsoft' (are you missing an assembly reference?)
1>Done building project "xunit-runner-sample.csproj" -- FAILED.

If I set <EnableXunitRunner>false</EnableXunitRunner>, the compile errors go away. It looks like the hooks for MSBuild are all triggering, but something is missing with references.

bradwilson commented 6 months ago

Looks like it might be package reference related. Converting this:

<PackageReference Include="xunit.runner.visualstudio" Version="2.5.8-pre.2">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

to this:

<PackageReference Include="xunit.runner.visualstudio" Version="2.5.8-pre.2" />

fixes the issue. So it looks like setting developmentDependency is probably wrong for .nuspec now. Next question to answer is: Will NuGet pick up changes to this during package upgrade and update the PackageReference appropriately?

bradwilson commented 6 months ago

All three of these variants did not upgrade the package correctly:

  1. Package Manager in VS2022
  2. dotnet add package xunit.runner.visualstudio --prerelease
  3. dotnet outdated -u

That's pretty disappointing. That means we're on a failure path here unless we can somehow convince people to uninstall and then reinstall the package. 😞

bradwilson commented 6 months ago

Also, it looks like you're setting a value in the .targets file (GenerateTestingPlatformEntryPoint) that you depend on in the .props files, which seems like the wrong order. Shouldn't that top PropertyGroup from the .targets file actually be in the .props files?

bradwilson commented 6 months ago

What are the plans for dotnet vstest? It appears that it currently falls back to the old behavior.

bradwilson commented 6 months ago

Why are .NET Framework tests run in x86 by default if the test project is compiled as AnyCPU?

Run tests: 'C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\bin\Debug\net462\xunit-runner-sample.exe' [net462|x86]
Run tests: 'C:\Dev\xunit\visualstudio\samples\xunit-runner-sample\bin\Debug\net6.0\xunit-runner-sample.dll' [net6.0|x64]
Evangelink commented 6 months ago

Hey @bradwilson,

Sorry for the delay in my replies, I am off and had some trouble finding some time to get back here. I'll resume work on 11th so will be fully available for sure at this stage.

So it looks like setting developmentDependency is probably wrong for .nuspec now

I assume that this is telling to msbuild that all deps are only compile time deps and not runtime ones but for the runner we do need the platform + adapter to be available at runtime. I see you have created a ticket on NuGet, I will try to ping some people so we can see how we can unblock this. @baronfel would you have some info or contact about that?

What are the plans for dotnet vstest? It appears that it currently falls back to the old behavior.

As the name implies this is explicitly asking for the vstest mode. This is equivalent to dotnet test without the property TestingPlatformDotnetTestSupport enabled (see https://learn.microsoft.com/dotnet/core/testing/unit-testing-mstest-runner-integrations#dotnet-test---mstest-runner-mode). While you reference the VSTest bridge package this will continue to work but as soon as this deps is dropped this will no longer work.

Our goal is to be able to deprecate VSTest entirely but this will most likely take some time as we will need to have enough people moved and happy on the new platform.

Why are .NET Framework tests run in x86 by default if the test project is compiled as AnyCPU?

We are not doing any change on our side, this seems to be MSBuild defaults that changed between netfx and netcore. @MarcoRossignoli do you know? @baronfel do you know about that? Or do you have someone that might know?

Evangelink commented 6 months ago

Why are .NET Framework tests run in x86 by default if the test project is compiled as AnyCPU?

@MarcoRossignoli just pointed out this ticket https://github.com/dotnet/sdk/issues/3492 that confirms this is the "default" behavior and this is a change linked to the sdk style project.

bradwilson commented 6 months ago

Sorry for the delay in my replies

No worries, this isn't a rush situation. :)

bradwilson commented 6 months ago

As the name implies this is explicitly asking for the vstest mode.

I ask because I use dotnet vstest with the --Parallel switch because it has superior behavior in that mode than just running dotnet test where all the output is interleaved and not combined (and it allows me to be more selective on what gets run). I understand that the new dotnet test output is basically so sparse that the interleaved output problem is gone, though I suspect you will get end user push back about this being too spares for interactive consumption. You should certainly consider my voice at the top of that list. 😄 Losing the ability to be more selective would be unfortunate, though in our case we do still plan to ship our multi-runners in v3 (like the console runner) that would allow us to continue to be more selective.

baronfel commented 6 months ago

@bradwilson .NET 9 preview 2 includes a rework of the test-reporting infrastructure that does show testing progress, as well as ordered test result reporting directly in MSBuild. It's built on a tiny bit of a hack while the MSBuild team works on a proper progress/ephemeral message API for the MSBuild logging infrastructure, after which we should be able to use those new message types to implement nice CLI progress reporting and such. So it's coming, we're working on it :)

Evangelink commented 6 months ago

We definitely want to work on and improve the interactive mode. As Chet is saying we have already started some work and we are also exploring some paths in parallel so we can decide what's best and most evolutive

bradwilson commented 6 months ago

@Evangelink @MarcoRossignoli @baronfel Where is the source for Microsoft.Testing.Extensions.VSTestBridge? I see dependencies on it in https://github.com/microsoft/testfx but I don't see the source.

bradwilson commented 6 months ago

Is the planned IPC for support inside Visual Studio/VS Code also going to be named pipes? If so, what is the equivalent of --internal-msbuild-node <pipename> to support that? If not, what is the planned IPC mechanism and which client class should I be using to support it?

Why are all the types in Microsoft.Testing.Platform.IPC marked as internal?

bradwilson commented 6 months ago

I'm just going to start opening issues with these questions so they're not buried here, and others can contribute (and perhaps get the benefit of the answers).

First set of issues:

https://github.com/microsoft/testfx/issues/2536 ("FailedTestNodeStateProperty should include the ability to report without an exception") https://github.com/microsoft/testfx/issues/2537 ("What is the intended purpose for the hierarchy provided in TestNode?") https://github.com/microsoft/testfx/issues/2538 ("How do I report a test that's not run?") https://github.com/microsoft/testfx/issues/2539 ("Please document the IPC handshake mechanisms and open up the classes under Microsoft.Testing.Platform.IPC")

bradwilson commented 6 months ago

FYI, I have a parallel prototype effort to integrate Microsoft.Testing.Platform directly into v3's existing architecture without using the VSTest adapter: https://github.com/xunit/xunit/tree/microsoft-testing-platform

Evangelink commented 2 months ago

@bradwilson Please let me know what you would need from us so we can move forward this PR.

bradwilson commented 2 months ago

@Evangelink I have at least two problems before this could be merged:

Evangelink commented 2 months ago

I can definitely do something on the first one (closed source deps) and I'll see what I can do for the NuGet part but as you know it's on a different team so I'm not sure about it 😕

bradwilson commented 2 months ago

I have other unanswered questions:

bradwilson commented 2 months ago

Another question:

Evangelink commented 2 months ago

What is the benefit to the end user of this change?

People can start testing in "form factors" that are not supported by VSTest (e.g. single file app, self contained, potentially native aot depending on your framework...). It's also fixing/avoiding most of the open issues on VSTest repo (concurrency, DOTNET_ROOT...).

In order to obtain the benefit above, the developer must be forced to choose whether their unit test library is compatible with TPv2 or TPvNext, right? How are users reconciling the ability to run their tests both with TPvNext and also with runners that aren't based on TPvNext, like Rider?

No, through the VSTest bridge package, you get support for both VSTest and the new mode.

By default, running vstest.console.exe YourApp.exe/dll will work, dotnet test will work using the VSTest mode (except if you use TestingPlatformDotnetTestSupport - see https://learn.microsoft.com/dotnet/core/testing/unit-testing-platform-integration-dotnet-test#dotnet-test---microsofttestingplatform-mode), VSTest task in AzDO, JetBrains... So all is as before.

In addition they can run the exe directly, use dotnet run or dotnet exec that will use the new platform.

Your adapter assumes that test projects are class libraries that you can provide an entry point for. This is not compatible with our v3 architecture where test projects are already programs with their own entry point. This is still true, right?

v3 is out of scope of this PR, we should discuss how we want to address it. Our MSBuild package is basically doing some work to generate the main method but this is not a requirement, you could define your main yourself. This is an example of a minimal explicit main:

var testApplicationBuilder = await TestApplication.CreateBuilderAsync(args);
// Register the testing framework
testApplicationBuilder.AddTestingFramework(XXX);
using var testApplication = await testApplicationBuilder.BuildAsync();
return await testApplication.RunAsync();

Support for dotnet test requires your adapter because of the way it manipulates MSBuild, correct? There's no way to get your dotnet test shim without using your adapter library, right? Further, you have not officially documented the way this works, right? (This is obviously heavily related to the issue with our v3 test projects already being applications and not class libraries.)

Correct. This is not documented as it's not a public extension point the same way as VSTest isn't documenting its integration with dotnet test. We want (but don't have time for now) to push forward the protocol and make it an open way for anyone to integrate with any dotnet client (dotnet test, VS Test Explorer, VSCode Test Explorer...).

How does Test Explorer know that a project is TPvNext rather than TPv2? If I were to directly add support for your https://github.com/microsoft/testfx/issues/2539#issuecomment-2076613074, how does Test Explorer identify that any random application being built is something that they could call with these command line options?

We expose a MSBuild property that lets TE knows this is using the protocol. There is an option to enable in your VS settings to enable the support of the new mode.

Evangelink commented 2 months ago

I can definitely do something on the first one (closed source deps) and I'll see what I can do for the NuGet part but as you know it's on a different team so I'm not sure about it 😕

Happy to share we got green light about opens sourcing the required components. I'll work on that on the upcoming days and release a preview version to use here (I expect final version to be available around beginning of July).

For the NuGet part, I am looking at the right contacts to help drive it forward.

bradwilson commented 2 months ago

No, through the VSTest bridge package, you get support for both VSTest and the new mode.

Then what is the purpose of the EnableXunitRunner flag?

bradwilson commented 2 months ago

We expose a MSBuild property that lets TE knows this is using the protocol. There is an option to enable in your VS settings to enable the support of the new mode.

Where is this documented?

bradwilson commented 2 months ago

v3 is out of scope of this PR, we should discuss how we want to address it.

It's my intention to migrate to the main branch shortly, so this is not a discussion that can be put off for very long.

Evangelink commented 2 months ago

Then what is the purpose of the EnableXunitRunner flag?

It's a safetynet if users don't want an exe to be produced. This is also the variable that commands having some properties set or not:

This is also controlling whether the test adapter dll should be only copied to the output or referenced: https://github.com/xunit/visualstudio.xunit/pull/403/files#diff-6311d61396af0339e07d9ec7f69921244c42e31ac28456dd51256f343fa2e0edR14-R29

Where is this documented?

It's not documented this is some internal implementation detail between us and Test Explorer. The same goes for some of VSTest configurations.

It's my intention to migrate to the main branch shortly, so this is not a discussion that can be put off for very long.

I saw that main was already merged as v3 hence my change of target to the v2 branch here. As I said, your v3 is a totally different mechanism where you are changing many deps of object model etc so we need to sync and decide what/how to handle the cases. v2 is the only thing we tested and are confident about the behavior.

bradwilson commented 2 months ago

It's not documented this is some internal implementation detail between us and Test Explorer. The same goes for some of VSTest configurations.

I'm going to be honest: answers like this aren't great, and your team has given me this answer a LOT about the new system, including a months old promise to document the JSON-RPC protocol. Answers like this make me feel like I don't want xUnit.net to be the guinea pig in a system you have not finished designing, much less documenting, including a seeming indefinite reliance on the old object model by way of the adapter being the only supported way to use your system.

I saw that main was already merged as v3 hence my change of target to the v2 branch here. As I said, your v3 is a totally different mechanism where you are changing many deps of object model etc so we need to sync and decide what/how to handle the cases. v2 is the only thing we tested and are confident about the behavior.

Combine this with the above, and I don't believe I'm comfortable adopting this yet. It, for all intents and purposes, leads me down a path that might not easily--if ever--support our v3 design.

Evangelink commented 2 months ago

including a months old promise to document the JSON-RPC protocol

cc @drognanar who owns that part

a system you have not finished designing,

Outside of the protocol who is still being developed, we are running our new platform internally for 1y and half without major issue and publicly since 6 months.

much less documenting

I disagree, we are putting a lot of personal time to improve documentation whether technical https://github.com/microsoft/testfx/blob/main/docs/testingplatform/Index.md or functional https://learn.microsoft.com/dotnet/core/testing/unit-testing-platform-intro

including a seeming indefinite reliance on the old object model by way of the adapter being the only supported way to use your system.

If you are talking about VSTest object model. You can definitely cut it out and that's our target goal. Sadly we need to ensure backward compatibility for our users. On your side, and I guess this is what you will do with v3 you can definitely cut it out.

If you are talking about using our new platform package as client for your v3, what's your issue here? Are you not taking any dependency to anything?

The LSP like protocol is definitely a WIP and not a direct priority at the moment as there is a lightweight alternative by using the core platform.

Combine this with the above, and I don't believe I'm comfortable adopting this yet.

I am pretty confused honestly. You have 2 drastically different versions (different enough that they could be 2 products). I am telling you we have been able to validate the v2 for some time and that we are confident this will be working. For the v3, AFAIK, it's still a beta/preview that we haven't looked at. I have absolutely no problem working with you to find how we can integrate with the v3.

As we discussed in past, if your constraint is to have zero dependency on anything we ship and rely only on the protocol then I can only say that this is not a priority for the current semester. We can totally discuss with you and management how to plan for the next one.

bradwilson commented 2 months ago

If you are talking about VSTest object model. You can definitely cut it out and that's our target goal.

How will that work when the entire design appears to be based on the adapter from the TPv2 Object Model? For example, how would one opt into TPvNext as a unit test framework that is not currently using the TPv2 Object Model?

I am pretty confused honestly. You have 2 drastically different versions (different enough that they could be 2 products). I am telling you we have been able to validate the v2 for some time and that we are confident this will be working. For the v3, AFAIK, it's still a beta/preview that we haven't looked at. I have absolutely no problem working with you to find how we can integrate with the v3.

I agree that v2 and v3 have very fundamentally different designs, and the fact that you have validated v2 doesn't help me with v3, and I'm not willing to yet adopt something that might be a dead end for me with v2.

As we discussed in past, if your constraint is to have zero dependency on anything we ship and rely only on the protocol then I can only say that this is not a priority for the current semester.

I don't know if that's the only way I can get what I want, because we haven't yet engaged on what it means to support v3 yet.


Let me see if I can make my position clearer.

You list these issues as the ones that may concern our users:

People can start testing in "form factors" that are not supported by VSTest (e.g. single file app, self contained, potentially native aot depending on your framework...).

We do not support any of these form factors. Native AOT may be something we eventually support, but that support would come at an undetermined point in the future.

It's also fixing/avoiding most of the open issues on VSTest repo (concurrency, DOTNET_ROOT...).

I don't know what these issues are, but they're also potentially sidestepped simply by our v3 architecture. Are you aware of any issues raised here against xUnit.net that would've been caused by these TPv2 problems?

I have also raised concerns with the reduction in interactive usability for our users with dotnet test (which seems to be based on a fundamental design decision that dotnet test should only be used for non-interactive testing like CI builds, an opinion I vehemently disagree with).

Finally, the question of how v3 gets supported needs to be resolved.

In short, I only see this as a step backward for my users, and a potential dead end for myself. I have (well founded, I think) concerns that if I were to merge this for v2, your team would go silent again when asked how the system would work against our v3 design. Today, I have v3 working against TPLv2, which means that trying to merge this will potentially become a blocker for me getting a v3-compatible build of xunit.runner.visualstudio shipped. That's an unacceptable situation for me.

Evangelink commented 2 months ago

Your point is clear I think. Moving forward by closing as you don't see any added value.

bradwilson commented 2 months ago

I'm open to helping you work out design issues to support v3 (and/or any test framework which comes already in executable format).