More MSBuild cleanup

[Youssef1313]

No description provided.

[nohwnd]

cannot add a suggestion here, but imo we should not define the property at all, and let the consumer (adapter / framework) opt-in (or opt out) from MTP by defining the property in their own terms.

e.g. as mstest does it mstest.testadapter.targets

 <EnableMSTestRunner Condition=" '$(EnableMSTestRunner)' == '' ">false</EnableMSTestRunner>
 <IsTestingPlatformApplication>$(EnableMSTestRunner)</IsTestingPlatformApplication>

This way we avoid relying on IsTestingPlatformApplication being true, before the adapter gets a chance to set it.
e.g. by incorrectly adding set of properties to our msbuild targets, that check for IsTestingPlatformApplication .

And we also avoid seeing property re-assignement in binary log, because the property won't be set twice.

This will make IsTestingPlatformApplication the global opt-in. And will let frameworks to further opt-out (or opt-in, but we dont' have such capability yet), to other features of mtp. e.g. like this

this is MSTest specific way to enable MTP
<EnableMSTestRunner Condition=" '$(EnableMSTestRunner)' == '' ">false</EnableMSTestRunner>

this is MSTest enabling MTP
 <IsTestingPlatformApplication>$(EnableMSTestRunner)</IsTestingPlatformApplication>
 
 This is further customization that MSTest can do if it does not like the defaults that are set by MTP
<DisableTestingPlatformServerCapability Condition=" '$(EnableMSTestRunner)' == 'false' " >true</DisableTestingPlatformServerCapability>

this is yet another (non-existing) capability that mstest does not like and wants to change the default

<AddTestingPlatformSupportForExternalExtensionsPaths>false</AddTestingPlatformSupportForExternalExtensionsPaths>

[Youssef1313]

All reads of IsTestingPlatformApplication should happen either in a target, or in as a condition of an item. So the value read is always correct. We still default to true to simplify cases that will only want to support MTP. I don't see anything technically wrong by defaulting it to true at this point. I don't feel like the re-assignment done by adapter is much of a big deal.

If there is a concern for incorrectly reading IsTestingPlatformApplication before it was set to adapters then:

1. The problem will still remain even if we removed the line you're referring. Difference is that the incorrect read value will be empty instead of "true".
2. We can have an MSBuild Check that warns when the value is read outside of a target and not part of an item condition.

[nohwnd]

1. to me seeing the value as empty is preferable, because then you know it is undefined, and you don't blindly rely on the value being set and being true (as the code did before both your refactorings).
2. yes this is for sure the best solution, as long as we can apply it only to our targets and not all targets, because the targets of the adapter itself does not have that limitation. :)

but you do you, I am just searching for places where it can go wrong =)