diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs index 6ae681e5d5..4236570b77 100644 --- a/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs +++ b/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs @@ -23,10 +23,27 @@ internal class TestExecutionRecorder : TestSessionMessageLogger, ITestExecutionR private readonly ITestCaseEventsHandler? _testCaseEventsHandler; /// - /// 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. /// - private readonly HashSet _testCaseInProgressMap; + private readonly Dictionary _testCaseInProgressMap; + + /// + /// Tracks test case IDs for which has been called at least once + /// while the entry is still in progress (count > 0). Used to suppress the + /// 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. + /// + /// Known limitation: suppression is ID-scoped, not per-invocation. If an adapter + /// calls for some rows sharing the same + /// but relies on the safety-net for others, those latter rows + /// will have their safety-net suppressed. In practice this is not a concern because + /// real adapters apply uniformly across all rows with the same ID. + /// + /// + private readonly HashSet _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(); + _testCaseInProgressMap = new Dictionary(); + _testCaseEndCalledSet = new HashSet(); } /// @@ -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); - } - /// - /// Send TestCaseEnd event for given testCase if not sent already - /// - /// - /// - 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; + } } } } diff --git a/test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectionTests.cs b/test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectionTests.cs index 7beb391ba9..1b4fb75c0c 100644 --- a/test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DataCollectionTests.cs +++ b/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 + { + ["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"); + } } diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs index cce3ab3ad1..b984f7f81b 100644 --- a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs +++ b/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 } diff --git a/test/TestAssets/DataDrivenTestProject/DataDrivenTestProject.csproj b/test/TestAssets/DataDrivenTestProject/DataDrivenTestProject.csproj new file mode 100644 index 0000000000..d44f992bf8 --- /dev/null +++ b/test/TestAssets/DataDrivenTestProject/DataDrivenTestProject.csproj @@ -0,0 +1,21 @@ + + + + + DataDrivenTestProject + $(TestProjectTargetFrameworks) + + + + + $(MSTestTestFrameworkVersion) + + + $(MSTestTestAdapterVersion) + + + $(PackageVersion) + + + + diff --git a/test/TestAssets/DataDrivenTestProject/DataDrivenTests.cs b/test/TestAssets/DataDrivenTestProject/DataDrivenTests.cs new file mode 100644 index 0000000000..46bd26f416 --- /dev/null +++ b/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() + { + } +} diff --git a/test/TestAssets/TestAssets.slnx b/test/TestAssets/TestAssets.slnx index 83551a9a06..0d5f0ccd3e 100644 --- a/test/TestAssets/TestAssets.slnx +++ b/test/TestAssets/TestAssets.slnx @@ -67,6 +67,7 @@ +