Fix msbuild property order, to make IsTestingPlatformApplication true, and make it global kill switch

[nohwnd]

Describe the bug

IsTestingPlatformApplication is re-assigned to false, making it difficult to use. And not acting as global opt-in.

Steps To Reproduce

The IsTestingPlatformApplication MSBuild property seems to be broken. It works for Mstest because all the props that IsTestingPlatformApplication splits into are being set again explicitly by mstest target.

This property not working is problematic for additional frameworks that wish to opt in (like NUnit) which then have to know too much about the internals. and cannot simply add single property to opt into the new testing platform. Instead the have to re-define every switch we provide.

The cause is these props being set in .props, so they are included and evaluated before the user has a chance to set them.

So IsTestingPlatformApplication will be set to false, and all the following props that use it as condition, e.g. this
are not included.

We don't see this as problematic in mstest because we do all the same work again but conditioning it to EnableMSTestRunner, and xunit and nunit do the same, because we fixed them that way.

We cannot move the IsTestingPlatformApplication property to targets because it would be evaluated too late:

Adapter.props
MSBuild.props
csproj
MSBuild.targets
Adapter.targets --> So too late because MSBuild.targets was already evaluated

This is because of package deps. Adapter -> MSBuild so Adapter is resolved first/last

so that means that we cannot use the IsTestingPlatformApplication property as a condition for property groups. And so we cannot use it to define the default values for the dependent props.

so there are 2 options.

1. require the testing frameworks to specify the properties in targets, and set all of them to true or false, this puts the burden of knowing all the knobs on the framework

2. ask testing frameworks to move their property to targets, and only require to set IsTestingPlatformApplication to true, which would enable all features for them, and then they can opt out from e.g. GenerateTestingPlatformEntryPoint by setting it to false.

And we would need to remove the property groups conditioned from our targets, they do nothing and are confusing. and change these conditions

that way the IsTestingPlatformApplication will remain the single global knob, and we can add more options that the frameworks can opt out from, but that they will have enabled by default

option 3) is like option 2, but we set all the values to true in msbuild.props, and add the istestingplatform to all conditions that use the opt-out knobs. so IsTestingPlatformApplication remains the global kill switch. This has "disadvantage of always seeing property re-assignment in the binlog, when run is not enabling TP or some feature

Expected behavior

IsTestingPlatformApplication is a single switch to opt into testing platform, and it will remain true when enabled.

[nohwnd]

Option 2 is the best fix here.

[Youssef1313]

IsTestingPlatformApplication is refactored on current main. @nohwnd Can you verify if there are issues on current main related to this?

[nohwnd]

specificvally, refactored here: #5021

[Youssef1313]

Yup. Assigning to you for now to verify if this issue is fixed or not.
If not, I'll let you add some details per the current implementation on main, and then feel free to hand it back to me again.

[nohwnd]

To me this is not changing the default, and should be tied to the IsTestingPlatformApplication by default. And named in the positive way, much like the rest of the opt-ins.

https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/buildTransitive/common/MSTest.TestAdapter.targets#L12

that is:

if (is testing platform app)
enable server capability = true
else
enable server capability = false

a small issue

https://github.com/microsoft/testfx/blob/main/src/Adapter/MSTest.TestAdapter/buildTransitive/common/MSTest.TestAdapter.targets#L6 at this place we re-assign the property. To me it feels more logical if IsTestingPlatformApplication is a known property that the adapter sets to opt-into testing platform.

Because then we don't end up setting it too early in the build process, and rely on it being true, before the adapter opts-out.

[nohwnd]

Further fixes in #5056

[nohwnd]

After discussion here: #5056 (comment)

I am pretty happy to close this.

Only follow up needed is removing the additional properties in nunit (and maybe xunit, I am not 100% sure about the situation there). I am making the PRs.

[Youssef1313]

Only follow up needed is removing the additional properties in nunit (and maybe xunit, I am not 100% sure about the situation there). I am making the PRs.

Latest stable of xunit.v3.core is not using latest MTP. So making cleanup there will only be viable if they do update. Similar for NUnit

[Youssef1313]

I am pretty happy to close this.

Closing out then!