`Microsoft.Testing.Extensions.Retry` can't move files from TestResults when running from `dotnet test` on Windows

[CheezeCS]

Describe the bug

Hello, when running tests on Windows with dotnet test when using the ‘retry’ parameter, the test run terminates with an error, due to moving a file that is used by another process

Steps To Reproduce

1. Create simple unit test
2. Run it through dotnet test -- --retry-failed-tests 1 on Windows
3. error run failed: Tests failed

Expected behavior

Test run completes without errors

Actual behavior

Test run failed

Additional context

TestResults:

=====================
Tests suite completed successfully in 1 attempts
=====================

=====================
Moving last attempt asset files to the default result directory
=====================

Moving file 'C:\RiderProjects\Allure.Testing.Platform\TestProject1\bin\Debug\net8.0\TestResults\Retries\qNqn3\1\TestProject1_net8.0_x64.log' to 'C:\RiderProjects\Allure.Testing.Platform\TestProject1\bin\Debug\net8.0\TestResults\TestProject1_net8.0_x64.log'
Unhandled exception. System.IO.IOException: The process cannot access the file because it is being used by another process.
   at System.IO.FileSystem.MoveFile(String sourceFullPath, String destFullPath, Boolean overwrite)
   at Microsoft.Testing.Extensions.Policy.RetryOrchestrator.OrchestrateTestHostExecutionAsync() in /_/src/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs:line 320
   at Microsoft.Testing.Platform.Hosts.TestHostOrchestratorHost.RunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Hosts/TestHostOchestratorHost.cs:line 30
   at Microsoft.Testing.Platform.Builder.TestApplication.RunAsync() in /_/src/Platform/Microsoft.Testing.Platform/Builder/TestApplication.cs:line 244
   at Xunit.Runner.InProc.SystemConsole.TestingPlatform.TestPlatformTestFramework.RunAsync(String[] args, Action`2 extensionRegistration) in /_/src/xunit.v3.runner.inproc.console/TestingPlatform/TestPlatformTestFramework.cs:line 270
   at XunitAutoGeneratedEntryPoint.Main(String[] args) in C:\.nuget\packages\xunit.v3.core\1.1.0\_content\EntryPoint-mtp.cs:line 11

=== COMMAND LINE ===
C:\Program Files\dotnet\dotnet.exe exec C:\RiderProjects\Allure.Testing.Platform\TestProject1\bin\Debug\net8.0\TestProject1.dll --internal-msbuild-node testingplatform.pipe.b37faf106d7441fd8df06ddea3148887 --retry-failed-tests 1 

[Youssef1313]

I can reproduce it.

Here is what happens:

1. When we run, RetryOrchestrator starts its job. It starts the test host to run the first attempt, and passes the results directory as "Retries/1"
2. The test host communicates back to InvokeTestingPlatformTask. The task receives ModuleInfoRequest, creates the log file, and keeps holding on it.

   testfx/src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/InvokeTestingPlatformTask.cs

   Lines 448 to 451 in 1421741

   	_outputFileStream = new StreamWriter(_fileSystem.CreateNew(_outputFileName), Encoding.Unicode)
   	{
   	AutoFlush = true,
   	};

3. If the tests complete from the first attempt, RetryOrchestrator will try to move everything from the "last attempt", which is Retries/1 to the original global results directory. But at this point, InvokeTestingPlatformTask is locking the log file.

I feel like the log file of InvokeTestingPlatformTask shouldn't be written in Retries/1 in the first place, and should instead be put to the global results directory from the beginning. That means, we actually want the ModuleInfoRequest to be sent by the orchestrator and not the test host, or alternatively, the orchestrator tells the test host about the "original" results directory.

NOTES:

1. ModuleInfoRequest is sent by MSBuildTestApplicationLifecycleCallbacks.BeforeRunAsync (implementing ITestApplicationLifecycleCallbacks). What we can do is to add an equivalent interface for orchestrators with BeforeOrchestrationAsync and AfterOrchestrationAsync methods. This will require us to add ITestHostOrchestratorManager to ITestApplicationBuilder as well as adding an equivalent of AddTestApplicationLifecycleCallbacks to ITestHostOrchestratorManager. Then MSBuild extension can send the ModuleInfoRequest in BeforeOrchestrationAsync.
2. Alternatively, Retry extension can start the child processes with env variable like RETRY_ORIGINAL_RESULTS_DIRECTORY, which will then be read by MSBuild extension, but this is generally much less extensible and tightly couples MSBuild extension and Retry extension.

[Youssef1313]

Some alternatives:

• Avoid creating the .log file by MSBuild extension early, and only create it at the very end after everything has already completed. But we will still write the log file in the wrong directory (Retries/1)
• Special case Retries/1 by MSBuild extension, and create the log file in the parent directory, with small risk of writing to the wrong directory if the original results directory is actually ending with Retries/1.

[Youssef1313]

As I was thinking more about this, I think it might be okay for InvokeTestingPlatformTask to get Retries/1 as the results directory as long as it's able to stop locking the file at the right time.

The flow today:

• InvokeTestingPlatformTask is executed by MSBuild when running dotnet test (using the VSTest mode of dotnet test (aka old dotnet test experience)
• InvokeTestingPlatformTask runs the test executable, which plays the role of "orchestrator"
• RetryOrchestrator runs
• RetryOrchestrator runs the test executable again, but now it plays the role of "test host"
• The "test host" sends ModuleInfoRequest to InvokeTestingPlatformTask (part of MSBuildTestApplicationLifecycleCallbacks.BeforeRunAsync) - at this point, InvokeTestingPlatformTask starts locking the file.
• The "test host" runs the tests. Let's assume all tests pass
• At this point, MSBuildTestApplicationLifecycleCallbacks.AfterRunAsync is run, but this does nothing today.
• The "test host" now terminates. Everything ran successfully.
• The test host orchestrator now finalizes its work. Part of what it does is copying stuff from the "last attempt results directory" to the originally specified results directory. But at this point the file is locked.

The suggested flow:

• SAME: InvokeTestingPlatformTask is executed by MSBuild when running dotnet test (using the VSTest mode of dotnet test (aka old dotnet test experience)
• SAME: InvokeTestingPlatformTask runs the test executable, which plays the role of "orchestrator"
• SAME: RetryOrchestrator runs
• SAME: RetryOrchestrator runs the test executable again, but now it plays the role of "test host"
• SAME: The "test host" sends ModuleInfoRequest to InvokeTestingPlatformTask (part of MSBuildTestApplicationLifecycleCallbacks.BeforeRunAsync) - at this point, InvokeTestingPlatformTask starts locking the file.
• SAME: The "test host" runs the tests. Let's assume all tests pass
• CHANGE HERE: At this point, MSBuildTestApplicationLifecycleCallbacks.AfterRunAsync is run, we will send a request to InvokeTestingPlatform to inform it of "end of run".
  • When InvokeTestingPlatform receives "end of run", it should complete writing the log file, and dispose the StreamWriter. So the file is now free to move.
• SAME: The "test host" now terminates. Everything ran successfully.
• The test host orchestrator now finalizes its work. Part of what it does is copying stuff from the "last attempt results directory" to the originally specified results directory. But now, the file is available to move.

NOTE: InvokeTestingPlatformTask can receive ModuleInfoRequest and EndOfRun multiple times, in case of actual retries happening. On the first ModuleInfoRequest, it will create the new file in Retries/1 directory. Then, on first EndOfRun, it will complete writing the file and let it be free. On subsequent ModuleInfoRequests, the log file should be moved from the previous directory, to the new directory pointed out by the new ModuleInfoRequest, and the file should be open in Append mode. In short:

• First test host start -> create new log file in the given results directory (Retries/1)
• First test host end -> complete writing the file and stop locking it.
• Second test host start -> move log file from the previous location to the new path, and open it in append mode.
• Second test host end -> complete writing the file and stop locking it.

[Youssef1313]

One problem with the previous approach is that it doesn't consider any logs that could be received after the last "test host end" (AfterRunAsync).