Avoid setting IsTestingPlatformApplication in MTP.targets#7566
Avoid setting IsTestingPlatformApplication in MTP.targets#7566Youssef1313 wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This change updates the MSBuild integration for Microsoft.Testing.Platform by removing the default assignment of the IsTestingPlatformApplication property from the platform’s .targets file.
Changes:
- Removed the conditional default (
truewhen unset) forIsTestingPlatformApplicationfromMicrosoft.Testing.Platform.targets.
You can also share your feedback on Copilot code review. Take the survey.
src/Platform/Microsoft.Testing.Platform/buildMultiTargeting/Microsoft.Testing.Platform.targets
Show resolved
Hide resolved
|
Ping @bradwilson. This is technically a breaking change that's expected to have only an impact for xunit.v3 which doesn't have a "gated" property for choosing between VSTest/MTP. If this is merged, what will happen if you want to preserve the current behavior is:
If nothing is done, then the most likely visible impact will be for .NET 10 SDK users using the MTP-based test command, which will now not recognize xunit.v3 test projects as MTP. |
|
We will need to update MTP integration tests here, but I'm deferring that until we decide whether we will move forward with the change. |
|
Since this is a breaking change, are you major version bumping? It's an easier story to tell people who break themselves with a major version upgrade than a minor. |
That's the main reason for explicitly asking you upfront :) As you know, having close major bumps is not liked much around here. It's also a particular problem as you generate new package names for every new version of MTP (it actually should not be but you know people). We were trying to see if there would be something from xUnit we could use to preserve the mode for you but we haven't really found anything relevant. |
There are no MSBuild properties for you to hang this off that I can think of. Everything we have is optional. |
|
And how big deal the breaking change will be for you? Maybe to reduce the impact, you can start adding |
If MTP suddenly stops working with no reasoning why? That seems pretty big.
Sure, that's no problem.
I honestly have no way of determining this, other than trying to watch and interpret changes in download counts, something I do not do now (nor have any tools for automating at the moment). |
|
An additional factor I have no insight into is how many people choose to include a newer version of MTP vs. the number of people who get what comes in-box by default. |
|
@bradwilson I will see if we have some more precise info from our telemetry or nuget telemetry, if so I'll ask what I can share with you (by email if that's ok). |
Is this just for MTP, or also for VSTest? (by name, it seems MTP specific, but just wanted to validate) |
|
Yes? diff --git a/src/xunit.v3.core/Package/buildTransitive/xunit.v3.core.props b/src/xunit.v3.core/Package/buildTransitive/xunit.v3.core.props
index 113d405a..fd926d4c 100644
--- a/src/xunit.v3.core/Package/buildTransitive/xunit.v3.core.props
+++ b/src/xunit.v3.core/Package/buildTransitive/xunit.v3.core.props
@@ -8,6 +8,8 @@
<GenerateSelfRegisteredExtensions>true</GenerateSelfRegisteredExtensions>
<!-- Turn off Microsoft.Testing.Platform.MSBuild's auto-generated entrypoint -->
<GenerateTestingPlatformEntryPoint>false</GenerateTestingPlatformEntryPoint>
+ <!-- Enable 'dotnet test' support for Microsoft.Testing.Platform -->
+ <IsTestingPlatformApplication Condition=" '$(IsTestingPlatformApplication)' == '' ">true</IsTestingPlatformApplication>
<TestProject>true</TestProject>
<!-- Force the app host for .NET builds, since it's required for xunit.v3.runner.utility to run v3 projects -->
<UseAppHost>true</UseAppHost> |
|
Out of curiosity, doesn't you moving Edit: It looks like maybe you moved it in 1.6.3, as it used to be in |
See discussion in #7557
In short, we should leave this up to test frameworks.