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.

Sign up for GitHub

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

More MSBuild cleanup #5056

Merged
merged 2 commits into from
Feb 18, 2025
+24 −30
Merged

More MSBuild cleanup #5056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a630c30
More MSBuild cleanup
Youssef1313 Feb 18, 2025
ab3723f
fix test
Youssef1313 Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Adapter/MSTest.TestAdapter/buildTransitive/common/MSTest.TestAdapter.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
<IsTestingPlatformApplication>$(EnableMSTestRunner)</IsTestingPlatformApplication>

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

<!-- While this targets file is imported *after* MTP.MSBuild.targets, it's still valid to set the property at this point -->
<!-- This is because items are evaluated after properties, and we use this property to control an item. -->
<DisableTestingPlatformServerCapability Condition=" '$(EnableMSTestRunner)' == 'false' or '$(EnableMSTestRunner)' == '' " >true</DisableTestingPlatformServerCapability>
</PropertyGroup>

<PropertyGroup Condition=" '$(UseWinUI)' == 'true' ">
Expand Down
48 changes: 23 additions & 25 deletions ...t.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,24 @@

<PropertyGroup>
<!-- This is the knob to disable completely for this project the machinery -->
<IsTestingPlatformApplication Condition=" '$(IsTestingPlatformApplication)' == '' ">True</IsTestingPlatformApplication>
<IsTestingPlatformApplication Condition=" '$(IsTestingPlatformApplication)' == '' ">true</IsTestingPlatformApplication>
Comment on lines 5 to +6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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 =)

</PropertyGroup>

<!-- Declare our capability to Visual Studio/Visual Studio Code -->
<!-- Note: DisableTestingPlatformServerCapability may be set by TestAdapter targets -->
<!-- Note: DisableTestingPlatformServerCapability or IsTestingPlatformApplication may be set by TestAdapter targets -->
<!-- While TestAdapter targets file is imported *after* this file, MSBuild evaluates properties -->
<!-- first, then items. So, we will be able to pick up the correct value here. -->
<ItemGroup Condition=" '$(DisableTestingPlatformServerCapability)' != 'True' ">
<ItemGroup Condition=" '$(DisableTestingPlatformServerCapability)' != 'true' AND '$(IsTestingPlatformApplication)' == 'true' ">
<ProjectCapability Include="TestingPlatformServer" />

<!-- Old capability expected by VS/VSCode -->
<ProjectCapability Include="TestContainer" />
</ItemGroup>

<Target Name="_AddAssemblyMetadata" BeforeTargets="GetAssemblyAttributes">
<!-- Add assembly attribute metadata for reflection usage -->
<ItemGroup>
<AssemblyMetadata Include="Microsoft.Testing.Platform.Application" Value="True" Condition=" '$(IsTestingPlatformApplication)' == 'True' " />
</ItemGroup>
</Target>
<!-- Add assembly attribute metadata for reflection usage -->
<ItemGroup>
<AssemblyMetadata Include="Microsoft.Testing.Platform.Application" Value="true" Condition=" '$(IsTestingPlatformApplication)' == 'true' " />
</ItemGroup>

<!-- Setup the task folder -->
<PropertyGroup>
Expand Down Expand Up @@ -67,7 +65,7 @@
<Target Name="_GenerateTestingPlatformConfigurationFileCore"
Inputs="$(_TestingPlatformConfigurationFileSourcePath)"
Outputs="$(_TestingPlatformConfigurationFile)"
Condition=" '$(GenerateTestingPlatformConfigurationFile)' == 'True' And Exists('$(_TestingPlatformConfigurationFileSourcePath)') " >
Condition=" '$(GenerateTestingPlatformConfigurationFile)' == 'true' And Exists('$(_TestingPlatformConfigurationFileSourcePath)') " >
<ConfigurationFileTask
MSBuildProjectDirectory="$(MSBuildProjectDirectory)"
AssemblyName="$(AssemblyName)"
Expand Down Expand Up @@ -126,8 +124,8 @@
<!-- Write the hash value to the cache file -->
<WriteLinesToFile Lines="$(_GenerateTestingPlatformEntryPointInputsCache)"
File="$(_GenerateTestingPlatformEntryPointInputsCachFilePath)"
Overwrite="True"
WriteOnlyWhenDifferent="True" />
Overwrite="true"
WriteOnlyWhenDifferent="true" />
<ItemGroup>
<FileWrites Include="$(_GenerateTestingPlatformEntryPointInputsCachFilePath)" />
</ItemGroup>
Expand All @@ -141,7 +139,7 @@
</PropertyGroup>
<Target Name="_GenerateTestingPlatformEntryPoint"
Inputs="@(GenerateTestingPlatformEntryPointInputsCacheFilePath)" Outputs="$(_TestingPlatformEntryPointSourcePath)"
Condition=" '$(GenerateTestingPlatformEntryPoint)' == 'True' " >
Condition=" '$(GenerateTestingPlatformEntryPoint)' == 'true' " >
<TestingPlatformEntryPointTask TestingPlatformEntryPointSourcePath="$(_TestingPlatformEntryPointSourcePath)"
RootNamespace="$(RootNamespace)"
Language="$(Language)" >
Expand All @@ -156,7 +154,7 @@
<Target Name="_IncludeGenerateTestingPlatformEntryPointIntoCompilation" BeforeTargets="BeforeCompile;XamlPreCompile"
DependsOnTargets="_CalculateGenerateTestingPlatformEntryPoint;_GenerateTestingPlatformEntryPointFileInputCache;_GenerateTestingPlatformEntryPoint;_IncludeGenerateAutoRegisteredExtensionsIntoCompilation">

<ItemGroup Condition=" '$(GenerateTestingPlatformEntryPoint)' == 'True' ">
<ItemGroup Condition=" '$(GenerateTestingPlatformEntryPoint)' == 'true' ">
<Compile Include="$(_TestingPlatformEntryPointSourcePath)" />

<!-- We need to report to the FileWrites here because is possible that we skip _GenerateTestingPlatformInjectEntryPoint and we want to correctly handle the "dotnet clean" target -->
Expand Down Expand Up @@ -213,8 +211,8 @@
<!-- Write the hash value to the cache file -->
<WriteLinesToFile Lines="$(_GenerateSelfRegisteredExtensionsInputsCache)"
File="$(_GenerateSelfRegisteredExtensionsInputsCachFilePath)"
Overwrite="True"
WriteOnlyWhenDifferent="True" />
Overwrite="true"
WriteOnlyWhenDifferent="true" />
<ItemGroup>
<FileWrites Include="$(_GenerateSelfRegisteredExtensionsInputsCachFilePath)" />
</ItemGroup>
Expand All @@ -228,7 +226,7 @@
</PropertyGroup>
<Target Name="_GenerateSelfRegisteredExtensions"
Inputs="@(GenerateSelfRegisteredExtensionsInputsCacheFilePath)" Outputs="$(_SelfRegisteredExtensionsSourcePath)"
Condition=" '$(GenerateSelfRegisteredExtensions)' == 'True' " >
Condition=" '$(GenerateSelfRegisteredExtensions)' == 'true' " >
<TestingPlatformSelfRegisteredExtensions SelfRegisteredExtensionsSourcePath="$(_SelfRegisteredExtensionsSourcePath)"
SelfRegisteredExtensionsBuilderHook="@(TestingPlatformBuilderHook)"
RootNamespace="$(RootNamespace)"
Expand All @@ -245,7 +243,7 @@
<Target Name="_IncludeGenerateAutoRegisteredExtensionsIntoCompilation" BeforeTargets="BeforeCompile;XamlPreCompile"
DependsOnTargets="_CalculateGenerateSelfRegisteredExtensions;_GenerateSelfRegisteredExtensionsFileInputCache;_GenerateSelfRegisteredExtensions">

<ItemGroup Condition=" '$(GenerateSelfRegisteredExtensions)' == 'True' ">
<ItemGroup Condition=" '$(GenerateSelfRegisteredExtensions)' == 'true' ">
<Compile Include="$(_SelfRegisteredExtensionsSourcePath)" />

<!-- We need to report to the FileWrites here because is possible that we skip _GenerateSelfRegisteredExtensions and we want to correctly handle the "dotnet clean" target -->
Expand Down Expand Up @@ -276,22 +274,22 @@
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.Common.CurrentVersion.targets#L6807
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.Common.Test.targets
-->
<_MSBuildTestInfrastructureAvailable Condition="Exists('$(MSBuildExtensionsPath)\Microsoft.Common.Test.targets')" >True</_MSBuildTestInfrastructureAvailable>
<_MSBuildTestInfrastructureAvailable Condition="Exists('$(MSBuildExtensionsPath)\Microsoft.Common.Test.targets')" >true</_MSBuildTestInfrastructureAvailable>
</PropertyGroup>

<Target Name="_SetCustomVSTargetForVSTest" BeforeTargets="VSTest" Condition=" '$(TestingPlatformDotnetTestSupport)' == 'True' ">
<Target Name="_SetCustomVSTargetForVSTest" BeforeTargets="VSTest" Condition=" '$(TestingPlatformDotnetTestSupport)' == 'true' ">
<PropertyGroup>
<!-- Set our VSTest target for the multi-tfm workflow -->
<InnerVSTestTargets Condition=" '$(TestingPlatformDotnetTestSupport)' == 'True' " >InvokeTestingPlatform</InnerVSTestTargets>
<InnerVSTestTargets Condition=" '$(TestingPlatformDotnetTestSupport)' == 'true' " >InvokeTestingPlatform</InnerVSTestTargets>
</PropertyGroup>
</Target>
<!-- Override VSTest only in case of single tfm, for multitfm we rely on built-in mechanism -->
<Import Project="$(MSBuildThisFileDirectory)Microsoft.Testing.Platform.MSBuild.VSTest.targets"
Condition=" '$(TargetFramework)' != '' AND '$(TestingPlatformDotnetTestSupport)' == 'True' "/>
Condition=" '$(TargetFramework)' != '' AND '$(TestingPlatformDotnetTestSupport)' == 'true' "/>

<!-- Rely on VSTest infra -->
<!-- We setup the InnerVSTestTargets is we're using the MSBuild infrastructure or if user didn't disble the custom test target -->
<Target Name="_SetCustomVSTargetForTest" BeforeTargets="Test" Condition=" ( '$(UseMSBuildTestInfrastructure)' == 'True' AND '$(_MSBuildTestInfrastructureAvailable)' == 'True' ) OR '$(TestingPlatformDisableCustomTestTarget)' == 'False' " >
<Target Name="_SetCustomVSTargetForTest" BeforeTargets="Test" Condition=" ( '$(UseMSBuildTestInfrastructure)' == 'true' AND '$(_MSBuildTestInfrastructureAvailable)' == 'true' ) OR '$(TestingPlatformDisableCustomTestTarget)' == 'False' " >
<PropertyGroup>
<InnerVSTestTargets>InvokeTestingPlatform</InnerVSTestTargets>
</PropertyGroup>
Expand All @@ -301,7 +299,7 @@
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.Common.CurrentVersion.targets#L6807
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.Common.Test.targets
-->
<Target Name="_MSBuildTest" AfterTargets="Test" Condition=" '$(UseMSBuildTestInfrastructure)' == 'True' AND '$(_MSBuildTestInfrastructureAvailable)' == 'True' " >
<Target Name="_MSBuildTest" AfterTargets="Test" Condition=" '$(UseMSBuildTestInfrastructure)' == 'true' AND '$(_MSBuildTestInfrastructureAvailable)' == 'true' " >
<CallTarget Targets="_TestingPlatformTest" />
</Target>

Expand Down Expand Up @@ -346,7 +344,7 @@
<!-- If everyting else fails, fallback to x64. -->
<_TestArchitecture Condition="'$(_TestArchitecture)' == '' or '$(_TestArchitecture)' == 'AnyCpu'">x64</_TestArchitecture>
<TestingPlatformShowTestsFailure Condition=" '$(TestingPlatformShowTestsFailure)' == '' ">False</TestingPlatformShowTestsFailure>
<TestingPlatformCaptureOutput Condition=" '$(TestingPlatformCaptureOutput)' == '' " >True</TestingPlatformCaptureOutput>
<TestingPlatformCaptureOutput Condition=" '$(TestingPlatformCaptureOutput)' == '' " >true</TestingPlatformCaptureOutput>
</PropertyGroup>
<Target Name="InvokeTestingPlatform" >
<!--
Expand Down
Loading