Adjust IsTestProject hacks to not include MTP
#49085
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adjusts test project conditions so that projects using the Microsoft Testing Platform (MTP) are no longer treated as standard test projects. The changes introduce a new property (_IsVSTestOnlyTestProject) and update existing conditions in the targets file to use this property.
- Introduces _IsVSTestOnlyTestProject to differentiate test projects from MTP projects.
- Updates condition checks for IsRidAgnostic, ValidateExecutableReferencesMatchSelfContained, and ShouldBeValidatedAsExecutableReference.
| <_DefaultUserProfileRuntimeStorePath Condition="$([MSBuild]::IsOSPlatform(`Windows`))">$(USERPROFILE)</_DefaultUserProfileRuntimeStorePath> | ||
| <_DefaultUserProfileRuntimeStorePath>$([System.IO.Path]::Combine($(_DefaultUserProfileRuntimeStorePath), '.dotnet', 'store'))</_DefaultUserProfileRuntimeStorePath> | ||
| <UserProfileRuntimeStorePath Condition="'$(UserProfileRuntimeStorePath)' == ''">$(_DefaultUserProfileRuntimeStorePath)</UserProfileRuntimeStorePath> | ||
| <_IsVSTestOnlyTestProject Condition="'$(IsTestProject)' == 'true' AND '$(IsTestingPlatformApplication)' != 'true'">true</_IsVSTestOnlyTestProject> |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment explaining the purpose of the new _IsVSTestOnlyTestProject property, clarifying that it excludes MTP projects from test-specific conditions.
| property values from referencing projects. --> | ||
| <PropertyGroup Condition="'$(IsRidAgnostic)' == ''"> | ||
| <IsRidAgnostic Condition="('$(_IsExecutable)' == 'true' And '$(IsTestProject)' != 'true') Or | ||
| <IsRidAgnostic Condition="('$(_IsExecutable)' == 'true' And '$(_IsVSTestOnlyTestProject)' != 'true') Or |
There was a problem hiding this comment.
[nitpick] Review and update any nearby inline comments to ensure they accurately reflect the change from IsTestProject to _IsVSTestOnlyTestProject for non-MTP test projects.
|
@dsplaisted Can I have an eye on this PR please? Thanks! |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
Show resolved
Hide resolved
dsplaisted
left a comment
dsplaisted
left a comment
There was a problem hiding this comment.
Looks good, though if we can define _IsVSTestOnlyTestProject only once I think that would be better.
Have you tested this? Do you think we should add automated tests? There are some existing tests for Reference Exe scenarios here: https://github.com/dotnet/sdk/blob/main/test/Microsoft.NET.Build.Tests/ReferenceExeTests.cs
| <_DefaultUserProfileRuntimeStorePath>$([System.IO.Path]::Combine($(_DefaultUserProfileRuntimeStorePath), '.dotnet', 'store'))</_DefaultUserProfileRuntimeStorePath> | ||
| <UserProfileRuntimeStorePath Condition="'$(UserProfileRuntimeStorePath)' == ''">$(_DefaultUserProfileRuntimeStorePath)</UserProfileRuntimeStorePath> | ||
| <!-- This excludes MTP projects, as they are normal console apps --> | ||
| <_IsVSTestOnlyTestProject Condition="'$(IsTestProject)' == 'true' AND '$(IsTestingPlatformApplication)' != 'true'">true</_IsVSTestOnlyTestProject> |
There was a problem hiding this comment.
It looks like this property is defined multiple times. Can you find a single place to define it? You may need to dig into the evaluation order to figure out the right place to do this.
|
@dsplaisted Would adding the property here be good? Or probably here can be good as well |
|
@Youssef1313 Looking at the ordering, I think you can leave it where you have it in Microsoft.NET.RuntimeIdentifierInference.targets, and remove it from Microsoft.NET.Sdk.targets, and that should work. |
|
@Youssef1313 Will you have time to respond to @dsplaisted's feedback? |
| Finally, library projects and non-executable projects have awkward interactions here so they are excluded.--> | ||
| <PropertyGroup Condition="'$(UseCurrentRuntimeIdentifier)' == ''"> | ||
| <!-- This excludes MTP projects, as they are normal console apps --> | ||
| <_IsVSTestOnlyTestProject Condition="'$(IsTestProject)' == 'true' AND '$(IsTestingPlatformApplication)' != 'true'">true</_IsVSTestOnlyTestProject> |
There was a problem hiding this comment.
TODO: Ensure that this is even late enough to read IsTestingPlatformApplication. It's not invalid to set it in Directory.Build.targets. So, if this is imported before, it may not work as expected.
Not yet unfortunately, and this might end up being more involved than what I initially thought. |
|
@dsplaisted @nagilson We might not be able to read What I have in mind is:
|
|
Replacing with #49838 |
Fixes #49063