Implement unit-tests for UpdateNuGetConfigPackageSourcesMappings task

[NikolaMilosavljevic]

Source-build tasks do not have any unit-tests. As tasks get complex tests are even more important, as was experienced with this task. There were developer-centric tests, which need to be converted into unit-tests.

This is related to #4666

As part of this work, it is important to improve organization of unified build and source-build tests. Currently we have two projects in VMR, under test, both of them are product tests. Source-build tests should run both source-build and unified-build tests, as those are common - today. In the future there will likely be some tests that are specific to unified build and not applicable to source-build. Common, source-build and unified-build (or Microsoft) tests are 3 natural classes of tests. Perhaps unified-build tests can be marked appropriately, so that common tests are included in source-build testing.

Unit-tests are usually built and run as part of the same build so they consume the same dependencies. Product tests are intentionally removed from main build, to enable these test projects to consume artifacts (including packages) that are produced by unified- (or source-) build.

Open question: should unit-tests for unified-build and source-build tasks be built/executed together with product tests or earlier in the build when tasks are built.

[ghost]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[NikolaMilosavljevic]

@MichaelSimons @ViktorHofer

[ViktorHofer]

Open question: should unit-tests for unified-build and source-build tasks be built/executed together with product tests or earlier in the build when tasks are built.

IMO the current sequence point (the Test target) is appropriate and I wouldn't change that. Our repo official builds usually don't run tests (there are a few outliers, i.e. vstest). We decided on that few years back for improved reliability and build times.

If we need a place to do unit testing then I would recommend adding a new test project under /test.

As part of this work, it is important to improve organization of unified build and source-build tests. Currently we have two projects in VMR, under test, both of them are product tests. Source-build tests should run both source-build and unified-build tests, as those are common - today. In the future there will likely be some tests that are specific to unified build and not applicable to source-build. Common, source-build and unified-build (or Microsoft) tests are 3 natural classes of tests. Perhaps unified-build tests can be marked appropriately, so that common tests are included in source-build testing.

Sounds good.

[MichaelSimons]

Currently we have two projects in VMR, under test, both of them are product tests. Source-build tests should run both source-build and unified-build tests, as those are common - today.

The current source-build tests are a mixture of unit tests and scenario tests. #4384 tracks removing the scenario tests now that we have a shared set of tests in the scenarios-tests repo.

Source-build does not and should not run the current unified-build tests. The current unified build tests are transient only for the purposes of comparing the unified build leg output to the current Microsoft builds as a means to identify differences in the unified build.

Common, source-build and unified-build (or Microsoft) tests are 3 natural classes of tests.

I like the idea of three separate projects for this.

[NikolaMilosavljevic]

The only remaining question is about unit-test location and the time they get built. While trying to build them as part of unified-build tests in test/Microsoft.DotNet.UnifiedBuild.Tests I encountered issues with referencing the Tasks project from eng/tools/tasks/Microsoft.DotNet.UnifiedBuild.Tasks. Tasks project is built with different dependencies, coming from PSB or SBRP. Anything under test is build using artifacts produced by build.

It should be possible to introduce some new '.proj` file to build the tasks project again, during testing to publish to a different location, and have it consume new dependencies.

However, I believe a better option might be to build unit-tests project along with tasks project, at the same time, using the same dependencies. That way we'd be building the tests using the same toolset and dependencies. Tasks are used during build, and consume old dependencies, unit-tests should consume those same dependencies. This allows unit-tests to test the actual tasks experience. In this scenario, we'd need to ensure we have the dependencies available during testing.

[ViktorHofer]

However, I believe a better option might be to build unit-tests project along with tasks project, at the same time, using the same dependencies. That way we'd be building the tests using the same toolset and dependencies. Tasks are used during build, and consume old dependencies, unit-tests should consume those same dependencies. This allows unit-tests to test the actual tasks experience. In this scenario, we'd need to ensure we have the dependencies available during testing.

I don't think that's the right approach. We want to build the unit test projects independently and also not part of the main build. That's why we have tests.proj as a separate unit.

remaining question is about unit-test location

IMO the location should be a separate project under test/.

It should be possible to introduce some new '.proj` file to build the tasks project again, during testing to publish to a different location, and have it consume new dependencies.

We want to test the tasks that are executed as part of the VMR build, hence they shouldn't get rebuilt with a different set of inputs.

Anything under test is build using artifacts produced by build.

Would you mind elaborating on that? How is that enforced today?

[NikolaMilosavljevic]

I might have phrased some of my previous comments tersely. When we build tests we have all the newly produced packages in NuGet store, which can lead to issues with package mismatch.

However, the issue I have seen in my initial attempt to add unit-tests is somewhat different.

Test project directly depends on NuGet.Protocol (and indirectly other NuGet.* packages) version 6.9.1 - getting dependency from root Directory.Packages.props, and version from eng/Versions.props - https://github.com/dotnet/sdk/blob/8584c262d4d2e1a0af388271ae0715db3c53a2b6/src/SourceBuild/content/test/Microsoft.DotNet.UnifiedBuild.Tests/Microsoft.DotNet.UnifiedBuild.Tests.csproj#L13

When I added the unit-test source file to this project and a necessary reference to Tasks project, I got the following build errors (just one here, for brevity):

/src/git/dotnet/.dotnet/sdk/9.0.100-rc.1.24452.12/Microsoft.Common.CurrentVersion.targets(2413,5): error MSB3277: Found conflicts between different versions of "NuGet.Packaging" that could not be resolved.
There was a conflict between "NuGet.Packaging, Version=6.9.1.3, Culture=neutral, PublicKeyToken=31bf3856ad364e35" and "NuGet.Packaging, Version=6.12.0.81, Culture=neutral, PublicKeyToken=31bf3856ad364e35".
    "NuGet.Packaging, Version=6.9.1.3, Culture=neutral, PublicKeyToken=31bf3856ad364e35" was chosen because it was primary and "NuGet.Packaging, Version=6.12.0.81, Culture=neutral, PublicKeyToken=31bf3856ad364e35" was not.
    References which depend on "NuGet.Packaging, Version=6.9.1.3, Culture=neutral, PublicKeyToken=31bf3856ad364e35" [/src/git/dotnet/.packages/nuget.packaging/6.9.1/lib/net5.0/NuGet.Packaging.dll].
        /src/git/dotnet/.packages/nuget.packaging/6.9.1/lib/net5.0/NuGet.Packaging.dll
          Project file item includes which caused reference "/src/git/dotnet/.packages/nuget.packaging/6.9.1/lib/net5.0/NuGet.Packaging.dll".
            /src/git/dotnet/.packages/nuget.packaging/6.9.1/lib/net5.0/NuGet.Packaging.dll
    References which depend on or have been unified to "NuGet.Packaging, Version=6.12.0.81, Culture=neutral, PublicKeyToken=31bf3856ad364e35" [].
        /src/git/dotnet/artifacts/bin/Microsoft.DotNet.UnifiedBuild.Tasks/Release/Microsoft.DotNet.UnifiedBuild.Tasks.dll
          Project file item includes which caused reference "/src/git/dotnet/artifacts/bin/Microsoft.DotNet.UnifiedBuild.Tasks/Release/Microsoft.DotNet.UnifiedBuild.Tasks.dll".
            /src/git/dotnet/artifacts/bin/Microsoft.DotNet.UnifiedBuild.Tasks/Release/Microsoft.DotNet.UnifiedBuild.Tasks.dll [/src/git/dotnet/test/Microsoft.DotNet.UnifiedBuild.Tests/Microsoft.DotNet.UnifiedBuild.Tests.csproj]

This was caused by direct references to NuGet assemblies in .NET SDK, from the Tasks project, and other projects under tools: https://github.com/dotnet/sdk/blob/8584c262d4d2e1a0af388271ae0715db3c53a2b6/src/SourceBuild/content/eng/tools/Directory.Build.props#L9-L19

  <!--
    Use some assemblies from the SDK, instead of package references. This ensures they match what's
    found when the task is loaded by the SDK's MSBuild.
    Reference NuGet assemblies, except a command line assembly that causes warnings such as:
    MSB3277: Found conflicts between different versions of "System.Collections" that could not be resolved.
  -->
  <ItemGroup>
    <SdkAssembly Include="$([MSBuild]::NormalizePath('$(NetCoreRoot)', 'sdk', '$(NETCoreSdkVersion)', 'Newtonsoft.Json.dll'));
                          $([MSBuild]::NormalizeDirectory('$(NetCoreRoot)', 'sdk', '$(NETCoreSdkVersion)'))NuGet.*.dll"
                 Exclude="$([MSBuild]::NormalizePath('$(NetCoreRoot)', 'sdk', '$(NETCoreSdkVersion)', 'NuGet.CommandLine.XPlat.dll'))" />
  </ItemGroup>

While, I can solve this by creating a new project for unit-tests, that would not have a dependency on any NuGet.* packages (as I don't need them), there might be other unit-tests in the future, that would need NuGet.* packages and will hit this issue. So, I think that we should have the right solution in place, to avoid such issues down the road.

[ViktorHofer]

Yes, that makes sense. It's a mismatch between a direct assembly reference (the NuGet assemblies from the .NET SDK) and a package reference. That is a general problem and RAR (ResolveAssemblyReference) warns about these to make sure that dependencies are compatible and can be unified.

Here, either all tasks need to use a package reference or if that isn't possible (i.e. because of prebuilts), the projects that reference task projects need to use the same mechanism to reference the NuGet assemblies from the SDK. Note that this only affects NuGet assemblies and Newtonsoft.Json. We have a tracking issue to remove the latter eventually.

Hope that helps.