[fix] Fix TestCaseStart/Stop events dropped for data-driven tests with shared TestCase.Id

src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs

@@ -23,10 +23,27 @@ internal class TestExecutionRecorder : TestSessionMessageLogger, ITestExecutionR
     private readonly ITestCaseEventsHandler? _testCaseEventsHandler;
 
     /// <summary>
-    /// Contains TestCase Ids for test cases that are in progress
-    /// Start has been recorded but End has not yet been recorded.
+    /// Tracks the number of in-progress starts per test case ID.
+    /// Multiple data-driven test executions sharing the same ID are each counted.
     /// </summary>
-    private readonly HashSet<Guid> _testCaseInProgressMap;
+    private readonly Dictionary<Guid, int> _testCaseInProgressMap;
+
+    /// <summary>
+    /// Tracks test case IDs for which <see cref="RecordEnd"/> has been called at least once
+    /// while the entry is still in progress (count &gt; 0). Used to suppress the
+    /// <see cref="RecordResult"/> safety-net in nested data-driven scenarios: once an
+    /// explicit RecordEnd fires for an ID, subsequent RecordResult calls must not send a
+    /// spurious extra TestCaseEnd that would consume the parent's pending count slot.
+    /// The ID is removed when the last in-progress count reaches zero.
+    /// <para>
+    /// <b>Known limitation:</b> suppression is ID-scoped, not per-invocation. If an adapter
+    /// calls <see cref="RecordEnd"/> for some rows sharing the same <see cref="TestCase.Id"/>
+    /// but relies on the <see cref="RecordResult"/> safety-net for others, those latter rows
+    /// will have their safety-net suppressed. In practice this is not a concern because
+    /// real adapters apply <see cref="RecordEnd"/> uniformly across all rows with the same ID.
+    /// </para>
+    /// </summary>
+    private readonly HashSet<Guid> _testCaseEndCalledSet;
 
     private readonly object _testCaseInProgressSyncObject = new();
 
@@ -47,7 +64,8 @@ public TestExecutionRecorder(ITestCaseEventsHandler? testCaseEventsHandler, ITes
         // 3. Test Case Result.
         // If that is not that case.
         // If Test Adapters don't send the events in the above order, Test Case Results are cached till the Test Case End event is received.
-        _testCaseInProgressMap = new HashSet<Guid>();
+        _testCaseInProgressMap = new Dictionary<Guid, int>();
+        _testCaseEndCalledSet = new HashSet<Guid>();
     }
 
     /// <summary>
@@ -75,12 +93,12 @@ public void RecordStart(TestCase testCase)
         {
             lock (_testCaseInProgressSyncObject)
             {
-                // Do not send TestCaseStart for a test in progress
-                if (!_testCaseInProgressMap.Contains(testCase.Id))
-                {
-                    _testCaseInProgressMap.Add(testCase.Id);
-                    _testCaseEventsHandler.SendTestCaseStart(testCase);
-                }
+                // Track the number of in-progress starts for this test case ID.
+                // Data-driven tests may call RecordStart multiple times with the same ID
+                // (when rows share the same fully-qualified name), so we must forward
+                // every start rather than deduplicating by ID.
+                _testCaseInProgressMap[testCase.Id] = _testCaseInProgressMap.TryGetValue(testCase.Id, out int count) ? count + 1 : 1;
+                _testCaseEventsHandler.SendTestCaseStart(testCase);
             }
         }
     }
@@ -96,8 +114,29 @@ public void RecordResult(TestResult testResult)
         EqtTrace.Verbose("TestExecutionRecorder.RecordResult: Received result for test: {0}.", testResult.TestCase.FullyQualifiedName);
         if (_testCaseEventsHandler != null)
         {
-            // Send TestCaseEnd in case RecordEnd was not called.
-            SendTestCaseEnd(testResult.TestCase, testResult.Outcome);
+            lock (_testCaseInProgressSyncObject)
+            {
+                // Safety net: send TestCaseEnd in case RecordEnd was not called.
+                // Guard: skip if RecordEnd was already called for this ID (indicated by presence in
+                // _testCaseEndCalledSet) to avoid consuming a count slot belonging to a parent or
+                // sibling in a nested data-driven scenario where all rows share the same TestCase.Id.
+                if (_testCaseInProgressMap.TryGetValue(testResult.TestCase.Id, out int count)
+                    && count > 0
+                    && !_testCaseEndCalledSet.Contains(testResult.TestCase.Id))
+                {
+                    _testCaseEventsHandler.SendTestCaseEnd(testResult.TestCase, testResult.Outcome);
+
+                    if (count == 1)
+                    {
+                        _testCaseInProgressMap.Remove(testResult.TestCase.Id);
+                    }
+                    else
+                    {
+                        _testCaseInProgressMap[testResult.TestCase.Id] = count - 1;
+                    }
+                }
+            }
+
             _testCaseEventsHandler.SendTestResult(testResult);
         }
 
@@ -115,28 +154,32 @@ public void RecordEnd(TestCase testCase, TestOutcome outcome)
     {
         EqtTrace.Verbose("TestExecutionRecorder.RecordEnd: test: {0} execution completed.", testCase.FullyQualifiedName);
         _testRunCache.OnTestCompletion(testCase);
-        SendTestCaseEnd(testCase, outcome);
-    }
 
-    /// <summary>
-    /// Send TestCaseEnd event for given testCase if not sent already
-    /// </summary>
-    /// <param name="testCase"></param>
-    /// <param name="outcome"></param>
-    private void SendTestCaseEnd(TestCase testCase, TestOutcome outcome)
-    {
         if (_testCaseEventsHandler != null)
         {
             lock (_testCaseInProgressSyncObject)
             {
-                // TestCaseEnd must always be preceded by TestCaseStart for a given test case id
-                if (_testCaseInProgressMap.Contains(testCase.Id))
+                // TestCaseEnd must always be preceded by TestCaseStart for a given test case id.
+                // Use the reference count to ensure we send exactly one End for each Start.
+                if (_testCaseInProgressMap.TryGetValue(testCase.Id, out int count) && count > 0)
                 {
-                    // Send test case end event to handler.
+                    // Mark that RecordEnd was called for this ID while it is still in progress.
+                    // This suppresses the RecordResult safety-net for any subsequent Result calls
+                    // that share the same ID (e.g., row results in a nested data-driven test).
+                    _testCaseEndCalledSet.Add(testCase.Id);
+
                     _testCaseEventsHandler.SendTestCaseEnd(testCase, outcome);
 
-                    // Remove it from map so that we send only one TestCaseEnd for every TestCaseStart.
-                    _testCaseInProgressMap.Remove(testCase.Id);
+                    // Decrement the count; remove both tracking entries when there are no more in-progress starts.
+                    if (count == 1)
+                    {
+                        _testCaseInProgressMap.Remove(testCase.Id);
+                        _testCaseEndCalledSet.Remove(testCase.Id);
+                    }
+                    else
+                    {
+                        _testCaseInProgressMap[testCase.Id] = count - 1;
+                    }
                 }
             }
         }

test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectionTests.cs

@@ -241,4 +241,42 @@ private static string GetRunsettingsFilePath(string resultsDir)
         CreateDataCollectionRunSettingsFile(runsettingsPath, dataCollectionAttributes);
         return runsettingsPath;
     }
+
+    [TestMethod]
+    [NetFullTargetFrameworkDataSource]
+    [NetCoreTargetFrameworkDataSource]
+    public void DataCollectorReceivesTestCaseStartForEveryDataDrivenRow(RunnerInfo runnerInfo)
+    {
+        // Regression test for https://github.com/microsoft/vstest/issues/4997
+        // When data-driven tests share the same TestCase.Id, TestCaseStart events
+        // must still fire for every row execution so data collectors can track each one.
+        SetTestEnvironment(_testEnvironment, runnerInfo);
+
+        var assemblyPaths = GetAssetFullPath("DataDrivenTestProject.dll");
+        string runSettings = GetRunsettingsFilePath(TempDirectory.Path);
+        string diagFileName = Path.Combine(TempDirectory.Path, "diaglog.txt");
+        var extensionsPath = Path.GetDirectoryName(GetTestDllForFramework("OutOfProcDataCollector.dll", "netstandard2.0"));
+        var arguments = PrepareArguments(assemblyPaths, null, runSettings, FrameworkArgValue, runnerInfo.InIsolationValue, resultsDirectory: TempDirectory.Path);
+        arguments = string.Concat(arguments, $" /Diag:{diagFileName}", $" /TestAdapterPath:{extensionsPath}");
+
+        var env = new Dictionary<string, string?>
+        {
+            ["TEST_ASSET_SAMPLE_COLLECTOR_PATH"] = TempDirectory.Path,
+        };
+
+        InvokeVsTest(arguments, env);
+
+        // DataDrivenTestProject has 4 test executions: 3 DataRow rows + 1 simple test.
+        ValidateSummaryStatus(4, 0, 0);
+
+        // The SampleDataCollector creates one testcasefilename{i}.txt per TestCaseStart event.
+        // Before the fix, DataRow rows sharing the same TestCase.Id would get deduplicated,
+        // producing fewer files than actual test executions.
+        var resultFiles = Directory.GetFiles(TempDirectory.Path, "testcasefilename*", SearchOption.AllDirectories);
+        Assert.HasCount(4, resultFiles);
+
+        // Verify the collector logged start/end for each execution.
+        StdOutputContains("TestCaseStarted");
+        StdOutputContains("TestCaseEnded");
+    }
 }

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs

@@ -156,15 +156,32 @@ public void RecordEndShouldInvokeSendTestCaseEndMultipleTimesInDataDrivenScenari
     }
 
     [TestMethod]
-    public void RecordStartAndRecordEndShouldIgnoreRedundantTestCaseStartAndTestCaseEnd()
+    public void RecordStartAndRecordEndShouldForwardAllTestCaseStartAndEndEvents()
     {
         _testRecorderWithTestEventsHandler.RecordStart(_testCase);
         _testRecorderWithTestEventsHandler.RecordStart(_testCase);
         _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);
         _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);
 
-        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseStart(_testCase), Times.Exactly(1));
-        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(_testCase, TestOutcome.Passed), Times.Exactly(1));
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseStart(_testCase), Times.Exactly(2));
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(_testCase, TestOutcome.Passed), Times.Exactly(2));
+    }
+
+    [TestMethod]
+    public void RecordStartAndRecordEndShouldSendEventsForNestedDataDrivenTestsWithSameId()
+    {
+        // Simulate a data-driven scenario where the parent test and its row executions
+        // share the same TestCase.Id (same fully-qualified name), with nested Start calls:
+        // Start(parent) → Start(row1) → End(row1) → Start(row2) → End(row2) → End(parent)
+        _testRecorderWithTestEventsHandler.RecordStart(_testCase);   // parent start
+        _testRecorderWithTestEventsHandler.RecordStart(_testCase);   // row1 start (same ID)
+        _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);   // row1 end
+        _testRecorderWithTestEventsHandler.RecordStart(_testCase);   // row2 start (same ID)
+        _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);   // row2 end
+        _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);   // parent end
+
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseStart(_testCase), Times.Exactly(3));
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(_testCase, TestOutcome.Passed), Times.Exactly(3));
     }
 
     [TestMethod]
@@ -219,5 +236,30 @@ public void RecordResultShouldSendTestCaseEndEventIfRecordEndWasNotCalled()
         Assert.Contains(_testResult, _testableTestRunCache.TestResultList);
     }
 
+    [TestMethod]
+    public void RecordResultShouldNotSendSpuriousTestCaseEndForParentInNestedDataDrivenScenario()
+    {
+        // Regression test for the nested data-driven scenario where the parent test and its row
+        // executions share the same TestCase.Id (same fully-qualified name).
+        //
+        // Pattern: Start(parent) → Start(row1) → End(row1) → Result(row1) → End(parent)
+        //
+        // Before the fix, RecordResult's safety-net would fire after End(row1) decremented
+        // the count to 1, consuming the count slot that belonged to the parent and causing
+        // the subsequent End(parent) to be silently dropped.
+        _testResult.Outcome = TestOutcome.Passed;
+
+        _testRecorderWithTestEventsHandler.RecordStart(_testCase);                       // parent start → count=1
+        _testRecorderWithTestEventsHandler.RecordStart(_testCase);                       // row1 start  → count=2
+        _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);     // row1 end    → count=1, sends End
+        _testRecorderWithTestEventsHandler.RecordResult(_testResult);                    // row1 result → safety-net must NOT fire
+        _testRecorderWithTestEventsHandler.RecordEnd(_testCase, TestOutcome.Passed);     // parent end  → count=0, sends End
+
+        // Exactly two Ends: one for row1 (from RecordEnd) and one for the parent (from RecordEnd).
+        // A third End from the RecordResult safety-net would indicate the regression is present.
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(_testCase, TestOutcome.Passed), Times.Exactly(2));
+        _mockTestCaseEventsHandler.Verify(x => x.SendTestResult(_testResult), Times.Once);
+    }
+
     #endregion
 }

test/TestAssets/DataDrivenTestProject/DataDrivenTestProject.csproj

@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <AssemblyName>DataDrivenTestProject</AssemblyName>
+    <TargetFrameworks>$(TestProjectTargetFrameworks)</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="MSTest.TestFramework">
+      <Version>$(MSTestTestFrameworkVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="MSTest.TestAdapter">
+      <Version>$(MSTestTestAdapterVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="Microsoft.NET.Test.Sdk">
+      <Version>$(PackageVersion)</Version>
+    </PackageReference>
+  </ItemGroup>
+
+</Project>

test/TestAssets/DataDrivenTestProject/DataDrivenTests.cs

@@ -0,0 +1,25 @@
+// 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.VisualStudio.TestTools.UnitTesting;
+
+namespace DataDrivenTestProject;
+
+[TestClass]
+public class DataDrivenTests
+{
+    [TestMethod]
+    [DataRow(1, "first")]
+    [DataRow(2, "second")]
+    [DataRow(3, "third")]
+    public void ParameterizedTest(int value, string name)
+    {
+        Assert.IsTrue(value > 0);
+        Assert.IsNotNull(name);
+    }
+
+    [TestMethod]
+    public void SimpleTest()
+    {
+    }
+}

test/TestAssets/TestAssets.slnx

@@ -67,6 +67,7 @@
   <Project Path="SimpleDataCollector/SimpleDataCollector.csproj" />
   <Project Path="SimpleTestAdapter/SimpleTestAdapter.csproj" />
   <Project Path="SimpleTestProject/SimpleTestProject.csproj" />
+  <Project Path="DataDrivenTestProject/DataDrivenTestProject.csproj" />
   <Project Path="SimpleTestProject2/SimpleTestProject2.csproj" />
   <Project Path="SimpleTestProject3/SimpleTestProject3.csproj" />
   <Project Path="SimpleTestProject4/SimpleTestProject4.csproj" />