Cleanup TestProgressState (#50056)

Youssef1313 · Copilot · nohwnd · web-flow · commit f3569b184b6c · 2025-08-11T09:07:26.000+02:00
Co-authored-by: Copilot &lt;175728472+Copilot@users.noreply.github.com&gt;
Co-authored-by: Jakub Jareš &lt;me@jakubjares.com&gt;
diff --git a/src/Cli/dotnet/Commands/Test/Terminal/TerminalTestReporter.cs b/src/Cli/dotnet/Commands/Test/Terminal/TerminalTestReporter.cs
@@ -172,10 +172,10 @@ public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount,
         _terminalWithProgress.StartShowingProgress(workerCount);
     }
 
-    public void AssemblyRunStarted(string assembly, string? targetFramework, string? architecture, string executionId)
+    public void AssemblyRunStarted(string assembly, string? targetFramework, string? architecture, string executionId, string instanceId)
     {
         var assemblyRun = GetOrAddAssemblyRun(assembly, targetFramework, architecture, executionId);
-        assemblyRun.TryCount++;
+        assemblyRun.NotifyHandshake(instanceId);
 
         // If we fail to parse out the parameter correctly this will enable retry on re-run of the assembly within the same execution.
         // Not good enough for general use, because we want to show (try 1) even on the first try, but this will at
@@ -494,14 +494,6 @@ internal void TestCompleted(
                 break;
         }
 
-        if (_isRetry && asm.TryCount > 1 && outcome == TestOutcome.Passed)
-        {
-            // This is a retry of a test, and the test succeeded, so these tests are potentially flaky.
-            // Tests that come from dynamic data sources and previously succeeded will also run on the second attempt,
-            // and most likely will succeed as well, so we will get them here, even though they are probably not flaky.
-            asm.FlakyTests.Add(testNodeUid);
-
-        }
         _terminalWithProgress.UpdateWorker(asm.SlotIndex);
         if (outcome != TestOutcome.Passed || GetShowPassedTests())
         {
@@ -806,7 +798,6 @@ internal void AssemblyRunCompleted(string executionId,
         int exitCode, string? outputData, string? errorData)
     {
         TestProgressState assemblyRun = _assemblies[executionId];
-        assemblyRun.ExitCode = exitCode;
         assemblyRun.Success = exitCode == 0 && assemblyRun.FailedTests == 0;
         assemblyRun.Stopwatch.Stop();
 
@@ -959,6 +950,14 @@ internal void TestDiscovered(
         string? displayName,
         string? uid)
     {
+        if (!_isDiscovery)
+        {
+            // Don't count discovered tests if we are not in discovery mode.
+            // NOTE: DiscoverTest call increments PassedTests, so it must not be called when we get discovery message in run mode.
+            // Also, don't bother adding discovered tests to the DiscoveredTests list when the list is only used in discovery mode.
+            return;
+        }
+
         TestProgressState asm = _assemblies[executionId];
 
         // TODO: add mode for discovered tests to the progress bar - jajares
diff --git a/src/Cli/dotnet/Commands/Test/Terminal/TestProgressState.cs b/src/Cli/dotnet/Commands/Test/Terminal/TestProgressState.cs
@@ -1,11 +1,18 @@
 ﻿// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.Diagnostics;
+using TestNodeInfoEntry = (int Passed, int Skipped, int Failed, int LastAttemptNumber);
+
 namespace Microsoft.DotNet.Cli.Commands.Test.Terminal;
 
 internal sealed class TestProgressState(long id, string assembly, string? targetFramework, string? architecture, IStopwatch stopwatch)
 {
-    private readonly Dictionary<string, (int Passed, int Skipped, int Failed, string LastInstanceId)> _testUidToResults = new();
+    private readonly Dictionary<string, TestNodeInfoEntry> _testUidToResults = new();
+
+    // In most cases, retries don't happen. So we start with a capacity of 1.
+    // Resizes will be rare and will be okay with such small sizes.
+    private readonly List<string> _orderedInstanceIds = new(capacity: 1);
 
     public string Assembly { get; } = assembly;
 
@@ -37,98 +44,114 @@ internal sealed class TestProgressState(long id, string assembly, string? target
 
     public List<(string? DisplayName, string? UID)> DiscoveredTests { get; internal set; } = [];
 
-    public int? ExitCode { get; internal set; }
-
     public bool Success { get; internal set; }
 
-    public int TryCount { get; internal set; }
-
-    public HashSet<string> FlakyTests { get; } = [];
+    public int TryCount { get; private set; }
 
-    public void ReportPassingTest(string testNodeUid, string instanceId)
+    private void ReportGenericTestResult(
+        string testNodeUid,
+        string instanceId,
+        Func<TestNodeInfoEntry, TestNodeInfoEntry> incrementTestNodeInfoEntry,
+        Action<TestProgressState> incrementCountAction)
     {
+        var currentAttemptNumber = GetAttemptNumberFromInstanceId(instanceId);
+
         if (_testUidToResults.TryGetValue(testNodeUid, out var value))
         {
-            if (value.LastInstanceId == instanceId)
+            // We received a result for this test node uid before.
+            if (value.LastAttemptNumber == currentAttemptNumber)
             {
-                // We are getting a test result for the same instance id.
-                value.Passed++;
-                _testUidToResults[testNodeUid] = value;
+                // We are getting a test result for the same attempt.
+                // This means that the test framework is reporting multiple results for the same test node uid.
+                // We will just increment the count of the result.
+                _testUidToResults[testNodeUid] = incrementTestNodeInfoEntry(value);
             }
-            else
+            else if (currentAttemptNumber > value.LastAttemptNumber)
             {
+                // This is a retry!
                 // We are getting a test result for a different instance id.
                 // This means that the test was retried.
                 // We discard the results from the previous instance id
-                RetriedFailedTests++;
+                RetriedFailedTests += value.Failed;
                 PassedTests -= value.Passed;
                 SkippedTests -= value.Skipped;
                 FailedTests -= value.Failed;
-                _testUidToResults[testNodeUid] = (Passed: 1, Skipped: 0, Failed: 0, LastInstanceId: instanceId);
+                _testUidToResults[testNodeUid] = incrementTestNodeInfoEntry((Passed: 0, Skipped: 0, Failed: 0, LastAttemptNumber: currentAttemptNumber));
+            }
+            else
+            {
+                // This is an unexpected case where we received a result for an instance id that is older than the last one we saw.
+                throw new UnreachableException($"Unexpected test result for attempt '{currentAttemptNumber}' while the last attempt is '{value.LastAttemptNumber}'");
             }
         }
         else
         {
             // This is the first time we see this test node.
-            _testUidToResults.Add(testNodeUid, (Passed: 1, Skipped: 0, Failed: 0, LastInstanceId: instanceId));
+            _testUidToResults.Add(testNodeUid, incrementTestNodeInfoEntry((Passed: 0, Skipped: 0, Failed: 0, LastAttemptNumber: currentAttemptNumber)));
         }
 
-        PassedTests++;
+        incrementCountAction(this);
+    }
+
+    public void ReportPassingTest(string testNodeUid, string instanceId)
+    {
+        ReportGenericTestResult(testNodeUid, instanceId, static entry =>
+        {
+            entry.Passed++;
+            return entry;
+        }, static @this => @this.PassedTests++);
     }
 
     public void ReportSkippedTest(string testNodeUid, string instanceId)
     {
-        if (_testUidToResults.TryGetValue(testNodeUid, out var value))
+        ReportGenericTestResult(testNodeUid, instanceId, static entry =>
         {
-            if (value.LastInstanceId == instanceId)
-            {
-                value.Skipped++;
-                _testUidToResults[testNodeUid] = value;
-            }
-            else
-            {
-                PassedTests -= value.Passed;
-                SkippedTests -= value.Skipped;
-                FailedTests -= value.Failed;
-                _testUidToResults[testNodeUid] = (Passed: 0, Skipped: 1, Failed: 0, LastInstanceId: instanceId);
-            }
-        }
-        else
+            entry.Skipped++;
+            return entry;
+        }, static @this => @this.SkippedTests++);
+    }
+
+    public void ReportFailedTest(string testNodeUid, string instanceId)
+    {
+        ReportGenericTestResult(testNodeUid, instanceId, static entry =>
         {
-            _testUidToResults.Add(testNodeUid, (Passed: 0, Skipped: 1, Failed: 0, LastInstanceId: instanceId));
-        }
+            entry.Failed++;
+            return entry;
+        }, static @this => @this.FailedTests++);
+    }
 
-        SkippedTests++;
+    public void DiscoverTest(string? displayName, string? uid)
+    {
+        PassedTests++;
+        DiscoveredTests.Add(new(displayName, uid));
     }
 
-    public void ReportFailedTest(string testNodeUid, string instanceId)
+    internal void NotifyHandshake(string instanceId)
     {
-        if (_testUidToResults.TryGetValue(testNodeUid, out var value))
+        var index = _orderedInstanceIds.IndexOf(instanceId);
+        if (index < 0)
         {
-            if (value.LastInstanceId == instanceId)
-            {
-                value.Failed++;
-                _testUidToResults[testNodeUid] = value;
-            }
-            else
-            {
-                PassedTests -= value.Passed;
-                SkippedTests -= value.Skipped;
-                FailedTests -= value.Failed;
-                _testUidToResults[testNodeUid] = (Passed: 0, Skipped: 0, Failed: 1, LastInstanceId: instanceId);
-            }
+            // New instanceId for a retry. We add it to _orderedInstanceIds.
+            _orderedInstanceIds.Add(instanceId);
+            TryCount++;
         }
-        else
+        else if (index != _orderedInstanceIds.Count - 1)
         {
-            _testUidToResults.Add(testNodeUid, (Passed: 0, Skipped: 0, Failed: 1, LastInstanceId: instanceId));
+            // This is an unexpected case where we received a handshake for an instance id that is not the last one we saw.
+            // This means that the test framework is trying to report results for an instance id that is not the last one.
+            throw new UnreachableException($"Unexpected handshake for instance id '{instanceId}' at index '{index}' while the last index is '{_orderedInstanceIds.Count - 1}'");
         }
-
-        FailedTests++;
     }
 
-    public void DiscoverTest(string? displayName, string? uid)
+    private int GetAttemptNumberFromInstanceId(string instanceId)
     {
-        PassedTests++;
-        DiscoveredTests.Add(new(displayName, uid));
+        var index = _orderedInstanceIds.IndexOf(instanceId);
+        if (index < 0)
+        {
+            throw new UnreachableException($"The instanceId '{instanceId}' not found.");
+        }
+
+        // Attempt numbers are 1-based, so we add 1 to the index.
+        return index + 1;
     }
 }
diff --git a/src/Cli/dotnet/Commands/Test/TestApplicationEventHandlers.cs b/src/Cli/dotnet/Commands/Test/TestApplicationEventHandlers.cs
@@ -26,11 +26,12 @@ public void OnHandshakeReceived(object sender, HandshakeArgs args)
             // Calling it for both test host and test host controllers means we will count retries incorrectly, and will messages twice.
             var testApplication = (TestApplication)sender;
             var executionId = args.Handshake.Properties[HandshakeMessagePropertyNames.ExecutionId];
+            var instanceId = args.Handshake.Properties[HandshakeMessagePropertyNames.InstanceId];
             var arch = args.Handshake.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
             var tfm = TargetFrameworkParser.GetShortTargetFramework(args.Handshake.Properties[HandshakeMessagePropertyNames.Framework]);
             (string ModulePath, string TargetFramework, string Architecture, string ExecutionId) appInfo = new(testApplication.Module.TargetPath, tfm, arch, executionId);
             _executions[testApplication] = appInfo;
-            _output.AssemblyRunStarted(appInfo.ModulePath, appInfo.TargetFramework, appInfo.Architecture, appInfo.ExecutionId);
+            _output.AssemblyRunStarted(appInfo.ModulePath, appInfo.TargetFramework, appInfo.Architecture, appInfo.ExecutionId, instanceId);
         }
 
         LogHandshake(args);
@@ -68,11 +69,6 @@ public void OnDiscoveredTestsReceived(object sender, DiscoveredTestEventArgs arg
 
     public void OnTestResultsReceived(object sender, TestResultEventArgs args)
     {
-        // TODO: If we got some results for ExecutionId1 and InstanceId1
-        // Then we started getting ExecutionId1 and InstanceId2,
-        // Then we started getting ExecutionId1 and InstanceId1 again.
-        // Should we discard the last result from ExecutionId1 and InstanceId1 completely?
-        // Or is it considered a violation of the protocol and should never happen? (in that case maybe we should throw?)
         var testApp = (TestApplication)sender;
         var appInfo = _executions[testApp];
 
diff --git a/test/dotnet.Tests/CommandTests/Test/TestProgressStateTests.cs b/test/dotnet.Tests/CommandTests/Test/TestProgressStateTests.cs
