Allow to use global packages folder#15483
Conversation
| /// Gets the local NuGet cache directory used by test assets. | ||
| /// Test assets are restored into this directory by Build.cs via <c>dotnet restore --packages</c>. | ||
| /// </summary> | ||
| public static string TestAssetsNuGetCacheDirectory { get; } = Path.Combine(RepoRootDirectory, ".packages"); |
There was a problem hiding this comment.
| public static string TestAssetsNuGetCacheDirectory { get; } = Path.Combine(RepoRootDirectory, ".packages"); | |
| public static string TestAssetsNuGetCacheDirectory { get; } = Path.Combine(RepoRootDirectory, "artifacts", ".packages"); |
I'd move it here to make it less confusing. Tests are responsible for managing that folder.
There was a problem hiding this comment.
this should now be moved
There was a problem hiding this comment.
Pull request overview
Updates integration/acceptance test infrastructure to use a consistent, repo-local NuGet packages cache (.packages) without relying on pipeline-level NUGET_PACKAGES overrides, addressing #15469.
Changes:
- Introduces a shared
TestAssetsNuGetCacheDirectoryand updates test code to use it instead of hardcoding.packagespaths. - Forces restore for the acceptance integration tests project into the repo-local
.packagesfolder and updates package content paths accordingly. - Removes
NUGET_PACKAGESoverrides from Azure Pipelines and adjustsNuGet.config/test config rewriting behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.TestUtilities/IntegrationTestEnvironment.cs | Adds a central .packages path constant and uses it for test package resolution. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/Microsoft.TestPlatform.Acceptance.IntegrationTests.csproj | Sets restore/package content paths to use repo-local .packages. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/Extension/CompatibilityRowsBuilder.cs | Switches hardcoded .packages path usage to the shared constant. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/Build.cs | Uses the shared .packages path for test package operations. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/AcceptanceTestBase.cs | Changes NuGet.config rewriting to inject globalPackagesFolder pointing to the shared .packages path. |
| azure-pipelines.yml | Removes pipeline-level NUGET_PACKAGES override. |
| azure-pipelines-official.yml | Removes pipeline-level NUGET_PACKAGES override. |
| NuGet.config | Removes default globalPackagesFolder entry and updates comments to reflect new injection approach. |
You can also share your feedback on Copilot code review. Take the survey.
| <!-- Restore packages to the local .packages directory so integration tests can find them at a known path. --> | ||
| <RestorePackagesPath>$(RepoRoot).packages\</RestorePackagesPath> |
There was a problem hiding this comment.
$(RepoRoot).packages\... concatenates without a path separator between $(RepoRoot) and .packages, and it hardcodes backslashes (which can be treated as a literal character on non-Windows). This can produce an invalid restore/content path (e.g., C:\repo.packages\... or a path containing \ on Linux), breaking restore and file inclusion. Prefer building the path with an explicit separator (e.g., $(RepoRoot)\.packages\...) or via an MSBuild property using System.IO.Path::Combine and then reuse that property for both RestorePackagesPath and Content Include.
| --> | ||
| <PackageReference Include="Microsoft.Internal.Dia" Version="$(TestPlatformMSDiaVersion)" PrivateAssets="all" /> | ||
| <Content Include="..\..\.packages\microsoft.internal.dia\$(TestPlatformMSDiaVersion)\tools\net451\**\*"> | ||
| <Content Include="$(RepoRoot).packages\microsoft.internal.dia\$(TestPlatformMSDiaVersion)\tools\net451\**\*"> |
There was a problem hiding this comment.
$(RepoRoot).packages\... concatenates without a path separator between $(RepoRoot) and .packages, and it hardcodes backslashes (which can be treated as a literal character on non-Windows). This can produce an invalid restore/content path (e.g., C:\repo.packages\... or a path containing \ on Linux), breaking restore and file inclusion. Prefer building the path with an explicit separator (e.g., $(RepoRoot)\.packages\...) or via an MSBuild property using System.IO.Path::Combine and then reuse that property for both RestorePackagesPath and Content Include.
| // Point packages folder to vstest's local .packages cache used by test assets. | ||
| .Replace("</config>", $"""<add key="globalPackagesFolder" value="{IntegrationTestEnvironment.TestAssetsNuGetCacheDirectory}" /></config>""") |
There was a problem hiding this comment.
This injects XML via a string replace on </config>, which is fragile if the copied NuGet.config formatting changes (e.g., whitespace variations, self-closing tags, or additional </config> occurrences from future edits). A more robust approach is to load the file as XML (e.g., XDocument), locate/create the <config> element, and add the <add key="globalPackagesFolder" ... /> node before saving.
| <OutputType>Exe</OutputType> | ||
| <TargetFrameworks>net9.0;net48</TargetFrameworks> | ||
| <!-- Restore packages to the local .packages directory so integration tests can find them at a known path. --> | ||
| <RestorePackagesPath>$(RepoRoot).packages\</RestorePackagesPath> |
There was a problem hiding this comment.
Is this only for the msdia dependency below?
There was a problem hiding this comment.
pretty much. also reading through how integration tests work.
so far my understanding is that with the global cache:
- the Build.cs will handle preparing the artifacts/.packages folder for all acceptance tests. there's a method that will remove all -dev packages before the test runs
- test assets such as CLIProject, MSTestV1UnitTestProject, LegacySettingsUnitTestProject are all using the local .packages cacheDid
it would seem that we can fully make this use the global cache. running all tests to validate that this works
There was a problem hiding this comment.
Yeah I think the only "problem" here is figuring out where the global cache is, but maybe we could get rid of all the dependencies on that package instead. I am not if the tests are testing something that an integration test does not test.
- Remove globalPackagesFolder from NuGet.config so the main solution build uses the default global NuGet cache (~/.nuget/packages/). - Remove NUGET_PACKAGES overrides from CI pipelines. - Move test asset NuGet cache from .packages/ to artifacts/.packages/ so it lives under the already-gitignored artifacts/ directory. - Test assets (built by Build.cs) continue to restore into the local cache via explicit --packages flag. - Update AcceptanceTestBase to inject globalPackagesFolder into copied NuGet.config for isolated test asset builds. - Fix msdia Content Include to use $(NuGetPackageRoot) since it is restored by the solution build, not by Build.cs test asset restoration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Will merge after #15484 , I touched the same files |
8b74ace to
46375b9
Compare
|
I've applied the feedback. We should now be using artifacts/.packages, and only use that for the integration test samples (so if integration tests are not being run things should be good). |
|
There were too many conflicts, moved your code to new PR, sorry for "stealing it". |
Fixes #15469