Add exclusion support to source-build SDK tests #15650

NikolaMilosavljevic · 2023-02-27T17:05:39Z

This adds support for exclusions in SDK diffing tests. The initial change adds 3 exclusions, for directories that are primary causes of maintenance issues with SDK tests. These directories are only present in one SDK (Microsoft or source-build) and cause test failure whenever there are new, or deleted, files in those directory trees.

Microsoft-only directories:

./sdk/x.y.z/TestHostNetFramework/?*
./sdk/x.y.z/Sdks/Microsoft.NET.Sdk.WindowsDesktop/?*

Source-build only directories:

./sdk/x.y.z/TestHost/?*

Exclusion list is processed using FileSystemName.MatchesSimpleExpression which supports simple file-matching wildcards.

In the example above, we intentionally use ?* and not just * - we want to keep the directory entry in the diff, so that the test fails if any of these directories get removed, or added to the SDK that is not carrying it today, i.e. if ./sdk/x.y.z/Sdks/Microsoft.NET.Sdk.WindowsDesktop/ gets added to source-build SDK.

Additional opportunities

We could add more exclusions. Here are some examples:

Microsoft-only directory that should never be added to source-build SDK:
./sdk/x.y.z/Sdks/Microsoft.NET.Sdk.Publish/tools/net472/?*

Localized content under ./sdk/x.y.z/Extensions/, i.e. ./sdk/x.y.z/Extensions/cs/?* This issue will be fixed - tracked in microsoft/vstest#4305

MichaelSimons · 2023-02-27T17:37:22Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs


        File.WriteAllLines(outputFileName, files);
    }

+        private static IEnumerable<string> FilteredFileList(IEnumerable<string> files, IEnumerable<string> exclusions)


To follow standard naming conventions, the method name should contain a verb. Perhaps including the Exclusion term would help the readability as it indicates what filter is being applied.

Suggested change

private static IEnumerable<string> FilteredFileList(IEnumerable<string> files, IEnumerable<string> exclusions)

private static IEnumerable<string> RemoveExclusions(IEnumerable<string> files, IEnumerable<string> exclusions)

Sure, I'll update that. The idea was that the method returns a list that is filtered.

Fixed with 9392ef3

MichaelSimons · 2023-02-27T17:43:08Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs


        File.WriteAllLines(outputFileName, files);
    }

+        private static IEnumerable<string> FilteredFileList(IEnumerable<string> files, IEnumerable<string> exclusions)
+        {
+            return files


Consider utilizing a expression bodied method here. It is a nice simplification IMO.

private static IEnumerable<string> FilteredFileList(IEnumerable<string> files, IEnumerable<string> exclusions) => files.Where(item => !exclusions.Any(p => FileSystemName.MatchesSimpleExpression(p, item)));

Sounds good - will update.

Fixed with 9392ef3

MichaelSimons · 2023-02-27T17:45:12Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs

+                    files,
+                    GetExclusionFilters(
+                        Path.Combine(BaselineHelper.GetAssetsDirectory(), "SdkDiffExclusions.txt"),
+                        tarballPath == Config.MsftSdkTarballPath ? "msft" : "sb"));


This breaks the roles and responsibilities of the method and hard codes it to a specific tarballPath. If the caller ever changes the path, this implementation will break. I think it would be better to pass sdkType into WriteTarballFileList as a parameter.

I wanted to avoid adding an extra parameter. But, I will add a simple string parameter to indicate the sdk type.

Fixed with 9392ef3

MichaelSimons · 2023-02-27T17:46:10Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs

+
+        private static IEnumerable<string> GetExclusionFilters(string exceptionFilePath, string sdkType)
+        {
+            var text = File.ReadAllText(exceptionFilePath);


Consider utilizing ReadAllLines instread of having the code perform the line splitting.

Yeah, I can do that. The code was originally lifted from https://github.com/dotnet/arcade-services/blob/a018eaa016d2132265bfafdcb58d8cb35da6d567/src/Microsoft.DotNet.Darc/src/DarcLib/VirtualMonoRepo/VmrScanner.cs#L89-L100

Fixed with 9392ef3

...ourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/assets/SdkDiffExclusions.txt

MichaelSimons · 2023-02-27T19:35:06Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs

+
+        private static IEnumerable<string> GetExclusionFilters(string exceptionFilePath, string sdkType)
+        {
+            string[] lines = File.ReadAllLines(exceptionFilePath);


For brevity, consider in-lining below

return File.ReadAllLines(exceptionFilePath) .Where(line => line.StartsWith(sdkType + ",")) // process only specific sdk exclusions .Select(line => { // Ignore comments var index = line.IndexOf('#'); return index >= 0 ? line[prefixSkip..index].TrimEnd() : line[prefixSkip..]; }) .ToList();

Fixed with 0048bcd

MichaelSimons · 2023-02-27T23:25:15Z

...ourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/assets/SdkDiffExclusions.txt

+
+# nuget localization is not available for Linux builds - https://github.com/NuGet/Home/issues/12440
+msft,./sdk/x.y.z/*?/NuGet.*?.resources.dll
+msft,./sdk/x.y.z/Sdks/NuGet.Build.Tasks.Pack/*?/NuGet.*?.resources.dll


I have a few questions about the remaining diffs.

Would it be appropriate to add an exclusion for the known diffs that are caused by ilmerge not being supported in source-build?

The following differences are caused by Pull in MAUI repo(s) in order to source-build MAUI manifest nupkgs source-build#3242 and should be added as exclusions IMO.

-./sdk-manifests/x.y.z/microsoft.net.sdk.android/ -./sdk-manifests/x.y.z/microsoft.net.sdk.android/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.android/WorkloadManifest.targets -./sdk-manifests/x.y.z/microsoft.net.sdk.ios/ -./sdk-manifests/x.y.z/microsoft.net.sdk.ios/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.ios/WorkloadManifest.targets -./sdk-manifests/x.y.z/microsoft.net.sdk.maccatalyst/ -./sdk-manifests/x.y.z/microsoft.net.sdk.maccatalyst/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.maccatalyst/WorkloadManifest.targets -./sdk-manifests/x.y.z/microsoft.net.sdk.macos/ -./sdk-manifests/x.y.z/microsoft.net.sdk.macos/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.macos/WorkloadManifest.targets -./sdk-manifests/x.y.z/microsoft.net.sdk.maui/ -./sdk-manifests/x.y.z/microsoft.net.sdk.maui/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.maui/WorkloadManifest.targets -./sdk-manifests/x.y.z/microsoft.net.sdk.tvos/ -./sdk-manifests/x.y.z/microsoft.net.sdk.tvos/WorkloadManifest.json -./sdk-manifests/x.y.z/microsoft.net.sdk.tvos/WorkloadManifest.targets

Do you have an understanding of what the cause is of the following two resource.dll diffs are?
./sdk/x.y.z/cs/Microsoft.Build.NuGetSdkResolver.resources.dll

If you don't know now that is fine. I think issues should be logged if they don't already exist for the remaining entries in the baseline.
./sdk/x.y.z/cs/Microsoft.CodeCoverage.IO.resources.dll

I'll address 1st and 2nd points. I'm not 100% sure if 3rd is related to NuGet-repo localization - will check and include it if it is - same for Microsoft.CodeCoverage.IO.resources.dll.

mthalman · 2023-02-28T20:43:23Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SdkContentTests.cs

+        private static IEnumerable<string> RemoveExclusions(IEnumerable<string> files, IEnumerable<string> exclusions) =>
+            files.Where(item => !exclusions.Any(p => FileSystemName.MatchesSimpleExpression(p, item)));
+
+        private static IEnumerable<string> GetExclusionFilters(string exceptionFilePath, string sdkType)


Consider renaming exceptionFilePath to exclusionsFilePath for consistency.

Fixed with 93d517d

MichaelSimons

LGTM - as a follow-up, please log issue(s) for the remaining unexplained differences in the baseline.

NikolaMilosavljevic added 2 commits February 26, 2023 16:26

Enable SDK diff exclusions

4336699

Update baseline

582fe0f

NikolaMilosavljevic requested a review from MichaelSimons February 27, 2023 17:05

NikolaMilosavljevic requested a review from a team as a code owner February 27, 2023 17:05

MichaelSimons reviewed Feb 27, 2023

View reviewed changes

MichaelSimons added the Area-SourceBuild label Feb 27, 2023

Addressing review comments

9392ef3

MichaelSimons reviewed Feb 27, 2023

View reviewed changes

NikolaMilosavljevic added 2 commits February 27, 2023 19:51

Small update

0048bcd

Update exclusions list and baseline

d45e89a

MichaelSimons reviewed Feb 27, 2023

View reviewed changes

More exclusions and updated baseline

f1cd2c1

mthalman approved these changes Feb 28, 2023

View reviewed changes

Small naming update

93d517d

MichaelSimons approved these changes Feb 28, 2023

View reviewed changes

NikolaMilosavljevic merged commit 86543a0 into dotnet:main Feb 28, 2023

This was referenced Mar 1, 2023

[release/7.0.1xx] SDK diff exclusions - backport #15670

Merged

[release/6.0.1xx] SDK diff exclusions - backport #15676

Merged

Understand and eliminate all remaining SDK content differences dotnet/source-build#3291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exclusion support to source-build SDK tests #15650

Add exclusion support to source-build SDK tests #15650

NikolaMilosavljevic commented Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

MichaelSimons Feb 27, 2023

NikolaMilosavljevic Feb 27, 2023

mthalman Feb 28, 2023

NikolaMilosavljevic Feb 28, 2023

MichaelSimons left a comment

	private static IEnumerable<string> FilteredFileList(IEnumerable<string> files, IEnumerable<string> exclusions)
	private static IEnumerable<string> RemoveExclusions(IEnumerable<string> files, IEnumerable<string> exclusions)

Add exclusion support to source-build SDK tests #15650

Add exclusion support to source-build SDK tests #15650

Conversation

NikolaMilosavljevic commented Feb 27, 2023

Additional opportunities

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

MichaelSimons left a comment

Choose a reason for hiding this comment