From b48b11707f90d07c81de1097565c979ada096e7c Mon Sep 17 00:00:00 2001 From: Sunanda Balasubramanian Date: Tue, 22 Jun 2021 16:25:59 -0700 Subject: [PATCH] Expose SDK as an ICollection to enable extention (#642) * Expose SDK as an ICollection to enable extention * Fix Tests * PR Feedback * Change IsSDK implementation * Fix Integration tests * More fixes for tests * Change to ICollection from List * PR Feedback * Remove usage of list in the Remove method * Remove isSDK * Reverting the IsSdk removal * Modifying contains calls * Add some basic tests for SdkCollection * Fix tests * Fix tests * Missed setting * PR Feedback * Add some more tests --- .../IProjectFile.cs | 2 +- .../ComponentIdentifier.cs | 4 +- .../MSBuildProject.File.cs | 20 +--- .../MSBuildProject.cs | 2 +- .../SdkCollection.cs | 79 ++++++++++++++ .../WebSdkCleanupAnalyzer.cs | 3 +- .../ComponentIdentifierTests.cs | 16 +-- .../SdkCollectionTests.cs | 102 ++++++++++++++++++ .../WebSdkCleanupAnalyzerTests.cs | 25 ++--- 9 files changed, 210 insertions(+), 43 deletions(-) create mode 100644 src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/SdkCollection.cs create mode 100644 tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/SdkCollectionTests.cs diff --git a/src/common/Microsoft.DotNet.UpgradeAssistant.Abstractions/IProjectFile.cs b/src/common/Microsoft.DotNet.UpgradeAssistant.Abstractions/IProjectFile.cs index cd1c6a3a..077e550e 100644 --- a/src/common/Microsoft.DotNet.UpgradeAssistant.Abstractions/IProjectFile.cs +++ b/src/common/Microsoft.DotNet.UpgradeAssistant.Abstractions/IProjectFile.cs @@ -9,7 +9,7 @@ namespace Microsoft.DotNet.UpgradeAssistant { public interface IProjectFile { - string Sdk { get; } + ICollection Sdk { get; } public bool IsSdk { get; } diff --git a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/ComponentIdentifier.cs b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/ComponentIdentifier.cs index 2c2f7555..7716d893 100644 --- a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/ComponentIdentifier.cs +++ b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/ComponentIdentifier.cs @@ -36,7 +36,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild private static ProjectComponents GetSDKProjectComponents(IProject project, IProjectFile file) { var components = ProjectComponents.None; - if (file.Sdk.Equals(MSBuildConstants.WebSdk, StringComparison.OrdinalIgnoreCase)) + if (file.Sdk.Contains(MSBuildConstants.WebSdk)) { components |= ProjectComponents.AspNetCore; } @@ -53,7 +53,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild components |= ProjectComponents.WindowsDesktop; } - if (file.Sdk.Equals(MSBuildConstants.DesktopSdk, StringComparison.OrdinalIgnoreCase)) + if (file.Sdk.Contains(MSBuildConstants.DesktopSdk)) { components |= ProjectComponents.WindowsDesktop; } diff --git a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.File.cs b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.File.cs index 992fcd78..73f8598f 100644 --- a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.File.cs +++ b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.File.cs @@ -38,26 +38,12 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild } } - public string Sdk - { - get - { - var sdk = ProjectRoot.Sdk; - - if (sdk is null) - { - throw new ArgumentOutOfRangeException("Should check IsSdk property first"); - } - - return sdk; - } - } - - public bool IsSdk => - ProjectRoot.Sdk is not null && ProjectRoot.Sdk.Contains(MSBuildConstants.DefaultSDK, StringComparison.OrdinalIgnoreCase); + public bool IsSdk => Sdk.Any(); public ICollection Imports => new ImportsCollection(ProjectRoot); + public ICollection Sdk => new SdkCollection(ProjectRoot); + public void SetTFM(TargetFrameworkMoniker tfm) => new TargetFrameworkMonikerCollection(this, _comparer).SetTargetFramework(tfm); public void AddPackages(IEnumerable references) diff --git a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.cs b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.cs index b34ce584..672c6d01 100644 --- a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.cs +++ b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.cs @@ -81,7 +81,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild private ProjectOutputType GetDefaultOutputType() { - if (IsSdk && MSBuildConstants.SDKsWithExeDefaultOutputType.Contains(Sdk, StringComparer.OrdinalIgnoreCase)) + if (Sdk.Any(p => MSBuildConstants.SDKsWithExeDefaultOutputType.Contains(p, StringComparer.OrdinalIgnoreCase))) { return ProjectOutputType.Exe; } diff --git a/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/SdkCollection.cs b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/SdkCollection.cs new file mode 100644 index 00000000..6201aab0 --- /dev/null +++ b/src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/SdkCollection.cs @@ -0,0 +1,79 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Microsoft.Build.Construction; + +namespace Microsoft.DotNet.UpgradeAssistant.MSBuild +{ + public class SdkCollection : ICollection + { + private readonly ProjectRootElement _projectRoot; + + public SdkCollection(ProjectRootElement projectRoot) + { + _projectRoot = projectRoot; + } + + private string[] SplitSdks() => _projectRoot.Sdk.Split(";", StringSplitOptions.RemoveEmptyEntries); + + public bool Contains(string item) + { + return SplitSdks().Contains(item, StringComparer.OrdinalIgnoreCase); + } + + public void Add(string item) + { + if (!Contains(item)) + { + _projectRoot.Sdk = string.Concat(_projectRoot.Sdk, ";", item); + } + } + + public void Clear() + { + _projectRoot.Sdk = string.Empty; + } + + public int Count + { + get + { + return SplitSdks().Length; + } + } + + public bool IsReadOnly + { + get { return false; } + } + + public bool Remove(string item) + { + if (Contains(item)) + { + var sdkList = SplitSdks().Where(p => p != item); + _projectRoot.Sdk = string.Join(";", sdkList); + return true; + } + + return false; + } + + public void CopyTo(string[] array, int arrayIndex) + { + ((ICollection)this).CopyTo(array, arrayIndex); + } + + public IEnumerator GetEnumerator() + { + return SplitSdks().Cast().GetEnumerator(); + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } +} diff --git a/src/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web/WebSdkCleanupAnalyzer.cs b/src/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web/WebSdkCleanupAnalyzer.cs index 2457dad9..ca29ff2a 100644 --- a/src/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web/WebSdkCleanupAnalyzer.cs +++ b/src/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web/WebSdkCleanupAnalyzer.cs @@ -1,6 +1,5 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. - using System; using System.Linq; using System.Threading; @@ -37,7 +36,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Web // Check SDK directly (instead of using project.Components) since having the FrameworkReference counts as // having the AspNetCore component and this analyzer is specifically insterested in cases where both the SDK // and the framework reference are present. - if (!projectRoot.IsSdk || !projectRoot.Sdk.Equals(WebSdk, StringComparison.OrdinalIgnoreCase)) + if (!projectRoot.Sdk.Contains(WebSdk)) { return Task.CompletedTask; } diff --git a/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/ComponentIdentifierTests.cs b/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/ComponentIdentifierTests.cs index b2f47dbe..53bba6c7 100644 --- a/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/ComponentIdentifierTests.cs +++ b/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/ComponentIdentifierTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using Autofac.Extras.Moq; @@ -27,7 +28,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests var projectFile = mock.Mock(); projectFile.Setup(f => f.IsSdk).Returns(true); - projectFile.Setup(f => f.Sdk).Returns(string.Empty); + projectFile.Setup(f => f.Sdk).Returns(Array.Empty()); projectFile.Setup(f => f.GetPropertyValue(It.IsAny())).Returns(string.Empty); projectFile.Setup(f => f.GetPropertyValue(propertyName)).Returns(value); @@ -45,18 +46,18 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests Assert.Equal(expected, components); } - [InlineData("", ProjectComponents.None)] - [InlineData("Microsoft.NET.Sdk.Web", ProjectComponents.AspNetCore)] - [InlineData("Microsoft.NET.Sdk.Desktop", ProjectComponents.WindowsDesktop)] + [InlineData(new[] { "" }, ProjectComponents.None)] + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, ProjectComponents.AspNetCore)] + [InlineData(new[] { "Microsoft.NET.Sdk.Desktop" }, ProjectComponents.WindowsDesktop)] [Theory] - public async Task SdkTypesAsync(string sdk, ProjectComponents expected) + public async Task SdkTypesAsync(string[] sdk, ProjectComponents expected) { // Arrange using var mock = AutoMock.GetLoose(); var projectFile = mock.Mock(); projectFile.Setup(f => f.IsSdk).Returns(true); - projectFile.Setup(f => f.Sdk).Returns(sdk); + projectFile.Setup(f => f.Sdk).Returns(new HashSet(sdk, StringComparer.OrdinalIgnoreCase)); projectFile.Setup(f => f.GetPropertyValue(It.IsAny())).Returns(string.Empty); var project = mock.Mock(); @@ -96,7 +97,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests var projectFile = mock.Mock(); projectFile.Setup(f => f.IsSdk).Returns(true); - projectFile.Setup(f => f.Sdk).Returns(string.Empty); + projectFile.Setup(f => f.Sdk).Returns(Array.Empty()); projectFile.Setup(f => f.GetPropertyValue(It.IsAny())).Returns(string.Empty); var project = mock.Mock(); @@ -135,7 +136,6 @@ namespace Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests using var mock = AutoMock.GetLoose(); var projectFile = mock.Mock(); - projectFile.Setup(f => f.IsSdk).Returns(false); var project = mock.Mock(); project.Setup(p => p.GetFile()).Returns(projectFile.Object); diff --git a/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/SdkCollectionTests.cs b/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/SdkCollectionTests.cs new file mode 100644 index 00000000..c419cc9d --- /dev/null +++ b/tests/components/Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests/SdkCollectionTests.cs @@ -0,0 +1,102 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Autofac.Extras.Moq; +using Microsoft.Build.Construction; +using Xunit; + +namespace Microsoft.DotNet.UpgradeAssistant.MSBuild.Tests +{ + [Collection(MSBuildStepTestCollection.Name)] + public class SdkCollectionTests + { + private const string DefaultSDK = "Microsoft.NET.Sdk"; + private const string WebSDK = "Microsoft.NET.Sdk.Web"; + + [Fact] + public void SdkCollectionEmptyTest() + { + // Arrange + using var mock = AutoMock.GetLoose(); + + var projectRoot = ProjectRootElement.Create(); + var sdkCollection = new SdkCollection(projectRoot); + + // Assert + Assert.Empty(sdkCollection); + } + + [InlineData(DefaultSDK)] + [Theory] + public void SdkCollectionContainsTest(string sdk) + { + // Arrange + using var mock = AutoMock.GetLoose(); + + var projectRoot = ProjectRootElement.Create(); + projectRoot.Sdk = sdk; + var sdkCollection = new SdkCollection(projectRoot); + + // Assert + Assert.Contains(sdk, sdkCollection); + } + + [InlineData(WebSDK)] + [Theory] + public void SdkCollectionNotContainsTest(string sdk) + { + // Arrange + using var mock = AutoMock.GetLoose(); + + var projectRoot = ProjectRootElement.Create(); + projectRoot.Sdk = DefaultSDK; + var sdkCollection = new SdkCollection(projectRoot); + + // Assert + Assert.DoesNotContain(sdk, sdkCollection); + } + + [InlineData(WebSDK, new string[] { DefaultSDK, WebSDK })] + [Theory] + public void SdkCollectionAddTest(string sdk, string[] expected) + { + // Arrange + using var mock = AutoMock.GetLoose(); + + var projectRoot = ProjectRootElement.Create(); + projectRoot.Sdk = DefaultSDK; + + var sdkCollection = new SdkCollection(projectRoot); + + // Act + sdkCollection.Add(sdk); + + // Assert + Assert.Equal( + sdkCollection, + expected); + } + + [InlineData(WebSDK, new string[] { DefaultSDK }, true)] + [InlineData("System.NET.Sdk.Test", new string[] { DefaultSDK, WebSDK }, false)] + [Theory] + public void SdkCollectionRemoveTest(string sdkToRemove, string[] expected, bool expectedStatus) + { + // Arrange + using var mock = AutoMock.GetLoose(); + + var projectRoot = ProjectRootElement.Create(); + projectRoot.Sdk = string.Concat(DefaultSDK, ";", WebSDK); + + var sdkCollection = new SdkCollection(projectRoot); + + // Act and Assert + Assert.Equal(expectedStatus, sdkCollection.Remove(sdkToRemove)); + + // Assert + Assert.Equal( + sdkCollection, + expected); + } + } +} diff --git a/tests/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests/WebSdkCleanupAnalyzerTests.cs b/tests/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests/WebSdkCleanupAnalyzerTests.cs index 13166496..2e3ff086 100644 --- a/tests/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests/WebSdkCleanupAnalyzerTests.cs +++ b/tests/extensions/web/Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests/WebSdkCleanupAnalyzerTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -35,16 +36,16 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests } [Theory] - [InlineData("Microsoft.NET.Sdk.Web", new[] { "Microsoft.AspNetCore.App" }, new[] { "Microsoft.AspNetCore.App" })] // Vanilla positive case - [InlineData("Microsoft.NET.Sdk.Web", new[] { "X", "Y", "Microsoft.AspNetCore.App", "Z" }, new[] { "Microsoft.AspNetCore.App" })] // Multiple references - [InlineData("Microsoft.NET.Sdk.Web", new[] { "Microsoft.AspNetCore.App", "Microsoft.AspNetCore.App" }, new[] { "Microsoft.AspNetCore.App" })] // Duplicate references - [InlineData("microsoft.NET.SDK.Web", new[] { "microsoft.ASPNetCore.App" }, new[] { "microsoft.ASPNetCore.App" })] // Ununusual case - [InlineData("Microsoft.NET.Sdk", new[] { "Microsoft.AspNetCore.App" }, new string[0])] // Wrong SDK - [InlineData("Microsoft.NET.Sdk.Web", new[] { "A", "B", "C" }, new string[0])] // Wrong framework reference + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, new[] { "Microsoft.AspNetCore.App" }, new[] { "Microsoft.AspNetCore.App" })] // Vanilla positive case + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, new[] { "X", "Y", "Microsoft.AspNetCore.App", "Z" }, new[] { "Microsoft.AspNetCore.App" })] // Multiple references + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, new[] { "Microsoft.AspNetCore.App", "Microsoft.AspNetCore.App" }, new[] { "Microsoft.AspNetCore.App" })] // Duplicate references + [InlineData(new[] { "microsoft.NET.SDK.Web" }, new[] { "microsoft.ASPNetCore.App" }, new[] { "microsoft.ASPNetCore.App" })] // Ununusual case + [InlineData(new[] { "Microsoft.NET.Sdk" }, new[] { "Microsoft.AspNetCore.App" }, new string[0])] // Wrong SDK + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, new[] { "A", "B", "C" }, new string[0])] // Wrong framework reference [InlineData(null, new[] { "Microsoft.AspNetCore.App" }, new string[0])] // No SDK - [InlineData("Microsoft.NET.Sdk.Web", new string[0], new string[0])] // No framework reference - [InlineData("Microsoft.NET.Sdk.Web", null, new string[0])] // Null framework references - public async Task AnalyzeAsyncTests(string? sdk, string[]? frameworkReferences, string[] expectedReferencesToRemove) + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, new string[0], new string[0])] // No framework reference + [InlineData(new[] { "Microsoft.NET.Sdk.Web" }, null, new string[0])] // Null framework references + public async Task AnalyzeAsyncTests(string[]? sdk, string[]? frameworkReferences, string[] expectedReferencesToRemove) { // Arrange using var mock = AutoMock.GetLoose(); @@ -86,17 +87,17 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Web.Tests await Assert.ThrowsAsync(() => analyzer.AnalyzeAsync(null!, state.Object, CancellationToken.None)).ConfigureAwait(true); } - private static IProject GetMockProjectAndPackageState(AutoMock mock, string? sdk = null, IEnumerable? frameworkReferences = null) + private static IProject GetMockProjectAndPackageState(AutoMock mock, string[]? sdk = null, IEnumerable? frameworkReferences = null) { var projectRoot = mock.Mock(); projectRoot.Setup(r => r.IsSdk).Returns(sdk is not null); if (sdk is not null) { - projectRoot.Setup(r => r.Sdk).Returns(sdk); + projectRoot.Setup(r => r.Sdk).Returns(new HashSet(sdk, StringComparer.OrdinalIgnoreCase)); } else { - projectRoot.Setup(r => r.Sdk).Throws(); + projectRoot.Setup(r => r.Sdk).Returns(Array.Empty()); } var project = mock.Mock();