Update to latest Xunit and Moq, fix test bugs, stop suppressing analyzers (#10077)

* Fix first wave of warnings (not suppressed by .editorconfig)
* Move to latest test dependencies (xunit and Moq)
* Fix Moq build errors
* Only build MVC view on Release
* Fix warnings in NuGetGallery.Core
* Fix non-Xunit warnings (except App Insights connection string)
* Fix tests that weren't running
* Move to matching NetAnalyzers
* Delete .editorconfig suppression now that we're clean
* Move to latest working Moq, 4.11.0 breaks controller tests so fixes were needed

Author: joelverhagen

.editorconfig

@@ -1,39 +0,0 @@
-[*.cs]
-
-# Suppress performance, usage, naming, maintainability, style diagnostics.
-dotnet_analyzer_diagnostic.category-design.severity = none
-dotnet_analyzer_diagnostic.category-globalization.severity = none
-dotnet_analyzer_diagnostic.category-naming.severity = none
-dotnet_analyzer_diagnostic.category-performance.severity = none
-dotnet_analyzer_diagnostic.category-reliability.severity = none
-dotnet_analyzer_diagnostic.category-security.severity = warning
-dotnet_analyzer_diagnostic.category-usage.severity = none
-dotnet_analyzer_diagnostic.category-Maintainability.severity = none
-dotnet_analyzer_diagnostic.category-Style.severity = none
-
-# Addtional suppress to be compatible with different version among VS2019
-
-# CA1001: Types that own disposable fields should be disposable
-dotnet_diagnostic.CA1001.severity = none
-
-# CA1063: Implement IDisposable Correctly
-dotnet_diagnostic.CA1063.severity = none
-
-# CA1033: Interface methods should be callable by child types
-dotnet_diagnostic.CA1033.severity = none
-
-# CA2229: Implement serialization constructors
-dotnet_diagnostic.CA2229.severity = none
-
-# CA2200: Rethrow to preserve stack details.
-dotnet_diagnostic.CA2200.severity = none
-
-# CA1065: Do not raise exceptions in unexpected locations
-dotnet_diagnostic.CA1065.severity = none
-
-# CA2214: Do not call overridable methods in constructors
-dotnet_diagnostic.CA2214.severity = none
-
-# Suppressions we want to reset
-dotnet_diagnostic.CA2017.severity = warning
-dotnet_diagnostic.IDE0005.severity = default # unnecessary usings

.nuget/packages.config

@@ -3,5 +3,4 @@
   <package id="MicroBuild.Core" version="0.3.0" />
   <package id="Microsoft.CodeAnalysis.BinSkim" version="1.3.6" />
   <package id="xunit.runner.console" version="2.1.0" />
-  <package id="xunit.runner.visualstudio" version="2.1.0" />
 </packages>

Directory.Build.props

@@ -1,12 +1,14 @@
 <Project>
   <PropertyGroup>
+    <LangVersion>9.0</LangVersion>
+
     <NuGetClientPackageVersion>6.9.1</NuGetClientPackageVersion>
     <ServerCommonPackageVersion>2.120.0</ServerCommonPackageVersion>
     <NuGetJobsPackageVersion>4.3.0-agr-gal-stsdk-9768098</NuGetJobsPackageVersion>
   </PropertyGroup>
   <ItemGroup>
     <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers">
-      <Version>7.0.3</Version>
+      <Version>8.0.0</Version>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
       <PrivateAssets>all</PrivateAssets>
     </PackageReference>

build.ps1

@@ -101,7 +101,8 @@ Invoke-BuildStep 'Removing .editorconfig file in NuGetGallery' { Remove-Editorco
 
 Invoke-BuildStep 'Building solution' { 
     $SolutionPath = Join-Path $PSScriptRoot "NuGetGallery.sln"
-    Build-Solution -Configuration $Configuration -BuildNumber $BuildNumber -SolutionPath $SolutionPath -SkipRestore:$SkipRestore -MSBuildProperties "/p:MvcBuildViews=true" `
+    $MvcBuildViews = $Configuration -eq "release"
+    Build-Solution -Configuration $Configuration -BuildNumber $BuildNumber -SolutionPath $SolutionPath -SkipRestore:$SkipRestore -MSBuildProperties "/p:MvcBuildViews=$MvcBuildViews" `
 } `
 -ev +BuildErrors
 

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

@@ -56,8 +56,8 @@ public async Task WriteVulnerabilityAsync(PackageVulnerability vulnerability, bo
             }
             catch (Exception ex)
             {
-                _logger.LogError("Failed to write vulnerability {GitHubAdvisoryUrl}", vulnerability.AdvisoryUrl);
-                throw ex;
+                _logger.LogError(ex, "Failed to write vulnerability {GitHubAdvisoryUrl}", vulnerability.AdvisoryUrl);
+                throw;
             }
         }
     }

src/GitHubVulnerabilities2v3/Extensions/BlobStorageVulnerabilityWriter.cs

@@ -53,7 +53,10 @@ public BlobStorageVulnerabilityWriter(
             _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
 
             _storage = _storageFactory.Create();
-            ((AzureStorage)_storage).CompressContent = _configuration.GzipFileContent;
+            if (_storage is AzureStorage azureStorage)
+            {
+                azureStorage.CompressContent = _configuration.GzipFileContent;
+            }
 
             _firstVulnWrittenTimestamp = DateTimeOffset.MinValue;
             _vulnerabilityDict = new Dictionary<string, List<Advisory>>(StringComparer.OrdinalIgnoreCase);

src/GitHubVulnerabilities2v3/GitHubVulnerabilities2V3.csproj

@@ -93,9 +93,6 @@
       <Name>NuGetGallery.Services</Name>
     </ProjectReference>
   </ItemGroup>
-  <ItemGroup>
-    <Folder Include="Properties\" />
-  </ItemGroup>
   <PropertyGroup>
     <SignPath>..\..\build</SignPath>
     <SignPath Condition="'$(BUILD_SOURCESDIRECTORY)' != ''">$(BUILD_SOURCESDIRECTORY)\build</SignPath>

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -8,6 +8,7 @@
 using System.Data.Entity.Infrastructure;
 using System.Data.Entity.Infrastructure.Annotations;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 using NuGet.Services.Entities;
 
@@ -97,14 +98,30 @@ DbSet<T> IReadOnlyEntitiesContext.Set<T>()
             return base.Set<T>();
         }
 
+        public override int SaveChanges()
+        {
+            ThrowIfReadOnly();
+            return base.SaveChanges();
+        }
+
+        public override Task<int> SaveChangesAsync(CancellationToken cancellationToken)
+        {
+            ThrowIfReadOnly();
+            return base.SaveChangesAsync(cancellationToken);
+        }
+
         public override async Task<int> SaveChangesAsync()
+        {
+            ThrowIfReadOnly();
+            return await base.SaveChangesAsync();
+        }
+
+        private void ThrowIfReadOnly()
         {
             if (ReadOnly)
             {
                 throw new ReadOnlyModeException("Save changes unavailable: the gallery is currently in read only mode, with limited service. Please try again later.");
             }
-
-            return await base.SaveChangesAsync();
         }
 
         public void DeleteOnCommit<T>(T entity) where T : class

src/NuGetGallery/Controllers/JsonApiController.cs

@@ -123,7 +123,7 @@ public async Task<JsonResult> AddPackageOwner(AddPackageOwnerViewModel addOwnerD
             string username = addOwnerData.Username;
             string message = addOwnerData.Message;
 
-            if (Regex.IsMatch(username, GalleryConstants.EmailValidationRegex, RegexOptions.None, GalleryConstants.EmailValidationRegexTimeout))
+            if (username is not null && Regex.IsMatch(username, GalleryConstants.EmailValidationRegex, RegexOptions.None, GalleryConstants.EmailValidationRegexTimeout))
             {
                 return Json(new { success = false, message = Strings.AddOwner_NameIsEmail }, JsonRequestBehavior.AllowGet);
             }

test.ps1

@@ -1,11 +1,10 @@
-[CmdletBinding(DefaultParameterSetName='RegularBuild')]
+[CmdletBinding(DefaultParameterSetName = 'RegularBuild')]
 param (
     [ValidateSet("debug", "release")]
     [string]$Configuration = 'debug',
     [int]$BuildNumber
 )
 
-# For TeamCity - If any issue occurs, this script fail the build. - By default, TeamCity returns an exit code of 0 for all powershell scripts, even if they fail
 trap {
     Write-Host "BUILD FAILED: $_" -ForegroundColor Red
     Write-Host "ERROR DETAILS:" -ForegroundColor Red
@@ -16,28 +15,33 @@ trap {
 
 . "$PSScriptRoot\build\common.ps1"
 
-Function Run-Tests {
+Function Invoke-Tests {
     [CmdletBinding()]
     param()
 
     Trace-Log 'Running tests'
 
     $xUnitExe = (Join-Path $PSScriptRoot "packages\xunit.runner.console\tools\xunit.console.exe")
 
-    $TestAssemblies = `
-        "tests\AccountDeleter.Facts\bin\$Configuration\AccountDeleter.Facts.dll", `
-        "tests\GitHubVulnerabilities2Db.Facts\bin\$Configuration\GitHubVulnerabilities2Db.Facts.dll", `
+    $GalleryTestAssemblies = `
+        "tests\AccountDeleter.Facts\bin\$Configuration\net472\AccountDeleter.Facts.dll", `
+        "tests\GitHubVulnerabilities2Db.Facts\bin\$Configuration\net472\GitHubVulnerabilities2Db.Facts.dll", `
         "tests\GitHubVulnerabilities2v3.Facts\bin\$Configuration\GitHubVulnerabilities2v3.Facts.dll", `
-        "tests\NuGet.Services.DatabaseMigration.Facts\bin\$Configuration\NuGet.Services.DatabaseMigration.Facts.dll", `
-        "tests\NuGet.Services.Entities.Tests\bin\$Configuration\NuGet.Services.Entities.Tests.dll", `
+        "tests\NuGet.Services.DatabaseMigration.Facts\bin\$Configuration\net472\NuGet.Services.DatabaseMigration.Facts.dll", `
+        "tests\NuGet.Services.Entities.Tests\bin\$Configuration\net472\NuGet.Services.Entities.Tests.dll", `
         "tests\NuGetGallery.Core.Facts\bin\$Configuration\NuGetGallery.Core.Facts.dll", `
         "tests\NuGetGallery.Facts\bin\$Configuration\NuGetGallery.Facts.dll", `
         "tests\VerifyMicrosoftPackage.Facts\bin\$Configuration\NuGet.VerifyMicrosoftPackage.Facts.dll"
 
     $TestCount = 0
-    
-    foreach ($Test in $TestAssemblies) {
-        & $xUnitExe (Join-Path $PSScriptRoot $Test) -xml "Results.$TestCount.xml"
+
+    $GalleryTestAssemblies | ForEach-Object {
+        $TestResultFile = Join-Path $PSScriptRoot "Results.$TestCount.xml"
+        & $xUnitExe (Join-Path $PSScriptRoot $_) -xml $TestResultFile
+        if (-not (Test-Path $TestResultFile)) {
+            Write-Error "The test run failed to produce a result file";
+            exit 1;
+        }
         $TestCount++
     }
 
@@ -56,7 +60,7 @@ Trace-Log "Build #$BuildNumber started at $startTime"
 
 $BuildErrors = @()
 
-Invoke-BuildStep 'Running tests' { Run-Tests } `
+Invoke-BuildStep 'Running tests' { Invoke-Tests } `
     -ev +BuildErrors
 
 Trace-Log ('-' * 60)
@@ -69,7 +73,7 @@ Trace-Log "Time elapsed $(Format-ElapsedTime ($endTime - $startTime))"
 Trace-Log ('=' * 60)
 
 if ($BuildErrors) {
-    $ErrorLines = $BuildErrors | %{ ">>> $($_.Exception.Message)" }
+    $ErrorLines = $BuildErrors | ForEach-Object { ">>> $($_.Exception.Message)" }
     Error-Log "Tests completed with $($BuildErrors.Count) error(s):`r`n$($ErrorLines -join "`r`n")" -Fatal
 }