Add package source mappings unit-tests #44649

NikolaMilosavljevic · 2024-11-05T17:40:16Z

This PR adds unit-tests for package source mappings task and introduces a new test project for common, shared, tests.

Some interesting details:

Using Array.Sort to get a stable output of Directory.GetFiles which is not guaranteed to return a sorted output. This helps with unit-tests.

Some assembly references, like nuget.* are used from SDK, which is necessary due to

sdk/src/SourceBuild/content/eng/tools/Directory.Build.props

Lines 9 to 19 in ffd7663

    
             <!-- 
        
               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>

and the need to use the same dependencies in unit-test assembly, as the task assembly. This still shows a warning when running source-build tests, as there other versions of nuget.* restored from source-build feeds. Therefore a NoWarn is needed for MSB3277.

9 tests cover all unified-build and source-build scenarios, including a special SBRP repo build and unified-build without local sources (i.e. arcade repo).

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.Build.props

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj

ViktorHofer · 2024-11-05T18:12:45Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.Build.props

+    <!--
+      Required while we're using direct SDK assembly references in
+      $(RepositoryEngineeringDir)tools/Directory.Build.props for all tools projects
+      including Microsoft.DotNet.UnifiedBuild.Tasks.
+    -->
+    <NoWarn>$(NoWarn);MSB3277</NoWarn>


Can you please elaborate on this? I'm unsure why this is necessary.

Without it I am getting the following warning/error:

/src/git/dotnet/.dotnet/sdk/9.0.100-rc.2.24474.11/Microsoft.Common.CurrentVersion.targets(2413,5): error MSB3277: There was a conflict between "NuGet.Frameworks, Version=5.11.0.10, Culture=neutral, PublicKeyToken=31bf3856ad364e35" and "NuGet.Frameworks, Version=6.12.0.115, Culture=neutral, PublicKeyToken=31bf3856ad364e35". [/src/git/dotnet/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj]

Version 6.12.0.115 is referenced from SDK via ReferencePath. Version 5.11.0.10 was discovered in NuGet cache. I'm not 100% sure why this is happening.

ViktorHofer · 2024-11-05T18:17:00Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+
+            string modifiedNugetConfig = Path.Combine(PackageSourceMappingsSetup.PackageSourceMappingsRoot, nugetConfigFilename);
+            Directory.CreateDirectory(Path.GetDirectoryName(modifiedNugetConfig)!);
+            File.Copy(originalNugetConfig, modifiedNugetConfig, true);


Are we doing all this in the source tree? Tests shouldn't mutate the source tree. Instead we usually call the create temp directory API to get a temp file / folder.

This is in test's temp folder, not in source tree.

Are you sure? Isn't this under <cwd>/PackageSourceMappingsTests?

cwd is the app's root, i.e. artifacts/bin/Microsoft.DotNet.Tests/Release/

Right and that's not a location that we should write to, if I'm not mistaken. In runtime most/all our tests use a temp location. @jkotas probably knows more about the why.

It's an easy fix, so I'll update this.

Fixed with 6337002

…Build.props Co-authored-by: Viktor Hofer <[email protected]>

…DotNet.Tests.csproj Co-authored-by: Viktor Hofer <[email protected]>

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.Build.props

MichaelSimons · 2024-11-05T18:50:12Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+
+        // Unified build - with existing mappings - online
+        [Fact]
+        public void UnifiedBuildWithMappings()


I think we should avoid the "UnifiedBuild" naming here. This is simply a "BuildWithMappings". UnifiedBuild is "the" way to build in the VMR.

Makes sense - will fix.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T18:51:15Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+        [Fact]
+        public void UnifiedBuildWithMappings()
+        {
+            string[] sources = ["source-built-arcade", "source-built-runtime", "net-sdk-supporting-feed"];


Consider utilizing constants for the sources to provide "stronger" typing to help avoid human error.

Yeah, not sure why I didn't do that. Will fix.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T18:57:10Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+            RunTest(nugetConfigFilename, useOnlineFeeds, sources);
+        }
+
+        private static void RunTest(string nugetConfigFilename, bool useOnlineFeeds, string[] sources, string[]? customSources = null, bool sourceBuild = true)


Having an optional boolean parameter defaulted to true is an antipattern. Either default it to false or negate the name.

Thought about this as well and will fix it.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T19:07:31Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+            File.WriteAllText(nugetConfigFile, fileContents);
+        }
+
+        private static void TokenizeLocalNugetConfigFeeds(string nugetConfigFile)


What is the different with this method versus UpdateNugetConfigTokens? The implementations appear identical.

One is replacing key with value, the other replacing value with key.

This was a result of refactoring, but I stopped too soon. I will refactor this further.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T19:16:31Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+            private static readonly object myLock = new();
+            private Dictionary<string, string>? localTokenSourceMappings;
+
+            public static readonly string PackageSourceMappingsRoot = Path.Combine(Directory.GetCurrentDirectory(), nameof(PackageSourceMappingsTests));


How does this work it you run the tests multiple times without "cleaning"? Should this be a unique name?

Yeah, I have a fix for this, will always use a new temp folder.

Fixed with 6337002

MichaelSimons · 2024-11-05T19:21:16Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+            private readonly string PrebuiltSource = Path.Combine(PackageSourceMappingsRoot, "prebuilt");
+            private readonly string SourceBuildReferencePackagesSource = Path.Combine(PackageSourceMappingsRoot, "source-build-reference-package-cache");
+
+            public readonly string SourceBuildReferencePackagesRepoDir = Path.Combine(PackageSourceMappingsRoot, "sbrpDir");


Why the Dir suffix?

No reason, really. It represents test SBRP repo directory. I'll remove the suffix.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T19:24:49Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+
+        // Source build tests - with and without mappings - online and offline
+        [Theory]
+        [InlineData("sb-online.config", true)]


I personally think it would help to name the NuGet.configs with a consistent explicit naming pattern. Specifically I think it would help to call our noMappings in the files. This is similar in nature how you have both online/offline in the name versus implying a default if not specified.

Makes sense - will update.

Fixed with f13f4f1

MichaelSimons · 2024-11-05T19:36:44Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+        [Fact]
+        public void UnifiedBuildNoMappings()
+        {
+            string[] sources = ["source-built-arcade", "source-built-runtime", "net-sdk-supporting-feed"];


This test's implementation appears identical to UnifiedBuildWithMappings except for the config file. For the SourceBuildTests you utilized a theory. Why not the same here? Overall, I wonder if a single Theory wouldn't make the tests simpler.

Will fix - thanks! I missed collapsing these two tests into a common Theory.

Fixed with f13f4f1

I have not tried to merge all tests into a single Theory. While possible, I feel that it would not bring much value, and make inline data harder to manage.

ViktorHofer · 2024-11-05T20:22:08Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

@@ -0,0 +1,242 @@
+using System.Collections.Generic;


Hmm this file is missing a license header. I think we are missing a rule in the editorconfig in the VMR.

Fixed with f13f4f1

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

MichaelSimons · 2024-11-06T15:23:47Z

src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSourceMappingsTests.cs

+        {
+            private static PackageSourceMappingsSetup? instance;
+            private static readonly object myLock = new();
+            private Dictionary<string, string>? localTokenSourceMappings;


I find it better to separate the static versus instance members as it helps the readability.

Fixed with f51cb51

…rceMappingsTests.cs Co-authored-by: Michael Simons <[email protected]>

Add package source mappings unit-tests

ed539b0

NikolaMilosavljevic requested review from a team as code owners November 5, 2024 17:40

ghost added Area-Infrastructure untriaged Request triage from a team member labels Nov 5, 2024

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.Build.props Outdated Show resolved Hide resolved

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.DotNet.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

NikolaMilosavljevic and others added 4 commits November 5, 2024 10:21

Update src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.…

15a9f19

…Build.props Co-authored-by: Viktor Hofer <[email protected]>

Update src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.…

af7136d

…DotNet.Tests.csproj Co-authored-by: Viktor Hofer <[email protected]>

Update src/SourceBuild/content/test/Microsoft.DotNet.Tests/Microsoft.…

4e7ca52

…DotNet.Tests.csproj Co-authored-by: Viktor Hofer <[email protected]>

Copy assets if there are updates

ce83556

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

src/SourceBuild/content/test/Microsoft.DotNet.Tests/Directory.Build.props Outdated Show resolved Hide resolved

Remove Directory.Build.props file

04209d3

MichaelSimons reviewed Nov 5, 2024

View reviewed changes

Few fixes

6337002

ViktorHofer reviewed Nov 5, 2024

View reviewed changes

NikolaMilosavljevic added 2 commits November 5, 2024 22:05

Addressing comments

f13f4f1

Merge branch 'main' into common.tests

8cd02cc

build-analysis bot mentioned this pull request Nov 6, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

MichaelSimons approved these changes Nov 6, 2024

View reviewed changes

NikolaMilosavljevic and others added 4 commits November 6, 2024 07:47

Update src/SourceBuild/content/test/Microsoft.DotNet.Tests/PackageSou…

dc31ab4

…rceMappingsTests.cs Co-authored-by: Michael Simons <[email protected]>

Update order of instance members

f51cb51

Merge branch 'main' into common.tests

75ace1a

Update VSTestLogger console verbosity

7f69446

NikolaMilosavljevic merged commit 8b91924 into dotnet:main Nov 6, 2024
37 checks passed

NikolaMilosavljevic mentioned this pull request Nov 7, 2024

Add test project and related infra where utests can be added for the VMR's build infrastructure dotnet/source-build#4244

Closed

	<!--
	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>

Add package source mappings unit-tests #44649

Add package source mappings unit-tests #44649

Conversation

NikolaMilosavljevic commented Nov 5, 2024

Choose a reason for hiding this comment

NikolaMilosavljevic Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic Nov 5, 2024 •

edited

Loading

ViktorHofer Nov 5, 2024 •

edited

Loading

ViktorHofer Nov 5, 2024 •

edited

Loading