[rel/3.8] Consider --results-directory before configuration (#5204)

Youssef1313 · web-flow · commit ccbaf506f485 · 2025-03-11T09:25:46.000+01:00
diff --git a/src/Platform/Microsoft.Testing.Platform/Configurations/AggregatedConfiguration.cs b/src/Platform/Microsoft.Testing.Platform/Configurations/AggregatedConfiguration.cs
@@ -1,4 +1,4 @@
-﻿// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using Microsoft.Testing.Platform.CommandLine;
@@ -8,27 +8,34 @@
 
 namespace Microsoft.Testing.Platform.Configurations;
 
-internal sealed class AggregatedConfiguration(IConfigurationProvider[] configurationProviders, ITestApplicationModuleInfo testApplicationModuleInfo, IFileSystem fileSystem) : IConfiguration
+internal sealed class AggregatedConfiguration(
+    IConfigurationProvider[] configurationProviders,
+    ITestApplicationModuleInfo testApplicationModuleInfo,
+    IFileSystem fileSystem,
+    CommandLineParseResult commandLineParseResult) : IConfiguration
 {
     public const string DefaultTestResultFolderName = "TestResults";
     private readonly IConfigurationProvider[] _configurationProviders = configurationProviders;
     private readonly ITestApplicationModuleInfo _testApplicationModuleInfo = testApplicationModuleInfo;
     private readonly IFileSystem _fileSystem = fileSystem;
-    private string? _resultDirectory;
+    private readonly CommandLineParseResult _commandLineParseResult = commandLineParseResult;
+    private string? _resultsDirectory;
     private string? _currentWorkingDirectory;
     private string? _testHostWorkingDirectory;
 
     public string? this[string key]
     {
         get
         {
-            if (key == PlatformConfigurationConstants.PlatformResultDirectory && _resultDirectory is not null)
+            if (key == PlatformConfigurationConstants.PlatformResultDirectory)
             {
-                return _resultDirectory;
+                _resultsDirectory = GetResultsDirectoryCore(_commandLineParseResult);
+                return _resultsDirectory;
             }
 
-            if (key == PlatformConfigurationConstants.PlatformCurrentWorkingDirectory && _currentWorkingDirectory is not null)
+            if (key == PlatformConfigurationConstants.PlatformCurrentWorkingDirectory)
             {
+                _currentWorkingDirectory = GetCurrentWorkingDirectoryCore();
                 return _currentWorkingDirectory;
             }
 
@@ -37,55 +44,73 @@ public string? this[string key]
                 return _testHostWorkingDirectory;
             }
 
-            foreach (IConfigurationProvider source in _configurationProviders)
+            // Fallback to calculating from configuration providers.
+            return CalculateFromConfigurationProviders(key);
+        }
+    }
+
+    private string? CalculateFromConfigurationProviders(string key)
+    {
+        foreach (IConfigurationProvider source in _configurationProviders)
+        {
+            if (source.TryGet(key, out string? value))
             {
-                if (source.TryGet(key, out string? value))
-                {
-                    return value;
-                }
+                return value;
             }
-
-            return null;
         }
-    }
 
-    public /* for testing */ void SetResultDirectory(string resultDirectory) =>
-        _resultDirectory = Guard.NotNull(resultDirectory);
+        return null;
+    }
 
     public /* for testing */ void SetCurrentWorkingDirectory(string workingDirectory) =>
         _currentWorkingDirectory = Guard.NotNull(workingDirectory);
 
     public void SetTestHostWorkingDirectory(string workingDirectory) =>
         _testHostWorkingDirectory = Guard.NotNull(workingDirectory);
 
-    public async Task CheckTestResultsDirectoryOverrideAndCreateItAsync(ICommandLineOptions commandLineOptions, IFileLoggerProvider? fileLoggerProvider)
+    public async Task CheckTestResultsDirectoryOverrideAndCreateItAsync(IFileLoggerProvider? fileLoggerProvider)
     {
-        // Load Configuration
-        _currentWorkingDirectory = Path.GetDirectoryName(_testApplicationModuleInfo.GetCurrentTestApplicationFullPath())!;
+        _resultsDirectory = _fileSystem.CreateDirectory(this[PlatformConfigurationConstants.PlatformResultDirectory]!);
 
-        string? resultDirectory = this[PlatformConfigurationConstants.PlatformResultDirectory];
-        if (resultDirectory is null)
+        // In case of the result directory is overridden by the config file we move logs to it.
+        // This can happen in case of VSTest mode where the result directory is set to a different location.
+        // This behavior is non documented and we reserve the right to change it in the future.
+        if (fileLoggerProvider is not null)
         {
-            if (commandLineOptions.TryGetOptionArgumentList(PlatformCommandLineProvider.ResultDirectoryOptionKey, out string[]? resultDirectoryArg))
-            {
-                _resultDirectory = resultDirectoryArg[0];
-            }
+            await fileLoggerProvider.CheckLogFolderAndMoveToTheNewIfNeededAsync(_resultsDirectory);
         }
-        else
+    }
+
+    private string GetResultsDirectoryCore(CommandLineParseResult commandLineParseResult)
+    {
+        // We already calculated it before, or was set by unit tests via SetCurrentWorkingDirectory.
+        if (_resultsDirectory is not null)
         {
-            _resultDirectory = resultDirectory;
+            return _resultsDirectory;
         }
 
-        _resultDirectory ??= Path.Combine(_currentWorkingDirectory, DefaultTestResultFolderName);
-
-        _resultDirectory = _fileSystem.CreateDirectory(_resultDirectory);
-
-        // In case of the result directory is overridden by the config file we move logs to it.
-        // This can happen in case of VSTest mode where the result directory is set to a different location.
-        // This behavior is non documented and we reserve the right to change it in the future.
-        if (fileLoggerProvider is not null)
+        // NOTE: It's important to consider the `--results-directory` option before the configurations.
+        // It makes more sense to respect it first, and is also relevant for Retry extension.
+        // Consider user is setting ResultsDirectory in configuration file.
+        // When tests are retried, we re-run the executable again, but we pass --results-directory containing the folder where the current retry attempt
+        // results should be stored. But we will also still pass configuration file (e.g, runsettings via --settings) that the user originally specified.
+        // In that case, we will want to respect --results-directory.
+        if (commandLineParseResult.TryGetOptionArgumentList(PlatformCommandLineProvider.ResultDirectoryOptionKey, out string[]? resultDirectoryArg))
         {
-            await fileLoggerProvider.CheckLogFolderAndMoveToTheNewIfNeededAsync(_resultDirectory);
+            return resultDirectoryArg[0];
         }
+
+        // If not specified by command line, then use the configuration providers.
+        // And finally fallback to DefaultTestResultFolderName relative to the current working directory.
+        return CalculateFromConfigurationProviders(PlatformConfigurationConstants.PlatformResultDirectory)
+            ?? Path.Combine(this[PlatformConfigurationConstants.PlatformCurrentWorkingDirectory]!, DefaultTestResultFolderName);
     }
+
+    private string GetCurrentWorkingDirectoryCore()
+         // If the value is already set, use that.
+         => _currentWorkingDirectory
+            // If first time calculating it, prefer the value from configuration,
+            ?? CalculateFromConfigurationProviders(PlatformConfigurationConstants.PlatformCurrentWorkingDirectory)
+            // then fallback to the actual working directory.
+            ?? Path.GetDirectoryName(_testApplicationModuleInfo.GetCurrentTestApplicationFullPath())!;
 }
diff --git a/src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationExtensions.cs b/src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationExtensions.cs
@@ -35,6 +35,7 @@ public static string GetCurrentWorkingDirectory(this IConfiguration configuratio
     /// </summary>
     /// <param name="configuration">The configuration.</param>
     /// <returns>The test host working directory.</returns>
+    // TODO: This is only used in unit tests. Should that be removed (maybe in v2)?
     public static string GetTestHostWorkingDirectory(this IConfiguration configuration)
     {
         string? workingDirectory = configuration[PlatformConfigurationConstants.PlatformTestHostWorkingDirectory];
diff --git a/src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationManager.cs b/src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationManager.cs
@@ -58,6 +58,6 @@ internal async Task<IConfiguration> BuildAsync(IFileLoggerProvider? syncFileLogg
 
         return defaultJsonConfiguration is null
             ? throw new InvalidOperationException(PlatformResources.ConfigurationManagerCannotFindDefaultJsonConfigurationErrorMessage)
-            : new AggregatedConfiguration(configurationProviders.OrderBy(x => x.Order).Select(x => x.ConfigurationProvider).ToArray(), _testApplicationModuleInfo, _fileSystem);
+            : new AggregatedConfiguration(configurationProviders.OrderBy(x => x.Order).Select(x => x.ConfigurationProvider).ToArray(), _testApplicationModuleInfo, _fileSystem, commandLineParseResult);
     }
 }
diff --git a/src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.cs b/src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.cs
@@ -288,7 +288,7 @@ public async Task<ITestHost> BuildAsync(
         // here we ensure to override the result directory if user passed the argument --results-directory in command line.
         // After this check users can get the result directory using IConfiguration["platformOptions:resultDirectory"] or the
         // extension method helper serviceProvider.GetConfiguration()
-        await configuration.CheckTestResultsDirectoryOverrideAndCreateItAsync(commandLineOptions, loggingState.FileLoggerProvider);
+        await configuration.CheckTestResultsDirectoryOverrideAndCreateItAsync(loggingState.FileLoggerProvider);
 
         // Display banner now because we need capture the output in case of MSBuild integration and we want to forward
         // to file disc also the banner, so at this point we need to have all services and configuration(result directory) built.
diff --git a/test/UnitTests/MSTest.Engine.UnitTests/Adapter_ExecuteRequestAsyncTests.cs b/test/UnitTests/MSTest.Engine.UnitTests/Adapter_ExecuteRequestAsyncTests.cs
@@ -116,7 +116,7 @@ public Services()
             ServiceProvider.AddService(new FakeClock());
             ServiceProvider.AddService(new SystemTask());
 #pragma warning disable TPEXP // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
-            ServiceProvider.AddService(new AggregatedConfiguration(Array.Empty<IConfigurationProvider>(), new CurrentTestApplicationModuleInfo(new SystemEnvironment(), new SystemProcessHandler()), new SystemFileSystem()));
+            ServiceProvider.AddService(new AggregatedConfiguration(Array.Empty<IConfigurationProvider>(), new CurrentTestApplicationModuleInfo(new SystemEnvironment(), new SystemProcessHandler()), new SystemFileSystem(), new(null, [], [])));
 #pragma warning restore TPEXP // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
         }
 
diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/AggregatedConfigurationTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/AggregatedConfigurationTests.cs
@@ -22,18 +22,28 @@ public sealed class AggregatedConfigurationTests
     [DataRow(PlatformConfigurationConstants.PlatformResultDirectory)]
     [DataRow(PlatformConfigurationConstants.PlatformCurrentWorkingDirectory)]
     [DataRow(PlatformConfigurationConstants.PlatformTestHostWorkingDirectory)]
-    public void IndexerTest_DirectoryNotSetAndNoConfigurationProviders_DirectoryIsNull(string key)
-        => Assert.IsNull(new AggregatedConfiguration([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object)[key]);
+    public void IndexerTest_DirectoryNotSetAndNoConfigurationProviders(string key)
+    {
+        _testApplicationModuleInfoMock.Setup(x => x.GetCurrentTestApplicationFullPath()).Returns("TestAppDir/test.exe");
+        string? expected = key switch
+        {
+            PlatformConfigurationConstants.PlatformResultDirectory => Path.Combine("TestAppDir", "TestResults"),
+            PlatformConfigurationConstants.PlatformCurrentWorkingDirectory => "TestAppDir",
+            PlatformConfigurationConstants.PlatformTestHostWorkingDirectory => null,
+            _ => throw ApplicationStateGuard.Unreachable(),
+        };
+
+        Assert.AreEqual(expected, new AggregatedConfiguration([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [], []))[key]);
+    }
 
     [TestMethod]
-    [DataRow(PlatformConfigurationConstants.PlatformResultDirectory)]
     [DataRow(PlatformConfigurationConstants.PlatformCurrentWorkingDirectory)]
     [DataRow(PlatformConfigurationConstants.PlatformTestHostWorkingDirectory)]
     public void IndexerTest_DirectoryNotSetButConfigurationProvidersPresent_DirectoryIsNull(string key)
     {
         Mock<IConfigurationProvider> mockProvider = new();
 
-        AggregatedConfiguration aggregatedConfiguration = new([mockProvider.Object], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([mockProvider.Object], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [], []));
         Assert.IsNull(aggregatedConfiguration[key]);
     }
 
@@ -43,23 +53,21 @@ public void IndexerTest_DirectoryNotSetButConfigurationProvidersPresent_Director
     [DataRow(PlatformConfigurationConstants.PlatformTestHostWorkingDirectory)]
     public void IndexerTest_DirectoryNotSetButConfigurationProvidersPresent_DirectoryIsNotNull(string key)
     {
-        AggregatedConfiguration aggregatedConfiguration = new([new FakeConfigurationProvider(ExpectedPath)], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([new FakeConfigurationProvider(ExpectedPath)], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [], []));
         Assert.AreEqual(ExpectedPath, aggregatedConfiguration[key]);
     }
 
     [TestMethod]
     public void IndexerTest_ResultDirectorySet_DirectoryIsNotNull()
     {
-        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object);
-
-        aggregatedConfiguration.SetResultDirectory(ExpectedPath);
+        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [new CommandLineParseOption("results-directory", [ExpectedPath])], []));
         Assert.AreEqual(ExpectedPath, aggregatedConfiguration[PlatformConfigurationConstants.PlatformResultDirectory]);
     }
 
     [TestMethod]
     public void IndexerTest_CurrentWorkingDirectorySet_DirectoryIsNotNull()
     {
-        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [], []));
 
         aggregatedConfiguration.SetCurrentWorkingDirectory(ExpectedPath);
         Assert.AreEqual(ExpectedPath, aggregatedConfiguration[PlatformConfigurationConstants.PlatformCurrentWorkingDirectory]);
@@ -68,7 +76,7 @@ public void IndexerTest_CurrentWorkingDirectorySet_DirectoryIsNotNull()
     [TestMethod]
     public void IndexerTest_TestHostWorkingDirectorySet_DirectoryIsNotNull()
     {
-        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([], _testApplicationModuleInfoMock.Object, _fileSystemMock.Object, new(null, [], []));
 
         aggregatedConfiguration.SetTestHostWorkingDirectory(ExpectedPath);
         Assert.AreEqual(ExpectedPath, aggregatedConfiguration[PlatformConfigurationConstants.PlatformTestHostWorkingDirectory]);
@@ -86,9 +94,8 @@ public async ValueTask CheckTestResultsDirectoryOverrideAndCreateItAsync_Results
         Mock<IFileLoggerProvider> mockFileLogger = new();
         mockFileLogger.Setup(x => x.CheckLogFolderAndMoveToTheNewIfNeededAsync(It.IsAny<string>())).Callback(() => { });
 
-        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object);
-        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(
-            new FakeCommandLineOptions(ExpectedPath), mockFileLogger.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object, new(null, [new CommandLineParseOption("results-directory", [ExpectedPath])], []));
+        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(mockFileLogger.Object);
 
         mockFileSystem.Verify(x => x.CreateDirectory(ExpectedPath), Times.Once);
         mockFileLogger.Verify(x => x.CheckLogFolderAndMoveToTheNewIfNeededAsync(ExpectedPath), Times.Once);
@@ -108,9 +115,8 @@ public async ValueTask CheckTestResultsDirectoryOverrideAndCreateItAsync_Results
         Mock<IFileLoggerProvider> mockFileLogger = new();
         mockFileLogger.Setup(x => x.CheckLogFolderAndMoveToTheNewIfNeededAsync(It.IsAny<string>())).Callback(() => { });
 
-        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object);
-        aggregatedConfiguration.SetResultDirectory(ExpectedPath);
-        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(new FakeCommandLineOptions(ExpectedPath), mockFileLogger.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object, new(null, [new CommandLineParseOption("results-directory", [ExpectedPath])], []));
+        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(mockFileLogger.Object);
 
         mockFileSystem.Verify(x => x.CreateDirectory(ExpectedPath), Times.Once);
         mockFileLogger.Verify(x => x.CheckLogFolderAndMoveToTheNewIfNeededAsync(ExpectedPath), Times.Once);
@@ -130,9 +136,8 @@ public async ValueTask CheckTestResultsDirectoryOverrideAndCreateItAsync_Results
         Mock<IFileLoggerProvider> mockFileLogger = new();
         mockFileLogger.Setup(x => x.CheckLogFolderAndMoveToTheNewIfNeededAsync(It.IsAny<string>())).Callback(() => { });
 
-        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object);
-        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(
-            new FakeCommandLineOptions(ExpectedPath, bypass: true), mockFileLogger.Object);
+        AggregatedConfiguration aggregatedConfiguration = new([], mockTestApplicationModuleInfo.Object, mockFileSystem.Object, new(null, [], []));
+        await aggregatedConfiguration.CheckTestResultsDirectoryOverrideAndCreateItAsync(mockFileLogger.Object);
 
         string expectedPath = "a" + Path.DirectorySeparatorChar + "b" + Path.DirectorySeparatorChar + "TestResults";
         mockFileSystem.Verify(x => x.CreateDirectory(expectedPath), Times.Once);
@@ -166,35 +171,3 @@ public bool TryGet(string key, out string? value)
         }
     }
 }
-
-internal sealed class FakeCommandLineOptions : ICommandLineOptions
-{
-    private readonly string _path;
-    private readonly bool _bypass;
-
-    public FakeCommandLineOptions(string path, bool bypass = false)
-    {
-        _path = path;
-        _bypass = bypass;
-    }
-
-    public bool IsOptionSet(string optionName) => throw new NotImplementedException();
-
-    public bool TryGetOptionArgumentList(string optionName, [NotNullWhen(true)] out string[]? arguments)
-    {
-        arguments = null;
-        if (_bypass)
-        {
-            return false;
-        }
-
-        switch (optionName)
-        {
-            case PlatformCommandLineProvider.ResultDirectoryOptionKey:
-                arguments = [_path];
-                return true;
-            default:
-                return false;
-        }
-    }
-}
diff --git a/test/UnitTests/Microsoft.Testing.Platform.UnitTests/TestApplicationBuilderTests.cs b/test/UnitTests/Microsoft.Testing.Platform.UnitTests/TestApplicationBuilderTests.cs

Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,7 @@ public static string GetCurrentWorkingDirectory(this IConfiguration configuratio`
`35`	`35`	`/// </summary>`
`36`	`36`	`/// <param name="configuration">The configuration.</param>`
`37`	`37`	`/// <returns>The test host working directory.</returns>`
	`38`	`+ // TODO: This is only used in unit tests. Should that be removed (maybe in v2)?`
`38`	`39`	`public static string GetTestHostWorkingDirectory(this IConfiguration configuration)`
`39`	`40`	`{`
`40`	`41`	`string? workingDirectory = configuration[PlatformConfigurationConstants.PlatformTestHostWorkingDirectory];`
Original file line number	Diff line number	Diff line change
`@@ -58,6 +58,6 @@ internal async Task<IConfiguration> BuildAsync(IFileLoggerProvider? syncFileLogg`
`58`	`58`
`59`	`59`	`return defaultJsonConfiguration is null`
`60`	`60`	`? throw new InvalidOperationException(PlatformResources.ConfigurationManagerCannotFindDefaultJsonConfigurationErrorMessage)`
`61`		`- : new AggregatedConfiguration(configurationProviders.OrderBy(x => x.Order).Select(x => x.ConfigurationProvider).ToArray(), _testApplicationModuleInfo, _fileSystem);`
	`61`	`+ : new AggregatedConfiguration(configurationProviders.OrderBy(x => x.Order).Select(x => x.ConfigurationProvider).ToArray(), _testApplicationModuleInfo, _fileSystem, commandLineParseResult);`
`62`	`62`	`}`
`63`	`63`	`}`
Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,7 @@ public Services()`
`116`	`116`	`ServiceProvider.AddService(new FakeClock());`
`117`	`117`	`ServiceProvider.AddService(new SystemTask());`
`118`	`118`	`#pragma warning disable TPEXP // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.`
`119`		`- ServiceProvider.AddService(new AggregatedConfiguration(Array.Empty<IConfigurationProvider>(), new CurrentTestApplicationModuleInfo(new SystemEnvironment(), new SystemProcessHandler()), new SystemFileSystem()));`
	`119`	`+ ServiceProvider.AddService(new AggregatedConfiguration(Array.Empty<IConfigurationProvider>(), new CurrentTestApplicationModuleInfo(new SystemEnvironment(), new SystemProcessHandler()), new SystemFileSystem(), new(null, [], [])));`
`120`	`120`	`#pragma warning restore TPEXP // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.`
`121`	`121`	`}`
`122`	`122`