Fix test assets importing repo defaults instead of SDK defaults#122154
Conversation
These test assets should use SDK defaults instead of repo defaults. - Set ImportDirectoryBuildProps=false to prevent automatic parent import - Define necessary properties locally (NetCoreAppCurrent, paths, etc.) - Keep Directory.Build.targets imports for local runtime targeting Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
|
@copilot some of these tests are failing because they reference old target frameworks (.NET 6). Can you please define a |
| <PropertyGroup> | ||
| <!-- Prevent automatic import of parent Directory.Build.props to avoid pulling in repo defaults. | ||
| These are test assets and should use SDK defaults instead. --> | ||
| <ImportDirectoryBuildProps>false</ImportDirectoryBuildProps> |
There was a problem hiding this comment.
This isn't necessary, just avoiding the import is enough.
| <PropertyGroup> | ||
| <!-- This should match NetCoreAppCurrentVersion in the repo root Directory.Build.props --> | ||
| <NetCoreAppCurrentVersion>10.0</NetCoreAppCurrentVersion> | ||
| <NetCoreAppCurrent>net$(NetCoreAppCurrentVersion)</NetCoreAppCurrent> |
There was a problem hiding this comment.
This can be the testtfm, actually
There was a problem hiding this comment.
Updated in commit 5a37deb. TestTfm is now the primary property with NetCoreAppCurrent as an alias.
- Added TestTfm property to Directory.Build.props (set to NetCoreAppCurrent) - Updated all test asset projects to use $(TestTfm) instead of $(NetCoreAppCurrent) or hardcoded target frameworks - This fixes the issue where SelfContained.csproj was using outdated net6.0 target framework Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Done in commit 9dfe47a. Added |
- Remove unnecessary ImportDirectoryBuildProps=false setting - Make TestTfm the primary property, with NetCoreAppCurrent as an alias - Simplify property definitions per code review feedback Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>$(NetCoreAppCurrent)</TargetFramework> |
There was a problem hiding this comment.
@copilot NetCoreAppCurrent was a fine name, actually. Let's keep it and set that variable in Directory.Build.props.
There was a problem hiding this comment.
Updated in commit f6d3163. NetCoreAppCurrent is now the primary property, and all 22 test projects use it.
- Removed TestTfm property from Directory.Build.props - NetCoreAppCurrent is now the primary target framework property - Updated all 22 test asset projects to use $(NetCoreAppCurrent) Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
| <!-- Define the target framework version these test assets should build against --> | ||
| <PropertyGroup> | ||
| <!-- This should match NetCoreAppCurrentVersion in the repo root Directory.Build.props --> | ||
| <NetCoreAppCurrentVersion>10.0</NetCoreAppCurrentVersion> |
There was a problem hiding this comment.
@copilot This value is out of sync now. Maybe we should pull in eng/Versions.props and define it based on that, similar to what System.Net StressTests do.
There was a problem hiding this comment.
Fixed in the latest commit. The Directory.Build.props now imports eng/Versions.props and derives NetCoreAppCurrentVersion from $(MajorVersion).$(MinorVersion), the same approach used by System.Net StressTests. The value now resolves to net11.0 automatically.
…rdcoding Co-authored-by: elinor-fung <47805090+elinor-fung@users.noreply.github.com>
Fixes #122153
main PR
Description
Test assets in
src/installer/tests/Assets/Projects/were importing parentDirectory.Build.props, inheriting repo-wide build conventions (TreatWarningsAsErrors, LangVersion=preview, analyzers, etc.) instead of using SDK defaults. This made them unrepresentative of normal .NET applications. Additionally, some test projects were using outdated target frameworks (net6.0).Changes:
Directory.Build.propsimport statement (without explicitly settingImportDirectoryBuildProps=false)RepoRoot,RepositoryEngineeringDir,ArtifactsDir,NetCoreAppCurrentNetCoreAppCurrentis set as the target framework property for all test assets$(NetCoreAppCurrent)instead of hardcoded target frameworksOutputPath,UseLocalTargetingRuntimePackTest assets now use SDK defaults (e.g., TreatWarningsAsErrors=false, LangVersion=14.0) and consistently target net10.0 while maintaining ability to target locally built runtime via
Directory.Build.targets.Customer Impact
N/A - Internal test infrastructure only.
Regression
No - this improves test asset fidelity to real-world applications.
Testing
Verified multiple test asset projects build successfully with SDK defaults instead of repo defaults. All test projects now correctly target net10.0 through the NetCoreAppCurrent property.
Risk
Low. Changes isolated to test asset build configuration. Does not affect production code or shipping artifacts.
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.