Revert "Revert "Move hosting tests to XUnit 3"" #121072
Conversation
This reverts commit 8e20ce4.
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
|
@jkoritzinsky Looks like the tests are trying to pull the host from NuGet, but I would expect the host to be part of our bootstrap. Is that correct? Or do we not yet build the host in the bootstrap? |
|
It looks like we do build the host, but we don't hook up the right logic in targetingpacks.targets for the bootstrapped apphost to be found. This target needs to be updated: runtime/eng/targetingpacks.targets Line 136 in 5efa15f It should do something like the target for the ref/targeting packs (use runtime/eng/targetingpacks.targets Lines 97 to 104 in 5efa15f |
There was a problem hiding this comment.
Pull request overview
This PR reverts a previous revert to re-apply changes that move hosting tests from XUnit 2 to XUnit 3. The migration involves significant infrastructure and test code updates to support the new test framework version.
Key changes:
- Migrates test projects to XUnit V3 with new package references and execution model (self-contained executables instead of
dotnet test) - Renames
TestContexttoHostTestContextthroughout all test files - Updates conditional test attributes to use
Assert.SkipUnlesspattern - Enhances Command class to support optional
ITestOutputHelperfor test output
Reviewed changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/installer/tests/helixpublish.proj |
Updates test execution to run tests as native executables with XUnit V3 CLI args |
src/installer/tests/TestUtils/TestUtils.csproj |
Updates XUnit package references from v2 to v3 |
src/installer/tests/TestUtils/HostTestContext.cs |
Renames TestContext class to HostTestContext |
src/installer/tests/TestUtils/Command.cs |
Adds optional ITestOutputHelper parameters for test output integration |
src/installer/tests/HostActivation.Tests/*.cs |
Mass update of test references from TestContext to HostTestContext |
src/installer/tests/Microsoft.NET.HostModel.Tests/*.cs |
Updates test attributes and context references for XUnit V3 |
src/installer/tests/AppHost.Bundle.Tests/*.cs |
Updates test attributes and context references for XUnit V3 |
src/installer/tests/Directory.Build.props |
Changes test filter syntax from v2 to v3 format, updates dump configuration |
src/installer/tests/Directory.Build.targets |
Adds MSTest extensions for crash/hang dumps |
src/installer/tests/Assets/Projects/Directory.Build.targets |
Simplifies build configuration for test asset projects |
src/installer/pkg/sfx/**/*.props |
Updates targeting pack configuration for new build model |
src/installer/Directory.Build.* |
Adds bootstrap layout support for platforms without LKG apphost |
src/installer/tests/Microsoft.NET.HostModel.Tests/MachObjectSigning/SigningTests.cs
Show resolved
Hide resolved
Temporarily disable test due to macOS codesign behavior change.
| <Import Project="$(RepositoryEngineeringDir)liveBuilds.targets" /> | ||
| <Import Project="$(RepositoryEngineeringDir)targetingpacks.targets" /> | ||
|
|
||
| <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.targets, $(MSBuildThisFileDirectory)..))" /> |
There was a problem hiding this comment.
@agocke this change looks wrong. Now these projects don't import the parent Directory.Build.targets file anymore but still the parent Directory.Build.props file.
There was a problem hiding this comment.
I noticed this as part of #121853 as properties like NETCoreAppMaximumVersion are not correctly set anymore.
There was a problem hiding this comment.
No I think that’s right. These are test assets. I don’t think they should be importing the upwards targets file
There was a problem hiding this comment.
It may be possible to import a specific targets file that provides a minimum set of functionality that we think should apply to tests, but the whole point of these projects is that they should be "normal" .NET projects that an everyday user would see. We do not want our targets files arbitrarily polluting their space.
There was a problem hiding this comment.
Then they shouldn't even import any of the upwards props files. They must work without any of our custom defined settings. Also some of our props files must even have targets in them. I think you get my point. We need to be in synchronicity between D.B.props and D.B.targets.
There was a problem hiding this comment.
Yes, that's probably right -- I didn't realize they import the props files. They shouldn't do that either.
Reverts #120997