Cleanup logs after successful test runs (#40807)

- backport for #39038
- `cherry-pick` of 6958517
- update `AssemblyTestLog` to perform actual log directory cleanup
  - add `ReportTestFailure()` method for tests to report failures, disabling cleanup
  - add `IAcceptFailureReports` for `AspNetTestAssemblyRunner` to report failures to `AssemblyTestLog`
- extend `AspNetTestAssemblyRunner` to optionally create fixture instances using `static ForAssembly(Assembly)`
- add `AssemblyTestLogFixtureAttribute` to register `AssemblyTestLog` as an assembly fixture
  - use `AssemblyTestLogFixtureAttribute` in all test projects
- disable log cleanup in three existing tests

- add tests of new cleanup features
  - also cover a few existing methods
- do cleanup before creating new logger
  - was deleting the just-created global.log file

nits:
- use `is [not] null` and `new()` more
- move `AssemblyTestLogTests` to same namespace as `AssemblyTestLog`

Author: dougbu

src/Mvc/test/Mvc.FunctionalTests/ErrorPageTests.cs

@@ -1,30 +1,25 @@
 // 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.IO;
-using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Net.Http.Headers;
 using System.Text.Encodings.Web;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation;
 using Microsoft.AspNetCore.TestHost;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Testing;
-using Xunit;
 using Xunit.Abstractions;
 
 namespace Microsoft.AspNetCore.Mvc.FunctionalTests
 {
     /// <summary>
     /// Functional test to verify the error reporting of Razor compilation by diagnostic middleware.
     /// </summary>
-    public class ErrorPageTests : IClassFixture<MvcTestFixture<ErrorPageMiddlewareWebSite.Startup>>, IDisposable
+    public class ErrorPageTests : IClassFixture<MvcTestFixture<ErrorPageMiddlewareWebSite.Startup>>
     {
         private static readonly string PreserveCompilationContextMessage = HtmlEncoder.Default.Encode(
             "One or more compilation references may be missing. " +
@@ -189,10 +184,5 @@ public async Task AggregateException_FlattensInnerExceptions()
             Assert.Contains(nullReferenceException, content);
             Assert.Contains(indexOutOfRangeException, content);
         }
-
-        public void Dispose()
-        {
-            _assemblyTestLog.Dispose();
-        }
     }
 }

src/Testing/src/AssemblyTestLog.cs

@@ -7,10 +7,8 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
-using System.Linq;
 using System.Reflection;
 using System.Runtime.CompilerServices;
-using System.Text;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Serilog;
@@ -22,20 +20,21 @@
 
 namespace Microsoft.AspNetCore.Testing
 {
-    public class AssemblyTestLog : IDisposable
+    public class AssemblyTestLog : IAcceptFailureReports, IDisposable
     {
         private const string MaxPathLengthEnvironmentVariableName = "ASPNETCORE_TEST_LOG_MAXPATH";
         private const string LogFileExtension = ".log";
         private static readonly int MaxPathLength = GetMaxPathLength();
 
-        private static readonly object _lock = new object();
-        private static readonly Dictionary<Assembly, AssemblyTestLog> _logs = new Dictionary<Assembly, AssemblyTestLog>();
+        private static readonly object _lock = new();
+        private static readonly Dictionary<Assembly, AssemblyTestLog> _logs = new();
 
         private readonly ILoggerFactory _globalLoggerFactory;
         private readonly ILogger _globalLogger;
         private readonly string _baseDirectory;
         private readonly Assembly _assembly;
         private readonly IServiceProvider _serviceProvider;
+        private bool _testFailureReported;
 
         private static int GetMaxPathLength()
         {
@@ -53,6 +52,9 @@ private AssemblyTestLog(ILoggerFactory globalLoggerFactory, ILogger globalLogger
             _serviceProvider = serviceProvider;
         }
 
+        // internal for testing
+        internal bool OnCI { get; set; } = SkipOnCIAttribute.OnCI();
+
         [SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "Required to maintain compatibility")]
         public IDisposable StartTestLog(ITestOutputHelper output, string className, out ILoggerFactory loggerFactory, [CallerMemberName] string testName = null) =>
             StartTestLog(output, className, out loggerFactory, LogLevel.Debug, testName);
@@ -178,11 +180,8 @@ public IServiceProvider CreateLoggerServices(ITestOutputHelper output, string cl
             return serviceCollection.BuildServiceProvider();
         }
 
-        // For back compat
-        public static AssemblyTestLog Create(string assemblyName, string baseDirectory)
-            => Create(Assembly.Load(new AssemblyName(assemblyName)), baseDirectory);
-
-        public static AssemblyTestLog Create(Assembly assembly, string baseDirectory)
+        // internal for testing. Expectation is AspNetTestAssembly runner calls ForAssembly() first for every Assembly.
+        internal static AssemblyTestLog Create(Assembly assembly, string baseDirectory)
         {
             var logStart = DateTimeOffset.UtcNow;
             SerilogLoggerProvider serilogLoggerProvider = null;
@@ -224,26 +223,46 @@ public static AssemblyTestLog ForAssembly(Assembly assembly)
             {
                 if (!_logs.TryGetValue(assembly, out var log))
                 {
-                    var baseDirectory = TestFileOutputContext.GetOutputDirectory(assembly);
+                    var stackTrace = Environment.StackTrace;
+                    if (!stackTrace.Contains(
+                        "Microsoft.AspNetCore.Testing"
+#if NETCOREAPP
+                        , StringComparison.Ordinal
+#endif
+                        ))
+                    {
+                        throw new InvalidOperationException($"Unexpected initial {nameof(ForAssembly)} caller.");
+                    }
 
-                    log = Create(assembly, baseDirectory);
-                    _logs[assembly] = log;
+                    var baseDirectory = TestFileOutputContext.GetOutputDirectory(assembly);
 
-                    // Try to clear previous logs, continue if it fails.
+                    // Try to clear previous logs, continue if it fails. Do this before creating new global logger.
                     var assemblyBaseDirectory = TestFileOutputContext.GetAssemblyBaseDirectory(assembly);
-                    if (!string.IsNullOrEmpty(assemblyBaseDirectory) && !TestFileOutputContext.GetPreserveExistingLogsInOutput(assembly))
+                    if (!string.IsNullOrEmpty(assemblyBaseDirectory) &&
+                        !TestFileOutputContext.GetPreserveExistingLogsInOutput(assembly))
                     {
                         try
                         {
                             Directory.Delete(assemblyBaseDirectory, recursive: true);
                         }
-                        catch { }
+                        catch
+                        {
+                        }
                     }
+
+                    log = Create(assembly, baseDirectory);
+                    _logs[assembly] = log;
                 }
+
                 return log;
             }
         }
 
+        public void ReportTestFailure()
+        {
+            _testFailureReported = true;
+        }
+
         private static TestFrameworkFileLoggerAttribute GetFileLoggerAttribute(Assembly assembly)
             => assembly.GetCustomAttribute<TestFrameworkFileLoggerAttribute>()
                 ?? throw new InvalidOperationException($"No {nameof(TestFrameworkFileLoggerAttribute)} found on the assembly {assembly.GetName().Name}. "
@@ -269,13 +288,32 @@ private static SerilogLoggerProvider ConfigureFileLogging(string fileName, DateT
                 .MinimumLevel.Verbose()
                 .WriteTo.File(fileName, outputTemplate: "[{TimestampOffset}] [{SourceContext}] [{Level}] {Message:l}{NewLine}{Exception}", flushToDiskInterval: TimeSpan.FromSeconds(1), shared: true)
                 .CreateLogger();
+
             return new SerilogLoggerProvider(serilogger, dispose: true);
         }
 
-        public void Dispose()
+        void IDisposable.Dispose()
         {
             (_serviceProvider as IDisposable)?.Dispose();
             _globalLoggerFactory.Dispose();
+
+            // Clean up if no tests failed and we're not running local tests. (Ignoring tests of this class, OnCI is
+            // true on both build and Helix agents.) In particular, remove the directory containing the global.log
+            // file. All test class log files for this assembly are in subdirectories of this location.
+            if (!_testFailureReported &&
+                OnCI &&
+                _baseDirectory is not null &&
+                Directory.Exists(_baseDirectory))
+            {
+                try
+                {
+                    Directory.Delete(_baseDirectory, recursive: true);
+                }
+                catch
+                {
+                    // Best effort. Ignore problems deleting locked logged files.
+                }
+            }
         }
 
         private class AssemblyLogTimestampOffsetEnricher : ILogEventEnricher

src/Testing/src/AssemblyTestLogFixtureAttribute.cs

@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Testing;
+
+public class AssemblyTestLogFixtureAttribute : AssemblyFixtureAttribute
+{
+    public AssemblyTestLogFixtureAttribute() : base(typeof(AssemblyTestLog))
+    {
+    }
+}

src/Testing/src/build/Microsoft.AspNetCore.Testing.props

@@ -11,8 +11,8 @@
   </PropertyGroup>
 
   <Target Name="SetLoggingTestingAssemblyAttributes"
-    BeforeTargets="GetAssemblyAttributes"
-    Condition="'$(GenerateLoggingTestingAssemblyAttributes)' != 'false'">
+      BeforeTargets="GetAssemblyAttributes"
+      Condition="'$(GenerateLoggingTestingAssemblyAttributes)' != 'false'">
     <PropertyGroup>
       <PreserveExistingLogsInOutput Condition="'$(PreserveExistingLogsInOutput)' == '' AND '$(ContinuousIntegrationBuild)' == 'true'">true</PreserveExistingLogsInOutput>
       <PreserveExistingLogsInOutput Condition="'$(PreserveExistingLogsInOutput)' == ''">false</PreserveExistingLogsInOutput>
@@ -24,6 +24,7 @@
         <_Parameter2>Microsoft.AspNetCore.Testing</_Parameter2>
       </AssemblyAttribute>
 
+      <AssemblyAttribute Include="Microsoft.AspNetCore.Testing.AssemblyTestLogFixtureAttribute" />
       <AssemblyAttribute Include="Microsoft.AspNetCore.Testing.TestFrameworkFileLoggerAttribute">
         <_Parameter1>$(PreserveExistingLogsInOutput)</_Parameter1>
         <_Parameter2>$(TargetFramework)</_Parameter2>

src/Testing/src/xunit/AspNetTestAssemblyRunner.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Reflection;
 using System.Threading;
 using System.Threading.Tasks;
 using Xunit;
@@ -14,7 +15,7 @@ namespace Microsoft.AspNetCore.Testing
 {
     public class AspNetTestAssemblyRunner : XunitTestAssemblyRunner
     {
-        private readonly Dictionary<Type, object> _assemblyFixtureMappings = new Dictionary<Type, object>();
+        private readonly Dictionary<Type, object> _assemblyFixtureMappings = new();
 
         public AspNetTestAssemblyRunner(
             ITestAssembly testAssembly,
@@ -26,31 +27,49 @@ public AspNetTestAssemblyRunner(
         {
         }
 
+        // internal for testing
+        internal IEnumerable<object> Fixtures => _assemblyFixtureMappings.Values;
+
         protected override async Task AfterTestAssemblyStartingAsync()
         {
             await base.AfterTestAssemblyStartingAsync();
 
             // Find all the AssemblyFixtureAttributes on the test assembly
             await Aggregator.RunAsync(async () =>
             {
-                var fixturesAttributes = ((IReflectionAssemblyInfo)TestAssembly.Assembly)
-                    .Assembly
+                var assembly = ((IReflectionAssemblyInfo)TestAssembly.Assembly).Assembly;
+                var fixturesAttributes = assembly
                     .GetCustomAttributes(typeof(AssemblyFixtureAttribute), false)
                     .Cast<AssemblyFixtureAttribute>()
                     .ToList();
 
                 // Instantiate all the fixtures
                 foreach (var fixtureAttribute in fixturesAttributes)
                 {
-                    var ctorWithDiagnostics = fixtureAttribute.FixtureType.GetConstructor(new[] { typeof(IMessageSink) });
                     object instance = null;
-                    if (ctorWithDiagnostics != null)
+                    var staticCreator = fixtureAttribute.FixtureType.GetMethod(
+                        name: "ForAssembly",
+                        bindingAttr: BindingFlags.Public | BindingFlags.Static,
+                        binder: null,
+                        types: new[] { typeof(Assembly) },
+                        modifiers: null);
+                    if (staticCreator is null)
                     {
-                        instance = Activator.CreateInstance(fixtureAttribute.FixtureType, DiagnosticMessageSink);
+                        var ctorWithDiagnostics = fixtureAttribute
+                            .FixtureType
+                            .GetConstructor(new[] { typeof(IMessageSink) });
+                        if (ctorWithDiagnostics is null)
+                        {
+                            instance = Activator.CreateInstance(fixtureAttribute.FixtureType);
+                        }
+                        else
+                        {
+                            instance = Activator.CreateInstance(fixtureAttribute.FixtureType, DiagnosticMessageSink);
+                        }
                     }
                     else
                     {
-                        instance = Activator.CreateInstance(fixtureAttribute.FixtureType);
+                        instance = staticCreator.Invoke(obj: null, parameters: new[] { assembly });
                     }
 
                     _assemblyFixtureMappings[fixtureAttribute.FixtureType] = instance;
@@ -66,32 +85,44 @@ await Aggregator.RunAsync(async () =>
         protected override async Task BeforeTestAssemblyFinishedAsync()
         {
             // Dispose fixtures
-            foreach (var disposable in _assemblyFixtureMappings.Values.OfType<IDisposable>())
+            foreach (var disposable in Fixtures.OfType<IDisposable>())
             {
                 Aggregator.Run(disposable.Dispose);
             }
 
-            foreach (var disposable in _assemblyFixtureMappings.Values.OfType<IAsyncLifetime>())
+            foreach (var disposable in Fixtures.OfType<IAsyncLifetime>())
             {
                 await Aggregator.RunAsync(disposable.DisposeAsync);
             }
 
             await base.BeforeTestAssemblyFinishedAsync();
         }
 
-        protected override Task<RunSummary> RunTestCollectionAsync(
+        protected override async Task<RunSummary> RunTestCollectionAsync(
             IMessageBus messageBus,
             ITestCollection testCollection,
             IEnumerable<IXunitTestCase> testCases,
             CancellationTokenSource cancellationTokenSource)
-            => new AspNetTestCollectionRunner(
+        {
+            var runSummary = await new AspNetTestCollectionRunner(
                 _assemblyFixtureMappings,
                 testCollection,
                 testCases,
                 DiagnosticMessageSink,
                 messageBus,
                 TestCaseOrderer,
                 new ExceptionAggregator(Aggregator),
-                cancellationTokenSource).RunAsync();
+                cancellationTokenSource)
+                .RunAsync();
+            if (runSummary.Failed != 0)
+            {
+                foreach (var fixture in Fixtures.OfType<IAcceptFailureReports>())
+                {
+                    fixture.ReportTestFailure();
+                }
+            }
+
+            return runSummary;
+        }
     }
 }

src/Testing/src/xunit/IAcceptFailureReports.cs

@@ -0,0 +1,9 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.AspNetCore.Testing;
+
+internal interface IAcceptFailureReports
+{
+    void ReportTestFailure();
+}