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

Microsoft.Testing.Platform for xUnit #3478

Closed
bradwilson opened this issue Aug 5, 2024 · 192 comments
Closed

Microsoft.Testing.Platform for xUnit #3478

bradwilson opened this issue Aug 5, 2024 · 192 comments
Assignees
Labels
Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library Area: xUnit Type: Discussion

Comments

@bradwilson
Copy link

The approach xUnit.net v3 is taking for supporting Microsoft.Testing.Platform (since it already produces executables) is to conditionally use the "create TestApplication.CreateBuilderAsync and build it" approach when it understands that it's time to run in that way, vs. letting it run with our entry point.

Our current strategy is to:

  • Link all v3 test projects against Microsoft.Testing.Platform and Microsoft.Testing.Platform.MSBuild
  • Set a NuGet package variable via .props file inside the xunit.v3.core NuGet package to indicate that a v3 test project is in place
  • Ship a (not linked by default) DLL (xunit.v3.runner.testingplatform) inside our xunit.v3.core NuGet package that has all the Microsoft.Testing.Platform logic
  • Detect from within xunit.runner.visualstudio when it's got a v3 test project, and override the entry point to conditionally dispatch to the code inside xunit.v3.runner.testingplatform when it realizes that we're being asked to run inside of dotnet test

There are two branches right now that perform this work in concert with one another:

https://github.com/xunit/xunit/tree/microsoft-testing-platform
https://github.com/xunit/visualstudio.xunit/tree/microsoft-testing-platform

My question is about the documentation & support behind the hook. Right now, through a combination of decompilation of Microsoft.Testing.Extensions.VSTestBridge (which we are not using) and inspecting the logs from dotnet test when using that, it appears that dotnet test is utilizing a command line option that looks like --internal-msbuild-node <argument>.

Based on this comment, this is currently an undocumented and unsupported way to get dotnet test to work:

This is the reason why the option starts with --internal-, options that start with it are hidden internal.

So my question is:

When will the interaction between Microsoft.Testing.Platform test frameworks and dotnet test become documented and supported? If this isn't the way we should be doing it, then what is the officially supported way to enable dotnet test for Microsoft.Testing.Platform test framework implementations that are not based on Microsoft.Testing.Extensions.VSTestBridge?

@bradwilson
Copy link
Author

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 6, 2024

My question is about the documentation & support behind the hook. Right now, through a combination of decompilation of Microsoft.Testing.Extensions.VSTestBridge (which we are not using) and inspecting the logs from dotnet test when using that, it appears that dotnet test is utilizing a command line option that looks like --internal-msbuild-node .

Let's start to describe what's inside the Microsoft.Testing.Platform.MSBuild: It's not a mandatory package, it's used to:

In this case we didn't do any update to the sdk we simply rely on the current support. The task in this case will run the application as-is connecting through a pipe with a custom internal protocol. It means that we're skipping the VSTest infra.

NB. We're working on a new version for dotnet test to skip completely MSBuild and start directly the tests module in server mode(for now with pipe binary protocol, not jsonRpc). This will solve the problem of unclear and confusing UX and giving us a new model for execution like dotnet test --testsmodule /**/*Tests.dll to support scenario where users build on one machine move to another one and run tests. Let's say we're working to avoid the needs to run through MSBuild because it imposes some UX and runtime limitation and make complex the evolutions (i.e. pre and post actions, post assets elaboration etc...).

The target is defined here https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets#L71

The testing framework supports extension and as you know they're linked at build-time. So this task takes also a specific Items collection and generates a call to an extension provided method passing the application builder and the entry point args. So to "plug" the "compile time" registration (vs manual registration where user can manually write the code inside the Main) an extension MUST provide a prop with the registration method. The task will emit the code in the autogenerated main. The sample schema for the Items is for instance the build-time registration of the Trx extension https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Extensions.TrxReport/buildMultiTargeting/Microsoft.Testing.Extensions.TrxReport.props

Thanks to the TestingPlatformBuilderHook we know how to emit the call, we use the TypeFullName provided by the extension and we "append" a method on it (.AddExtensions(builder, args);).

== Why you see internal-msbuild-node <argument> as args?

Because the Microsoft.Testing.Platform.MSBuild it's nothing special than an extension relying on the built-in extensibility point and so it's injecting an IDataConsumer https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/TestPlatformExtensions/MSBuildExtensions.cs using the static registration mechanism https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.props. That pipe is used to transfer the information needed by the InvokeTestingPlatformTask using a custom consumer https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/TestPlatformExtensions/MSBuildConsumer.cs#L65.

So Microsoft.Testing.Platform.MSBuild is not a mandatory package to run tests it's shipping some support to help users to write less code generating some code. This package has got nothing to do with the Microsoft.Testing.Extensions.VSTestBridge. The Microsoft.Testing.Extensions.VSTestBridge is trasparent and is plugged under the test framework adapter interface https://github.com/microsoft/testfx/blob/main/docs/testingplatform/itestframework.md. This is used to have 0 update for old VSTest adapters. The future goal would be remove it and avoid deps on VSTest infrastructure. Clearly the jurney won't be quick, the new testing platform has got a complete different extensibility and runtime model so extensions like collectors, loggers are no more supported, and for that we need to fill the gap with new extensions like we did here https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-platform-extensions.

From versioning perspective Microsoft.Testing.Extensions.VSTestBridge and Microsoft.Testing.Platform.MSBuild depend on the single core platform with 0 dependency Microsoft.Testing.Platform. This means that at built-time nuget will resolve the version, giving us (if we play good with semver) pretty stable and deterministic/testable platform+extensions configuration, a pain point of VSTest.

Microsoft.Testing.Platform.MSBuild is a transitive dep so the targets will flow up. The issue I see with the custom main with the if is that we need to take the control of program.cs if you want to give to xUnit access to the extensions. We emit the full program.cs with the registration using the MSBuild Items above.
This is what we codegen:

//------------------------------------------------------------------------------
// <auto-generated>
//     This code was generated by Microsoft.Testing.Platform.MSBuild
// </auto-generated>
//------------------------------------------------------------------------------

[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
internal sealed class TestingPlatformEntryPoint
{
    public static async global::System.Threading.Tasks.Task<int> Main(string[] args)
    {
        global::Microsoft.Testing.Platform.Builder.ITestApplicationBuilder builder = await global::Microsoft.Testing.Platform.Builder.TestApplication.CreateBuilderAsync(args);
        Microsoft.Testing.Platform.MSBuild.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.Telemetry.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.VisualStudio.TestTools.UnitTesting.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.CodeCoverage.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.TrxReport.TestingPlatformBuilderHook.AddExtensions(builder, args);
        using (global::Microsoft.Testing.Platform.Builder.ITestApplication app = await builder.BuildAsync())
        {
            return await app.RunAsync();
        }
    }
}

Collecting from MSBuild

image

So I think that if you want to plug to the new testing platform you have to opt-out your one and give us the control on the starting point or extensions won't work.

Another possible issue I see is that the new testing platform support custom command line arguments so if you want your users use your v3 options at command line you should provide the extension for it. Here the documentation https://github.com/microsoft/testfx/blob/main/docs/testingplatform/icommandlineoptionsprovider.md with a sample here https://github.com/microsoft/testfx/blob/main/docs/testingplatform/Source/TestingPlatformExplorer/TestingFramework/TestingFramework.CommandLineOptions.cs

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 6, 2024

If this isn't the way we should be doing it, then what is the officially supported way to enable dotnet test for Microsoft.Testing.Platform test framework implementations that are not based on Microsoft.Testing.Extensions.VSTestBridge?

Finally if you plug to the Microsoft.Testing.Platform you will have built-in support to all the "protocol" model we will ship for dotnet test and VS/VS Code transparently. The server mode is developed, maintained and evolved inside the core Microsoft.Testing.Platform.

You could have an msbuild property like <UseTestingPlatform> and if true you ref Microsoft.Testing.Platform and Microsoft.Testing.Platform.MSBuild same version and opting out your codegen.
In this way users can opt-in the new testing platform for what you've already implemented the framework and we take control of the entry-point and extensions(if users will add some). In that way you will have everything out of the box.

Another suggestion maybe for next loop is to have all this inside an MSBuild sdk. It helps a lot with clean opt-in/out without giving to users a way to break the config https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-mstest-sdk source here https://github.com/microsoft/testfx/tree/main/src/Package/MSTest.Sdk

@bradwilson
Copy link
Author

bradwilson commented Aug 6, 2024

So Microsoft.Testing.Platform.MSBuild is not a mandatory package to run tests

When users are asking us to support Microsoft.Testing.Platform (primarily because they've been told there are performance benefits), I don't think this statement is true, because they will expect dotnet test to run with Microsoft.Testing.Platform if we claim to support it.

Another way to phrase this is: if any unit test framework (ignore xUnit.net v3 the moment) only supported Microsoft.Testing.Platform and took no other dependencies, how would users be able to see that benefit? There would be no support in dotnet test and no support in Test Explorer. The only way they'd "see" it is if they tried to directly run the test project itself.

If that's the intended goal, to be able to run directly run a project and have it be faster than dotnet test, we already meet that goal with our v3 design, without needing Microsoft.Testing.Platform.

I personally believe that from a user perspective they're going to expect that we use Microsoft.Testing.Platform for dotnet test and Test Explorer in order to consider this feature complete. There would be zero benefit for us shipping an "only Microsoft.Testing.Platform" feature today, because all we'd end up doing is adding your layer of indirection around a discovery and execution system that already exists, and by definition is faster when we don't involve you. (That assumes that we let you control the entry point.) Do you agree?

This is used to have 0 update for old VSTest adapters. The future goal would be remove it and avoid deps on VSTest infrastructure.

We're ready now. 😄

Our current split plan right now with xunit.runner.visualstudio is that we'll continue to use TPv2 for v1 and v2 unit tests, and Microsoft.Testing.Platform for v3 unit tests. I have isolated the logic to "turn on" Microsoft.Testing.Platform for v3 tests into this package, but there's no reason for that to be true today. My original prototype just enabled it straight away for all v3 test projects without needing that separate package. I'm honestly torn on which is better, but for sure because of the way NuGet dependencies work in my current plan, all v3 test projects will end up getting linked against Microsoft.Testing.Platform whether they end up bringing in xunit.runner.visualstudio or not, simply because there's no other way for me to conditionally add that reference based on whether the user has a v3 test project. NuGet's package dependency logic is "always on" or "always off": your dependencies are in your .nuspec, no way around that. And we don't want to split v1/v2 users off to one package and v3 users off to a different package. Besides, there appears to be zero downside to our users having that dependency when it isn't used, precisely because you've designed it for zero upstream dependencies.

The issue I see with the custom main with the if is that we need to take the control of program.cs if you want to give to xUnit access to the extensions. We emit the full program.cs with the registration using the MSBuild Items above.

You've botched the dependency tree, though. Microsoft.Testing.Platform.MSBuild requires references, based on that auto-generated code, that you haven't brought along with you. You're assuming they'll come from users who are referencing Microsoft.Testing.Extensions.VSTestBridge. Right now you probably assume that those two packages always live together, but that's not true for me. That's the reason that I don't have any of the extension registrations in my code that you do, other than the one from Microsoft.Testing.Platform.MSBuild, because I haven't taken any of those other dependencies: https://github.com/xunit/xunit/blob/cccc2b35f58bf2503b63b5ab380d20f511006621/src/xunit.v3.runner.testingplatform/TestPlatformTestFramework.cs#L227-L232

We can't unilaterally give you control of our entry point, though. That's out of the question, because our users need to be able to run tests outside of dotnet test and Test Explorer. Not only do we have the interactive scenario (where a user just runs the test project) where we need strong control over the UX, there's also the scenario where our multi-assembly runners (like xunit.runner.visualstudio and xunit.v3.runner.console) need to be able to launch and control v3 unit test projects, and that is simply not possible if we let you own the command line. I looked at your command line infrastructure and it's not sufficient for the kinds of parsing and validation we do. We also need strong control over StdOut when we're running in what I called "automated" mode, which is how v3 unit test projects are controlled by our multi-assembly runners. The only output permitted on StdOut is one-per-line JSON serialization of our runner message model.

So I think that if you want to plug to the new testing platform you have to opt-out your one and give us the control on the starting point or extensions won't work.

The two questions I have here aren't around the core of what we're doing right now in that code snippet above, but:

  1. How stable is this extension list over time? Would it be possible for us to discover that list at build time without using your injected entry point, like maybe you inject a function at a well known namespace/class/method that could be called by us at the right time to do extension registration? That's basically 90% of your auto-gen'd code, but in a way that we could consume; the other 10% is understanding what command line switches tell us we need to delegate control to you for dotnet test and Test Explorer to function (like --server).
  2. How many dependencies do you force into us as a result?

You speak above about the zero-dependency Microsoft.Testing.Platform, but those extensions aren't in Microsoft.Testing.Platform. They're spread among a bunch of other NuGet packages, which do have real dependency impacts directly on our end users. The top level dependency list from Microsoft.Testing.Extensions.VSTestBridge 1.3.2 is:

Digging into those, you can see more upstream dependencies of things that unit test users very well may be using, and therefore have potential places to conflict when different versions are needed that end up not being compatible.

Bottom Line

Is the "we must own your entry point" a 100% requirement that you are immovable from? If so, then it sounds like we will never support Microsoft.Testing.Platform, because it is a 100% requirement on our side that we own our entry point.

We'd prefer a model where we work with you to find a way so that you can evolve Microsoft.Testing.Platform while we still own our own command line UX. An ideal solution would be for us to be able to just update NuGet package versions and it "just works", but if that means occasionally updating our entry point to accommodate new extensions and the evolution of your command line requirements, I'm fine with that. We can move forward with that strategy, assuming that things get documented appropriately. Right now having to dig everything out of decompiled binaries and log file isn't really a tenable solution. I understand that that's because you're still in an experimental place (at least as it pertains to dotnet test; I expect we're going to have a similar conversation in my other issue related to Test Explorer). But right now, I know that you would like for us to support Microsoft.Testing.Platform, and our users would like the presumed performance benefits when using dotnet test and Test Explorer. It just seems like it's too early for us to be able to do that officially, because we are unable to use the VSTestBridge (where it seems like the heavy investment went in order to get things bootstrapped went).

If the entry point ownership is 100% no compromise, then please say that plainly so that I can close this issue and point users here when they ask why we won't support Microsoft.Testing.Platform.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 6, 2024

I personally believe that from a user perspective they're going to expect that we use Microsoft.Testing.Platform for dotnet test and Test Explorer in order to consider this feature complete. There would be zero benefit for us shipping an "only Microsoft.Testing.Platform" feature today, because all we'd end up doing is adding your layer of indirection around a discovery and execution system that already exists, and by definition is faster when we don't involve you. (That assumes that we let you control the entry point.) Do you agree?

We're towards that goal but we added already some benefits that are currently used.

First step was have the new platform with all the needed extensibility point.
This already solved some problem for some team, they're running in CI with dotnet run (with AzureCLI@2 for instance) or manually with scripts.
Others are using F5 in VS/VS Code working in a focus way without the TestExplorer (they setup the filter in the launcher and they debug in that way) for tests that are hard to run or failing inside VSTest host.
We shipped some interesting extension like Retry that now is portable in every OS/.NET version, this is another already used scenario, where the user friendliness of dotnet test is less important than the feature.
Plus allowing to run aot or with custom configuration like in fan out or without the needs to have the sdk on the machine are anyway appreciated additions.
We're close to have Test Explorer using directly the testing application in server mode (we're dogfooding it and we're adding the last needed feature to have the parity with VSTest).

So the Microsoft.Testing.Platform.MSBuild currently is giving the same performance as dotnet run but with the known gesture. And we're working to drop it in favor of the new dotnet test version that will talk directly with the test application like Test Explorer without the needs of the MSBuild infra, this should solve the old confusing UX output. This needs some time.

We're ready now. 😄

I saw our branch, happy to see that the documentation and the sample helped to quickly integrate. On our side we need a bit more time to "adapt" to the rest of the environment and tooling.

NuGet's package dependency logic is "always on" or "always off": your dependencies are in your .nuspec, no way around that.

We solved that problem with the MSBuild SDK there you can conditionally opt-in out set of different packages. We switch completely the implementation of the engine when we go AOT. Preserving the UseVSTest for users that wants to opt-in old set of packages.

You've botched the dependency tree, though. Microsoft.Testing.Platform.MSBuild requires references, based on that auto-generated code, that you haven't brought along with you. You're assuming they'll come from users who are referencing Microsoft.Testing.Extensions.VSTestBridge

What you mean? Microsoft.Testing.Platform.MSBuild depends only on the core plat to insert the extension needed for dotnet test (IDataConsumer) https://www.nuget.org/packages/Microsoft.Testing.Platform.MSBuild#dependencies-body-tab it doesn't need Microsoft.Testing.Extensions.VSTestBridge. It generates the entry point and the configuration but you can opt-out it.

I looked at your command line infrastructure and it's not sufficient for the kinds of parsing and validation we do.

What's missing?

Not only do we have the interactive scenario (where a user just runs the test project) where we need strong control over the UX

We ship by default with our console UX to report the result and the errors but we don't capture the output in any way, if you do Console.WriteLine it will end in console. If you need the complete control on the UX(we call it OutputDevice) we can open this extension (closed because we were waiting to understand if can be interesting for someone) https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/OutputDevice/IPlatformOutputDevice.cs and you can substitute completely the UX. The new testing platform can run headless. @nohwnd is working to make the built-in one better at the moment #3292.

How stable is this extension list over time? Would it be possible for us to discover that list at build time without using your injected entry point, like maybe you inject a function at a well known namespace/class/method that could be called by us at the right time to do extension registration? That's basically 90% of your auto-gen'd code, but in a way that we could consume; the other 10% is understanding what command line switches tell us we need to delegate control to you for dotnet test and Test Explorer to function (like --server).

This is not super clear as question, at the moment we're doing what you said, we use MSBuild prop Items that we codegen at build-time as I've explained above(https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.props). Extension will be shipped by us and by users and if they provide the correct Item inside the prop they will register themselves without the need of any change inside the program.cs. Otherwise users can disable the codegen and do it manually with the classic extension method.

How many dependencies do you force into us as a result?

For a test adapter the only dependency is Microsoft.Testing.Platform. Microsoft.Testing.Platform.MSBuild is needed today to inject the entry point with the custom extension registration that you can do by your own or providing a clear Program.cs and to plug dotnet test using MSBuild that you can do by your own using the current infrastructure. When TextExplorer and the new dotnet test will be completed the usage of Microsoft.Testing.Platform.MSBuild can be opted-out and used only to insert the entry point and transform the configuration file.
Anyway these 2 extension are pretty light Microsoft.Testing.Platform takes no deps and Microsoft.Testing.Platform.MSBuild takes Microsoft.Testing.Platform. So we should not pollute or interfere in any way.

Microsoft.Testing.Extensions.Telemetry is used for telemetry, useful to understand what we can deprecate, here all the info collected https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-platform-telemetry, but optional.
Microsoft.Testing.Extensions.TrxReport.Abstractions: it's part of the Trx generator extension. The new testing platform is shipped with a new capability system that allows extensions to talk and provide information to other extension. This lib export the object model for some custom property to push in the message bus to make the trx complete (trx is based on class/method names and the default list of properties shipped with the core lib doesn't containt specific extension information). So it's optional, good to have to produce a good trx, the deps is without any deps but again the core one. Here the "properties" exposed by the trx extension https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Extensions.TrxReport.Abstractions/TrxReportProperties.cs

After users can add extensions and extensions can take dependencies our of our control.

You speak above about the zero-dependency Microsoft.Testing.Platform, but those extensions aren't in Microsoft.Testing.Platform. They're spread among a bunch of other NuGet packages, which do have real dependency impacts directly on our end users. The top level dependency list from Microsoft.Testing.Extensions.VSTestBridge 1.3.2 is:

You don't need Microsoft.Testing.Extensions.VSTestBridge if you drop the old VSTest support. For what I see you didn't use it in xunit v3 so you should not see it in the list of deps. We use it in MSTest because unfortunately we're not yet ready to drop the object model of VSTest that's down the code and we don't have separation between engine objects and VSTest objects.

Is the "we must own your entry point" a 100% requirement that you are immovable from? If so, then it sounds like we will never support Microsoft.Testing.Platform, because it is a 100% requirement on our side that we own our entry point.

If you copy our way to at compile time register the dependency you can take it. It's explained above, if you codegen the registration by your own it's fine we don't need the entry point. You need to flow the options to our builder for server mode (for dotnet test and TestExplorer)

But right now, I know that you would like for us to support Microsoft.Testing.Platform, and our users would like the presumed performance benefits when using dotnet test and Test Explorer. It just seems like it's too early for us to be able to do that officially, because we are unable to use the VSTestBridge (where it seems like the heavy investment went in order to get things bootstrapped went).

Our goal is to drop the VSTestBridge so you're one step ahead. If you want to wait till we will have complete the new dotnet test and the TestExplorer to complete the integration I think it's understandable. We need to understand how to handle the command line issue. If you try a --help you'll see some options and extension are also registering custom options. I think that if you want to take the entry is doable for the compile-time extension registration so that problem I think it's solved(here how we're doing and the Items spec https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/TestingPlatformEntryPointTask.cs#L45 and we can help there too given that we spec the MSBuild Items), we provide the server mode for dotnet test and TestExplorer and we can fill the gap in case something is missing so also that part should be fine, the only tricky part is the command line.

@bradwilson
Copy link
Author

What you mean? Microsoft.Testing.Platform.MSBuild depends only on the core plat to insert the extension needed for dotnet test (IDataConsumer) https://www.nuget.org/packages/Microsoft.Testing.Platform.MSBuild#dependencies-body-tab it doesn't need Microsoft.Testing.Extensions.VSTestBridge. It generates the entry point and the configuration but you can opt-out it.

Your claim was that you auto-generate this entry point (which, BTW, I cannot duplicate):

[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
internal sealed class TestingPlatformEntryPoint
{
    public static async global::System.Threading.Tasks.Task<int> Main(string[] args)
    {
        global::Microsoft.Testing.Platform.Builder.ITestApplicationBuilder builder = await global::Microsoft.Testing.Platform.Builder.TestApplication.CreateBuilderAsync(args);
        Microsoft.Testing.Platform.MSBuild.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.Telemetry.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.VisualStudio.TestTools.UnitTesting.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.CodeCoverage.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.TrxReport.TestingPlatformBuilderHook.AddExtensions(builder, args);
        using (global::Microsoft.Testing.Platform.Builder.ITestApplication app = await builder.BuildAsync())
        {
            return await app.RunAsync();
        }
    }
}

My counterpoint was that several of these lines would not compile without additional references; specifically:

        Microsoft.Testing.Extensions.Telemetry.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.VisualStudio.TestTools.UnitTesting.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.CodeCoverage.TestingPlatformBuilderHook.AddExtensions(builder, args);
        Microsoft.Testing.Extensions.TrxReport.TestingPlatformBuilderHook.AddExtensions(builder, args);

Given that I tried to repro this, and saw only this as your auto-generated entry point, I'm confused by what you claimed previously:

image

Note that this is exactly what we're doing as well:

image

(The extra Register call is us registering ourselves as a test framework.)

So I'm not sure what/how we go from what I'm seeing today to what you claimed above. Is your claim above an idealized goal for the future? Is the auto-generation dependent on the user adding references to NuGet packages that include register-able things (for lack of a better term)? Are you sniffing the TestingPlatformBuilderHook item groups from the NuGet packages to decide how to generate the entry point? If so, that's something I could do as well, and that would always keep us in sync. Users could add the packages they want and we'll make sure they get registered. Is that how it works today?

What's missing?

  • How can I make the -? output look like this? Also note: there are two command line options that aren't based on dashes: you can specify :<seed> to set the randomization seed and you can specify a path to the configuration file. This was done to match the command line option in xunit.v3.runner.console which allows you to specify <assembly>:<seed> [<configfile>] [<assembly>:<seed> [<configfile>]...].

    image

  • Is there a hook that exists for me, before you ask to discover or execute, that I could inspect those command line options and set up things like the reporter?

  • How would I validate something complex like the -maxThreads option?

  • Can I use single-dashes for long names, which has historically been how our console runner works? Or will I be forced to change all my command line options that my users have a decade of muscle memory (and build scripts)?

We ship by default with our console UX to report the result and the errors but we don't capture the output in any way, if you do Console.WriteLine it will end in console.

I'll take your word for it if you say that you don't ever output anything to the console. My experience so far has been with dotnet test, but those headers and the VSTest style output may be a result of running via dotnet test rather than running directly.

You need to flow the options to our builder for server mode (for dotnet test and TestExplorer)

Does this mean I only have to look for you to pass --server, at which point I just hand you all the args and go? That's effectively what I'm doing today (except that I'm also looking for --internal-msbuild-node, but if we remove Microsoft.Testing.Platform.MSBuild and the temporary dotnet test overloads, then I could stop looking for that).

If you want to wait till we will have complete the new dotnet test and the TestExplorer to complete the integration I think it's understandable.

If there's value in me providing something in the meantime that people could opt into that would work, then I'd be happy to do that (as I presume it would also help you all do testing and validation of the proposed usage).

Today that looks like me including Microsoft.Testing.Platform.MSBuild and recognizing --internal-msbuild-node to offer that transitional dotnet test support, which I'm okay with if that's what people want.

Also today, as #3479 illustrates, I have no idea whether I'm being used via TPLv2 or Microsoft.Testing.Platform inside of Visual Studio. I do know the tests are being run, but there is no visible way to know which path they're taking (short of giving myself environment debugger hacks to watch and see which path things take), but I'm happy to have that discussion over in that issue, if it's appropriate.

I feel like, based on this conversation, the appropriate path is probably:

  • Remove our dependency on Microsoft.Testing.Platform.MSBuild and accept that dotnet test for now will still be done via TPLv2
  • Enable Microsoft.Testing.Platform in v3 test projects, always, and remove the necessity of xunit.runner.visualstudio (which will remain how you enable TPLv2)
  • Trust that when I have both Microsoft.Testing.Platform and TPLv2 support (because almost all users will still want to reference xunit.runner.visualstudio, at least until such time that realistically zero users will need the support of a TPLv2 runner), you all are picking up that we support M.T.P and using us that way.

Given the non-intrusive nature of this (basically, I'm just sniffing for --server and users bring along one dependency they won't care about), I have no problem doing this now and not waiting. I just want to get clarification on my question about how your auto-generation is triggered today, and make sure I can do something equivalent on my end.

@bradwilson
Copy link
Author

I do see one problem right now with my current thought:

<TestingPlatformBuilderHook Include="98058041-B5B6-4A75-9834-58E6DF796A22" Condition=" '$(GenerateTestingPlatformEntryPoint)' == 'True' " >

We obviously have to set GenerateTestingPlatformEntryPoint to false so you won't generate the entry point, which means I can't sniff to TestingPlatformBuilderHook entries to do the dynamic registration.

Is there any reason this should actually be conditional? What's the harm in all of your extensions just always having TestingPlatformBuilderHook MSBuild items? It's just a couple strings. Unless you make this stuff always on, nobody else will be able to generate an entry point but you.

(That said, I could make it work today, because removing Microsoft.Testing.Platform.MSBuild turns off your entry point generation anyway, but it does sound like eventually you're going to move that somewhere else and then we'll be stuck.)

@bradwilson
Copy link
Author

I have updated the microsoft-testing-platform branch in the core framework, and I'm throwing the one from xunit.runner.visualstudio away, since it's not necessary in the current proposed model.

If you want to take a look at the changeset and see if this aligns with what you'd expect: xunit/xunit@0073f0b

This is a project that just references xunit.v3 (albeit one built from my branch):

image

It references the Microsoft.Testing.Extensions.TrxReport NuGet package, so this is what the auto-generated entry point looks like:

// <auto-generated> This file has been auto generated by xunit.v3.core. </auto-generated>

[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
public class XunitAutoGeneratedEntryPoint
{
    public static int Main(string[] args)
    {
        if (args.Length > 0 && args[0] == "--server")
            return global::Xunit.Runner.InProc.SystemConsole.TestingPlatform.TestPlatformTestFramework.RunAsync(args, typeof(Microsoft.Testing.Extensions.TrxReport.TestingPlatformBuilderHook)).GetAwaiter().GetResult();
        else
            return global::Xunit.Runner.InProc.SystemConsole.ConsoleRunner.Run(args).GetAwaiter().GetResult();
    }
}

I followed the pattern of the code you linked to that ensures that the TrxReport extension is always registered last.

@bradwilson
Copy link
Author

I followed the pattern of the code you linked to that ensures that the TrxReport extension is always registered last.

I feel like maybe an optional priority might be better than hard coding this logic.

@MarcoRossignoli
Copy link
Contributor

So I'm not sure what/how we go from what I'm seeing today to what you claimed above. Is your claim above an idealized goal for the future? Is the auto-generation dependent on the user adding references to NuGet packages that include register-able things (for lack of a better term)? Are you sniffing the TestingPlatformBuilderHook item groups from the NuGet packages to decide how to generate the entry point? If so, that's something I could do as well, and that would always keep us in sync. Users could add the packages they want and we'll make sure they get registered. Is that how it works today?

Yes we're "sniffing the TestingPlatformBuilderHook" so the lines you see inside the codegen depends on the extension packages added.

I feel like maybe an optional priority might be better than hard coding this logic.

I agree, we discussed and for first version we decided to embed it. But it's on the table.

How can I make the -? output look like this? Also note: there are two command line options that aren't based on dashes: you can specify : to set the randomization seed and you can specify a path to the configuration file. This was done to match the command line option in xunit.v3.runner.console which allows you to specify : [] [: []...].

I think that is not fundamental have 100% same UX for the help. Under the new(in-progress) "dotnet test --help" for instance we aggregate all the options and we notify the users if some tests module has got different options (usually brought by extensions) because at runtime for that specific test module with the missing extension if will fail for invalid option. I mean the UX will be for sure different.

So I think that you can map all your options under the testing plat and for the option without a "-some" add a new one i.e. --config ... --seed ... only when is running in testing platform mode. Despite your UX looks really well organized I think that at least for the start have a plain list where users can read and maybe look at the documentation if something is not clear is a good starting point. Usually once users understand the option they won't change it for some time. I mean the important thing I see is that users can in some way opt-in/out all the xunit feature.

PS. It's not so bad idea give an extensibility point where test framework author can write the custom help, I'll open a ticket for it. cc: @Evangelink

Is there a hook that exists for me, before you ask to discover or execute, that I could inspect those command line options and set up things like the reporter?

In your ITestFramework implementation you can get thought the IServiceProvider the ICommandLineOptions and query the command line and apply the options. For instance https://github.com/microsoft/testfx/blob/main/docs/testingplatform/Source/TestingPlatformExplorer/TestingFramework/TestingFramework.cs#L47

How would I validate something complex like the -maxThreads option?

The ICommandLineOptionsProvider allows you to validate single options and global configuration at the end for instance https://github.com/microsoft/testfx/blob/main/docs/testingplatform/Source/TestingPlatformExplorer/TestingFramework/TestingFramework.CommandLineOptions.cs

Can I use single-dashes for long names, which has historically been how our console runner works? Or will I be forced to change all my command line options that my users have a decade of muscle memory (and build scripts)?

At the moment we support -- and - also if we advice for the -- in the documentation and in the help.

I'll take your word for it if you say that you don't ever output anything to the console. My experience so far has been with dotnet test, but those headers and the VSTest style output may be a result of running via dotnet test rather than running directly.

We give a way to personalize the banner implementing the IBannerMessageOwnerCapability and at the moment we ship our built-in output console. But we can if you need add an option that will allow you to have headless runtime and you can write your own UX. If it's a requirement for you open a ticket for it.

Does this mean I only have to look for you to pass --server, at which point I just hand you all the args and go? That's effectively what I'm doing today (except that I'm also looking for --internal-msbuild-node, but if we remove Microsoft.Testing.Platform.MSBuild and the temporary dotnet test overloads, then I could stop looking for that).

Yes, in--server modes (jsonRpc for VS and pipe binary for dotnet test) we expect to invoke the tests module passing --server --client-port xx --client-host xxxx (VS create the socket at the moment) or --server --dotnet-test-pipe xxxx (but you don't have to take care of it we could change in future at the moment the options for server mode are hidden and used only by tools) to decide which protocol we want to use...so if you do an index of inside the args[] and you find this parameter and it's not one of yours you could "forward" to the testing platform workflow. And anyway if this is not enough we can add an hidden option (we support hidden options) like --internal-run-testingplatform and you can check that one or something specific.

Today that looks like me including Microsoft.Testing.Platform.MSBuild and recognizing --internal-msbuild-node to offer that transitional dotnet test support, which I'm okay with if that's what people want.

True current dotnet integration that rely on the current(old) MSBuild infra can be recognized using --internal-msbuild-node . NB --internal- prefix is reserved.

Also today, as #3479 illustrates, I have no idea whether I'm being used via TPLv2 or Microsoft.Testing.Platform inside of Visual Studio. I do know the tests are being run, but there is no visible way to know which path they're taking (short of giving myself environment debugger hacks to watch and see which path things take), but I'm happy to have that discussion over in that issue, if it's appropriate.

VS decide if start using the server mode (start process passing args) looking at the ProjectCapability named TestingPlatformServer. We set it inside the Microsoft.Testing.Platform.MSBuild at the moment https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets#L11-L16 if that is found and the preview feature(not yet public) is enabled the TestExplorer will use the jsonRpc protocol and will skip VSTest infra.
Again, if you accept for now to link Microsoft.Testing.Platform.MSBuild and Microsoft.Testing.Platform that are deps free we can "incrementally" provide the glue and when we will be there we can drop the Microsoft.Testing.Platform.MSBuild. The reason for the split of the two lib is because we want to be able to "always" in every situation run tests, to have always the plan b. You can link only Microsoft.Testing.Platform and we can run the test, the services provided by msbuild can be done manually(entry point, config) and in some scenario dotnet test is not available at all so your can anyway run your tests.

Remove our dependency on Microsoft.Testing.Platform.MSBuild and accept that dotnet test for now will still be done via TPLv2
Enable Microsoft.Testing.Platform in v3 test projects, always, and remove the necessity of xunit.runner.visualstudio (which will remain how you enable TPLv2)
Trust that when I have both Microsoft.Testing.Platform and TPLv2 support (because almost all users will still want to reference xunit.runner.visualstudio, at least until such time that realistically zero users will need the support of a TPLv2 runner), you all are picking up that we support M.T.P and using us that way.

I think you could keep both deps Microsoft.Testing.Platform.MSBuild and Microsoft.Testing.Platform for v3, when you're running with Tpv2 we're using reflection/scan files to load the adapter and you're "entering" in your code ignoring the 2 dll (Microsoft.Testing.Platform.MSBuild.dll and Microsoft.Testing.Platform), isn't it? We ship both libs aligned always so if they're harmful(if not 2 small file inside the bin) update them to take new stuff should be easy upgrade. Users can opt-in/out the integration with dotnet test using the TestingPlatformDotnetTestSupport.
But up to you if you want to start without the current dotnet test support. The good part is that the msbuild package allows you also to plug VS thanks to the TestingPlatformServer. Let's say that we could think to have the same setup we have in MSTest but you don't have the VSTestBridge and the others deps that we ship by-default (extensions like codecoverage, trx and telemetry).

Is there any reason this should actually be conditional? What's the harm in all of your extensions just always having TestingPlatformBuilderHook MSBuild items? It's just a couple strings. Unless you make this stuff always on, nobody else will be able to generate an entry point but you.

The reason is to not pollute MSBuild with unused items. We didn't expect the needs to provide the same items outside our codege. But it's fine for me remove the conditional and let you sniff.

If you want to take a look at the changeset and see if this aligns with what you'd expect: xunit/xunit@0073f0b

I'll check and let you know soon. Thanks for the effort.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 7, 2024

I was wondering if to solve the issue with the codegen for the extension we can do something simpler where you can keep the Microsoft.Testing.Platform.MSBuild and actually you opt-out our entry point but we always emit a full registration method kinda like:

Microsoft.Testing.Platform.TestingPlatformBuilderHook.AddExtensions(builder,args);

So you can simply emit in your code and we take care of the dirty sniff.

And we update our codegen to call the same method for symmetry.
In this way we can evolve the extensions infra without the need to bother you too much and you don't have to implement the logic by your own.

@bradwilson
Copy link
Author

I think that is not fundamental have 100% same UX for the help. Under the new(in-progress) "dotnet test --help" for instance we aggregate all the options and we notify the users if some tests module has got different options (usually brought by extensions) because at runtime for that specific test module with the missing extension if will fail for invalid option. I mean the UX will be for sure different.

I'm not talking about what the user sees with dotnet test --help, but rather what the user sees with dotnet run -- -?, though I understand if we were using you to output the latter, then what we registered in terms of options would impact the former, and that potentially complicates things.

At the moment I think of dotnet test as falling into the "third party multi-assembly runner" pattern (just like Test Explorer or Resharper or NCrunch or CodeRush) rather than the dotnet run "first party single-assembly runner" experience. I don't expect multi-assembly runners to have our same UX, but I would like our single-assembly runner experience to be as near to identical in UX as our multi-assembly runner experience (i.e., xunit.v3.runner.console). For ease of migration, I don't want to radically change that UX from v2 to v3 (not to mention I quite like our UX here, for the most part).

I think that you can map all your options under the testing plat and for the option without a "-some" add a new one i.e. --config ... --seed ... only when is running in testing platform mode.

Agreed.

The interesting problem with seed, specifically, is that it's a per-assembly value. I don't think there's any way to run dotnet test and have it run multiple assemblies (and/or multiple target frameworks, which do end up with unique seed values), but then allow the user to say "the seed for assembly A in net472 is 123, but the seed for assembly B in net6.0 is 456". That's probably a degenerate case that doesn't need to be dealt with, though: if the user is asking to run a specific seed, then they're trying to repro a problem with a specific assembly.

A related question: how will these command line options be surfaced into Test Explorer, if at all? To use the seed example, would I be able to set a seed for a specific test assembly (at a specific target framework) with some UI gesture in Test Explorer?

We give a way to personalize the banner implementing the IBannerMessageOwnerCapability and at the moment we ship our built-in output console. But we can if you need add an option that will allow you to have headless runtime and you can write your own UX. If it's a requirement for you open a ticket for it.

For the moment, since we're trying to achieve the ability for me to still own my entry point, I don't think we need to open any other issues. I'd personally prefer for me to continue to own my entry point, at which point it becomes unnecessary for me to own the banner or anything else like that (since it would only be seen in the case of the user running dotnet test, and not in the case of the user directly running the assembly or running it via xunit.v3.runner.console).

so if you do an index of inside the args[]

This is a simple change for me to make (rather than assuming it's first).

if this is not enough we can add an hidden option

Personally I'm fine with --server, because it doesn't match the pattern we use for command line options anyways (we converted from /option to -option in 2.x for cross platform reasons, and we did not adopt the --option pattern).

I'm also fine if it turns out that some other test framework wants this hidden option, we can look for that hidden option instead.

VS decide if start using the server mode (start process passing args) looking at the ProjectCapability named TestingPlatformServer. [...] the preview feature(not yet public) is enabled the TestExplorer will use the jsonRpc protocol and will skip VSTest infra

You should be able to test this with my branch right now. All I do is run ./build packages in the main branch of xunit/xunit and it'll make *-dev packages. You use nuget.config to point at the /artifacts/packages folder in the clone to pull the package locally.

I also use a function that I wrote that cleans out -dev packages from the NuGet cache before trying to pick up a newly build package:

function Clean-DevPackages {
    $packageCache = $ENV:NUGET_PACKAGES
    if ($null -eq $packageCache) {
        $packageCache = [System.IO.Path]::Combine($home, ".nuget", "packages")
    }
    if ($null -ne $packageCache) {
        Get-ChildItem ([System.IO.Path]::Combine($packageCache, "xunit.*")) | ForEach-Object {
            Get-ChildItem ([System.IO.Path]::Combine($_.FullName, "*-dev")) | ForEach-Object {
                Write-Host $_.FullName -ForegroundColor Yellow
                Remove-Item -Recurse -Force $_
            }
        }
    }
}

Alternatively, if you can point me to instructions on enabling this preview Test Explorer feature (assuming I can do so), I could test this integration myself.

up to you if you want to start without the current dotnet test support

I agree that shipping what I have right now to end users doesn't buy them anything, because (a) there's no dotnet test support (because we're incompatible with Microsoft.Testing.Platform.MSBuild), and (b) there's no Test Explorer support (because you haven't shipped that publicly yet).

I'd be willing to ship this as-is to help your team use us as a second implementation (that does not use VSTestBridge) to verify that everything has been documented and working from the core protocol/API perspective.

Obviously it requires either implementing my or your suggestion on how to work around my problems today with needing GenerateTestingPlatformEntryPoint set to true then causing our auto-generated entry points to collide. I would also, for the time being, need to continue looking for --internal-msbuild-node, at least until dotnet test support is moved over to using --server.

the others deps that we ship by-default (extensions like codecoverage, trx and telemetry)

Part of the end user documentation of this experimental feature could be to not only say "you need to turn on TestingPlatformDotnetTestSupport" but also "the VSTest team recommends you enable these extensions" (or even "here are the known extensions today, and what registering them gives you", which is something I'm not 100% clear on myself in all cases, since there are more extensions available than just those default 3).

I could give them a simple copy/paste block to add those three defaults, which is a sneaky way to ensure they were using a version that's compatible (if we end up needing versions without the conditional on GenerateTestingPlatformEntryPoint). If it turns out that we just shift the requirements over into newer versions of Microsoft.Testing.Platform and/or Microsoft.Testing.Platform.MSBuild, then so much the easier for the end user, because those references would already be coming via xunit.v3.core.

Later on, when this becomes non-experimental, it would make sense for us to update our project templates for v3 to include these extensions by default (much like the v2 templates today include coverlet, as a way to document "here's how you enable this" in a way that users can opt out of by simply deleting the dependencies they don't care about).

How permanent is TestingPlatformDotnetTestSupport? Is this something that you envision existing even after your first official dotnet test support is shipped, or does it exist today because you're still evolving all that support and consider it experimental?

you opt-out our entry point but we always emit a full registration method kinda like: [...] In this way we can evolve the extensions infra without the need to bother you too much and you don't have to implement the logic by your own.

I think is is better than my suggestion, because it leaves the ownership of the discovery and ordering (and whatever else you might need in the future) to you, rather than duplicated by me. You can auto-generate this static method in a well known namespace & class, and I could push a reference to it (as a lambda) into my existing compiled TestPlatformTestFramework entry point rather than the array of typeofs. That means I can get rid of my code-gen-as-MSBuild-task and go back to a simple template.

@bradwilson
Copy link
Author

bradwilson commented Aug 8, 2024

I've updated my branch, and with this update, I can use dotnet test (if you ignore the entry point conflict warning, which thankfully we win):

image

If you don't set TestingPlatformDotnetTestSupport then of course we get failures related to a lack of TPv2 support (for that you'd need to bring in xunit.runner.visualstudio and Microsoft.NET.Test.Sdk), but this seems like we're pretty much in alignment now and just in need of the auto-generated registration method instead of entry point and this will be ready for users to try out. 🤞🏼

@MarcoRossignoli
Copy link
Contributor

I'm not talking about what the user sees with dotnet test --help, but rather what the user sees with dotnet run -- -?, though I understand if we were using you to output the latter, then what we registered in terms of options would impact the former, and that potentially complicates things.

We can add the alias --? to our --help I think it's fine. #3501

At the moment I think of dotnet test as falling into the "third party multi-assembly runner" pattern (just like Test Explorer or Resharper or NCrunch or CodeRush) rather than the dotnet run "first party single-assembly runner" experience. I don't expect multi-assembly runners to have our same UX, but I would like our single-assembly runner experience to be as near to identical in UX as our multi-assembly runner experience (i.e., xunit.v3.runner.console). For ease of migration, I don't want to radically change that UX from v2 to v3 (not to mention I quite like our UX here, for the most part).

Ok I think make sense.

The interesting problem with seed, specifically, is that it's a per-assembly value. I don't think there's any way to run dotnet test and have it run multiple assemblies (and/or multiple target frameworks, which do end up with unique seed values), but then allow the user to say "the seed for assembly A in net472 is 123, but the seed for assembly B in net6.0 is 456". That's probably a degenerate case that doesn't need to be dealt with, though: if the user is asking to run a specific seed, then they're trying to repro a problem with a specific assembly.

True, we forward all param to all underneath test modules, if you output the seed on console with the output users can repro or am I out of track?

A related question: how will these command line options be surfaced into Test Explorer, if at all? To use the seed example, would I be able to set a seed for a specific test assembly (at a specific target framework) with some UI gesture in Test Explorer?

At the moment we have fixed way to startup the modules in Test Explorer, but we discussed the concept of profile file (TBD if use VS or VS Code or custom launcher format) where users can create a profile file in the project self and decide which one to run, but this needs some UX updates. cc: @drognanar

For the moment, since we're trying to achieve the ability for me to still own my entry point, I don't think we need to open any other issues. I'd personally prefer for me to continue to own my entry point, at which point it becomes unnecessary for me to own the banner or anything else like that (since it would only be seen in the case of the user running dotnet test, and not in the case of the user directly running the assembly or running it via xunit.v3.runner.console).

I wonder if you want to support also our runner without dotnet test in case users doesn't have it in their machine and they want to use testing platform extension, I dunno something like --use-testing-platform or something like that. But up to you if you want to wait users feedback for that.

Personally I'm fine with --server, because it doesn't match the pattern we use for command line options anyways (we converted from /option to -option in 2.x for cross platform reasons, and we did not adopt the --option pattern).
I'm also fine if it turns out that some other test framework wants this hidden option, we can look for that hidden option instead

Ok let's start with --server

Alternatively, if you can point me to instructions on enabling this preview Test Explorer feature (assuming I can do so), I could test this integration myself.

I don't think it's shipped in public yet so I need to test it by my own, @drognanar can you confirm that's only internal for now?

You should be able to test this with my branch right now. All I do is run ./build packages in the main branch of xunit/xunit and it'll make *-dev packages. You use nuget.config to point at the /artifacts/packages folder in the clone to pull the package locally.

I did it quickly yesterday the clone I got some sec issue with 9 preview so I need to add a global.json I suppose with 8. Have you tried to build with 9 preview?

I'd be willing to ship this as-is to help your team use us as a second implementation (that does not use VSTestBridge) to verify that everything has been documented and working from the core protocol/API perspective.

That's great help for use you're not using the bridge so we can fold the difference at protocol level inside the Test Explorer(VSTestBridge is adding some custom vstest.* info for VS that we expect to fold).

Obviously it requires either implementing my or your suggestion on how to work around my problems today with needing GenerateTestingPlatformEntryPoint set to true then causing our auto-generated entry points to collide. I would also, for the time being, need to continue looking for --internal-msbuild-node, at least until dotnet test support is moved over to using --server.
@Evangelink will work on the idea to emit a file with the static method for the "sniffed" static registration soon, so you can set our generation to false and emit our "registration method". I agree also on the current logic to indexof--internal-msbuild-node`.

Part of the end user documentation of this experimental feature could be to not only say "you need to turn on TestingPlatformDotnetTestSupport" but also "the VSTest team recommends you enable these extensions" (or even "here are the known extensions today, and what registering them gives you", which is something I'm not 100% clear on myself in all cases, since there are more extensions available than just those default 3).

You can point that part of the docs to our extension list where there's the description and the setup https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-platform-extensions in that case you can avoid to add any extension by default users can start adding manually in their csproj. After you can decide if implement an SDK(imo great clarity for users, they see their csproj completely empty and with MSBuild knobs you can really change everything without any confusion on what packages needs to be added or remove or needs of complex porting documentation, but it's my opinion).

I could give them a simple copy/paste block to add those three defaults, which is a sneaky way to ensure they were using a version that's compatible (if we end up needing versions without the conditional on GenerateTestingPlatformEntryPoint). If it turns out that we just shift the requirements over into newer versions of Microsoft.Testing.Platform and/or Microsoft.Testing.Platform.MSBuild, then so much the easier for the end user, because those references would already be coming via xunit.v3.core.

We will remove the conditional and offer the extension registration you can track this label https://github.com/microsoft/testfx/issues?q=is%3Aissue+is%3Aopen+label%3Axunit

Later on, when this becomes non-experimental, it would make sense for us to update our project templates for v3 to include these extensions by default (much like the v2 templates today include coverlet, as a way to document "here's how you enable this" in a way that users can opt out of by simply deleting the dependencies they don't care about).

Up to you

How permanent is TestingPlatformDotnetTestSupport? Is this something that you envision existing even after your first official dotnet test support is shipped, or does it exist today because you're still evolving all that support and consider it experimental?

I'd says that they will coexist for at least 3/6(worst case) months after our dotnet test release.
The new test will be opted in using a global.json for the whole solution and in future it should become the "default" when enough users will move out VSTest. But as you know it's a long run.
So before to "deprecate" the current "MSBuild" dotnet test we need feedbacks and we cannot plan with strict ETA so I prefer be pessimistic.

I think is is better than my suggestion, because it leaves the ownership of the discovery and ordering (and whatever else you might need in the future) to you, rather than duplicated by me. You can auto-generate this static method in a well known namespace & class, and I could push a reference to it (as a lambda) into my existing compiled TestPlatformTestFramework entry point rather than the array of typeofs. That means I can get rid of my code-gen-as-MSBuild-task and go back to a simple template.

We have discussed with @Evangelink today about it #3494

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 8, 2024

If you don't set TestingPlatformDotnetTestSupport then of course we get failures related to a lack of TPv2 support (for that you'd need to bring in xunit.runner.visualstudio and Microsoft.NET.Test.Sdk), but this seems like we're pretty much in alignment now and just in need of the auto-generated registration method instead of entry point and this will be ready for users to try out. 🤞🏼

I think that the idea to drop VSBridge/VSTest for v3 will be also useful for us to understand if something is missing to "deprecate" VSTest for .NET. We don't have the luxury to drop it with MSTest yet and xunit is great help to understand what we really need to deprecate the glorious but a bit tired VSTest.

@bradwilson
Copy link
Author

I wonder if you want to support also our runner without dotnet test in case users doesn't have it in their machine and they want to use testing platform extension, I dunno something like --use-testing-platform or something like that.

Yep, this is a good idea. Right now our only way to "push" into your command line parser is by picking up on --server (or --internal-msbuild-node), which does not give them the opportunity to use you appropriately.

In order to implement this and make it usable, we'll also need to make sure we register our command line options (we don't do that yet).

Have you tried to build with 9 preview?

No, I generally try to keep my machine free of preview builds, and I also try not to be overly prescriptive with global.json for local development. The CI machine installs 8 and we use a global.json that we swap in there for stricter control in that environment.

Since our minimum platform right now is .NET 6, the only reason we'd push to .NET SDK 8 is to take advantage of newer compiler features. I'll fire up a Linux machine with a .NET SDK 9 preview and see what the breaking changes are.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 8, 2024

--use-testing-platform

A cleaner idea could be also to allow users to specify in their project <UseMicrosoftTestingPlatformRunner> and you emit a simple static class with a boolean that you can check in your else branch inside the entry point, so it's more "natural" for users to select the mode. Clearly the order of evaluation is always --server else xunitrunner or testing platform.

So they can opt-in it globally using directory props.

@bradwilson
Copy link
Author

bradwilson commented Aug 8, 2024

Done: xunit/xunit@b7806ed

image

@bradwilson
Copy link
Author

bradwilson commented Aug 8, 2024

I did a logic flip: instead of checking for --server and then running me if not, I check for -automated (or our response file indicator @@) and then run you if not. This is so that we can still run with our multi-assembly runners:

image

@MarcoRossignoli
Copy link
Contributor

I did a logic flip: instead of checking for --server and then running me if not, I check for -automated (or our response file indicator @@) and then run you if not. This is so that we can still run with our multi-assembly runners:

Great, one detail that should not affect the entry point but I share so we can co-think about it, the testing platform is written to be Single module deploy it means that to support single file/aot with extensions we cannot have a build with "more than one file" in these formats. This means that for instance if a user register a native aot out of process extension we "restart" ourself passing the original parameter plus some more parameter to handle the information between the the test host controller(out of process that can observe the process that hosts the run of the tests) and the test host(where the adapter run the tests).

I think that this should not affect anything anyway you will forward to the correct branch due to the fact that we don't "add" -automated or @@. I wonder anyway if in future it's possible that someone will ship some extension with --automated command line and we will clash. For now I think we can go as-is, but we need to sleep a bit on it.

@MarcoRossignoli
Copy link
Contributor

FYI you can build against our dev feed till we will reach a good integration so we can verify the progress https://github.com/microsoft/testfx/blob/main/docs/preview.md

@bradwilson
Copy link
Author

I wonder anyway if in future it's possible that someone will ship some extension with --automated command line and we will clash.

At the moment we look for exactly -automated and --automated wouldn't match. All the calls through from the multi-assembly runners strictly use the @@ path because the out-of-process launcher always creates a response file: https://github.com/xunit/xunit/blob/d1331155eba56ca99b4c25fe318eeba2c37a77ad/src/xunit.v3.runner.utility/Frameworks/v3/Xunit3.cs#L373

I included -automated on the idea that something other than our runner utility code might be trying to automate things, although that's purely hypothetical at this point so early in the lifetime of v3, and I wouldn't want to break those automations. We do document -automated but we don't automate the response file pattern; I don't want to particularly mandate the response file pattern, because that's less convenient from a scripting perspective.

I'll have to think about whether there is concern about the -automated clash, and whether it's worth adding something guaranteed not to conflict (like something with xunit in the name, or a GUID, or something like that).

to support single file/aot with extensions we cannot have a build with "more than one file" in these formats

At the moment we are not planning to support AOT. I've had some discussions about what would be required to make this happen but it is not anywhere near the top of the backlog.

@MarcoRossignoli MarcoRossignoli pinned this issue Aug 10, 2024
@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 10, 2024

I'll have to think about whether there is concern about the -automated clash, and whether it's worth adding something guaranteed not to conflict (like something with xunit in the name, or a GUID, or something like that).

Ok let's sleep on it

At the moment we are not planning to support AOT. I've had some discussions about what would be required to make this happen but it is not anywhere near the top of the backlog.

Ack

@bradwilson
Copy link
Author

The new properties are on main and 3.6 branch today

I've pushed up setting these properties. You should be able to validate it on your end now with your fixed Test Explorer. Just add a test that calls TestContext.Current.TestOutputHelper?.WriteLine("...") and the output should show.

@bradwilson
Copy link
Author

I should be done with all the tests now, so unless we track down any more issues, I think I'm basically just waiting for the 1.4 release later this month. 😄

I'm assuming the VS 2022 17.12 final release will probably coincide with .NET 9 in November.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 4, 2024

I should be done with all the tests now, so unless we track down any more issues, I think I'm basically just waiting for the 1.4 release later this month. 😄

That's great, I think we're done here for now, we will release the 1.4 soon cc: @Evangelink

I'm assuming the VS 2022 17.12 final release will probably coincide with .NET 9 in November.

This is TBD we didn't complete the VS side work yet we're close but not yet there so it's also possible that we will have another preview round for 17.13 and have it as "public preview toggle" in 17.13 GA. As you can image a lot of users are using the test explorer so we need to progress with caution. cc: @pavelhorak

Thanks for your effort here @bradwilson and feel free to suggest feature or improvement if you something!

@bradwilson
Copy link
Author

I can't think of anything for the Platform for the moment.

We already have the other issue discussing the "Not Run" state, but I think from a UX perspective in Test Explorer we're in the same place we were before and a custom state is not necessary there. It would be nice to be able to alert the user to the "not run" test in the summary, but that's still a fairly low priority, IMO (and if xUnit.net users care about that, they can use our CLI UX and/or our reports for it, since we'll put it in the report even if the MTP CLI UX doesn't).

I was going to ask whether you thought I should enable TestingPlatformServer by default or not when shipping this. I was having the same thoughts about "proceeding with caution", but I think if Test Explorer is going to end up opt-in to start, then leaving TestingPlatformServer on by default is probably safe. In our case, if we disabled it by default then practically speaking nobody would use it because they wouldn't know to seek it out, and Test Explorer would still "work" because of TPv2 support. So for now my existing plan to offer an opt out (via $(XunitDisableTestingPlatformServer)') is likely the correct answer, and very unlikely to be used. 🤞🏼

@bradwilson
Copy link
Author

From a release schedule perspective, do you know when you plan to release the newer/final version of dotnet test? I assume 1.4 will still be using the "MSBuild switcheroo" or whatever you call that technique. 😄

@thomhurst
Copy link
Contributor

I haven't tested, but in theory couldn't you just push a new test node with the DiscoveredState on it? Might cause the test explorer to replace in progress with not run?

@bradwilson
Copy link
Author

To be clear: Test Explorer is behaving exactly how I want it to behave now. I just want to make sure I'm not breaking anything by telling the Platform that I started running a test but never telling it that it finished.

@bradwilson
Copy link
Author

bradwilson commented Sep 4, 2024

Also, I'd find it surprising that the Platform wouldn't find the behavior of giving a discovery result during an execution request more unexpected than just starting but never finishing a test. 😂

@bradwilson
Copy link
Author

bradwilson commented Sep 4, 2024

I do have a problem that I need to look at: debugging seems to be broken in VS Code with the current source, but it's working as expected in Visual Studio. (This is TPv2, not Microsoft.Testing.Platform.) Who can I talk to about getting help debugging what's going on?

I've fixed it with 3.0.0-pre.31.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 5, 2024

We already have the other issue discussing the "Not Run" state, but I think from a UX perspective in Test Explorer we're in the same place we were before and a custom state is not necessary there. It would be nice to be able to alert the user to the "not run" test in the summary, but that's still a fairly low priority, IMO (and if xUnit.net users care about that, they can use our CLI UX and/or our reports for it, since we'll put it in the report even if the MTP CLI UX doesn't).

Yep for this one let's use the other issue it's a Text Explorer use case unrelated to the testing platform, despite we could add a new state if the UX cannot handle it it's useless at the moment.

I was going to ask whether you thought I should enable TestingPlatformServer by default or not when shipping this. I was having the same thoughts about "proceeding with caution", but I think if Test Explorer is going to end up opt-in to start, then leaving TestingPlatformServer on by default is probably safe. In our case, if we disabled it by default then practically speaking nobody would use it because they wouldn't know to seek it out, and Test Explorer would still "work" because of TPv2 support. So for now my existing plan to offer an opt out (via $(XunitDisableTestingPlatformServer)') is likely the correct answer, and very unlikely to be used. 🤞🏼

We will add the opt-in flag in the preview box...so the idea is if the flag is opted-in plus you expose TestingPlatformServer we go for it. cc: @drognanar if my expectation is wrong.

From a release schedule perspective, do you know when you plan to release the newer/final version of dotnet test? I assume 1.4 will still be using the "MSBuild switcheroo" or whatever you call that technique. 😄

Yep, we call it "MSBuild VSTest task workflow"(the name of the built-in target to plug to). We don't have ETA yet about the release of the new dotnet test, we're not far from the first preview anyway, I think we need another 1/2 months to be there with something "usable as preview" cc: @mariam-abdulla that's driving this effort.

I haven't tested, but in theory couldn't you just push a new test node with the DiscoveredState on it? Might cause the test explorer to replace in progress with not run?

@thomhurst if we want to add this new "state" and we think it's something that make broadly sense for adapters as a known shared scenario I prefer to have a specific state for it. We should not base states on Test Explorer, if we need we will go amend there the recognition of this state.

Also, I'd find it surprising that the Platform wouldn't find the behavior of giving a discovery result during an execution request more unexpected than just starting but never finishing a test. 😂

At the moment "the platform" is nothing more than a IPlatformOutputDevice that collects an print in console or in server mode sent states outside. We don't have validation on what adapters are sending. We could think to add a "strict" verification of the state of every test node at least from the perspective of the lifecycle expected by the platform "at minimum"(you can push what you want if other extensions understand your shared model, like trx), like "discovered(non mandatory) -> in-progress(not mandatory) -> executionstate(mandatory)" rejecting with an exception if you push out of this expected state. cc: @Evangelink

Ticket opened for discussion for the next iteration #3759

Also, I'd find it surprising that the Platform wouldn't find the behavior of giving a discovery result during an execution request more unexpected than just starting but never finishing a test. 😂

@drognanar can you help here? Is it a problem for the current Test Explorer expectations?

@thomhurst
Copy link
Contributor

@thomhurst if we want to add this new "state" and we think it's something that make broadly sense for adapters as a known shared scenario I prefer to have a specific state for it. We should not base states on Test Explorer, if we need we will go amend there the recognition of this state.

I've opened #3699 around an Inconclusive state. There's discussions around it being similar to Skipped, but I see Skipped as a user decision. But keen to hear other people's thoughts.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 5, 2024

I'm going to close this issue because we're done with the "first integration wave" for xunit and this ticket is needed to complete the 1.4 milestone.
Feel free to open new "specific" issues since now.

cc: @Evangelink @nohwnd

@bradwilson
Copy link
Author

FYI, I'm adding --report-xunit-trx because my version will include "not run" tests properly in the report (as NotRunnable).

@Evangelink
Copy link
Member

@bradwilson Thank you for the nice collaboration! Looking forward for your alpha/beta/preview and for the next steps of our collaboration.

I was off for a week, I am catching up with notifications and will be reading/replying to all points as I move through them.

@Evangelink Evangelink unpinned this issue Sep 10, 2024
@MarcoRossignoli
Copy link
Contributor

@bradwilson we've shipped official 1.4.0 you can take it to complete and merge the PR if you want https://www.nuget.org/packages/Microsoft.Testing.Platform/1.4.0 it includes our integration work.

@bradwilson
Copy link
Author

My branch is merged, and the build is available as 0.4.0-pre.10 https://xunit.net/docs/using-ci-builds

Minions celebration

I've created a documentation page: https://xunit.net/docs/getting-started/v3/microsoft-testing-platform

The documentation page is not yet linked from the home page, because I wanted to give your team a chance to review it regarding language/links/etc. and see if there are any changes you'd like to see. You don't have to be concerned with the xUnit.net technical details; you can just assume I got them right. 😂 But obviously if I got any of the technical details about MTP wrong, please let me know!

Once the documentation is good, then you can update https://learn.microsoft.com/dotnet/core/testing/unit-testing-platform-intro with a link to the doc page.

I won't make a full announcement until I ship the next prerelease build to NuGet. I probably won't do that until I finish the filter query stuff. I'm waiting on feedback from @Evangelink here before I finalize my implementation decisions, as I'd like to try to match the MSTest implementation as close as is reasonable.

@MarcoRossignoli
Copy link
Contributor

I'll read the doc and I'll give it a try and after I'll link to our docs.

image

@nohwnd
Copy link
Member

nohwnd commented Sep 13, 2024

Same here, I've read the docs, and checked most of the links and all looks good. Great job :)

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 13, 2024

So I've read and did all the sample and it's all good and working as expected, one nit is the output of the new dotnet test experience, we did some change to be close to the current VSTest terminal logger implementation and now the ux is something like:

PS C:\....\xunitnew> dotnet test
Restore complete (0.7s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  xunitnew succeeded (3.8s) → bin\Debug\net8.0\xunitnew.dll
  xunitnew test failed with 1 error(s) (0.8s)
    C:\...\xunitnew\bin\Debug\net8.0\xunitnew.dll : error run failed: Tests failed: 'C:\...\xunitnew\bin\Debug\net8.0\TestResults\xunitnew_net8.0_x64.log' [net8.0|x64]

Test summary: total: 1, failed: 1, succeeded: 0, skipped: 0, duration: 0.8s
Build failed with 1 error(s) in 5.9s

Anyway I think it's not so important, it's possible that we will update at every version till steady state so I think it can stay as-is. I'll link the doc to our docs.

PR out dotnet/docs#42614

@bradwilson
Copy link
Author

Yeah, the difference is .NET SDK 8 vs. 9. I opted to include the output from 8 since 9 is still prerelease.

@bradwilson
Copy link
Author

Sounds like the docs are good to go, so I just pushed up the link from the home page. If you have any feedback, feel free to leave it here (or you can submit a PR to the gh-pages branch).

@MarcoRossignoli
Copy link
Contributor

Thanks!

@bradwilson
Copy link
Author

FYI, we have just officially shipped 0.4.0-pre.20 to NuGet which includes the Microsoft Testing Platform enablement. https://xunit.net/releases/v3/0.4.0-pre.20

@MarcoRossignoli
Copy link
Contributor

Amazing thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library Area: xUnit Type: Discussion
Projects
None yet
Development

No branches or pull requests

6 participants