Cleanup logs after successful test runs

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

@@ -18,7 +18,7 @@ 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. " +
@@ -183,9 +183,4 @@ public async Task AggregateException_FlattensInnerExceptions()
         Assert.Contains(nullReferenceException, content);
         Assert.Contains(indexOutOfRangeException, content);
     }
-
-    public void Dispose()
-    {
-        _assemblyTestLog.Dispose();
-    }
 }

src/Testing/src/AssemblyTestLog.cs

@@ -20,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()
     {
@@ -51,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);
@@ -176,7 +180,8 @@ public IServiceProvider CreateLoggerServices(ITestOutputHelper output, string cl
         return serviceCollection.BuildServiceProvider();
     }
 
-    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;
@@ -218,12 +223,20 @@ 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))
@@ -232,13 +245,24 @@ public static AssemblyTestLog ForAssembly(Assembly assembly)
                     {
                         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}. "
@@ -268,10 +292,28 @@ private static SerilogLoggerProvider ConfigureFileLogging(string fileName, DateT
         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().ConfigureAwait(false);
 
         // 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,31 +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).ConfigureAwait(false);
         }
 
         await base.BeforeTestAssemblyFinishedAsync().ConfigureAwait(false);
     }
 
-    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()
+            .ConfigureAwait(false);
+        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();
+}