Fix for video datacollector throwing exception (#1719)

* Fixed the issue with video data collector throwing exception for ordered tests. Do not send TestCaseEnd for testcases whose TestCaseStart has not been recorded via adapter. * Allow only one TestCaseStart per TestCaseEnd.
microsoft · Aug 7, 2018 · 263385e · 263385e
1 parent df96f83
commit 263385e
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 19 deletions.
diff --git a/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs b/src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/TestExecutionRecorder.cs
@@ -22,9 +22,13 @@ internal class TestExecutionRecorder : TestSessionMessageLogger, ITestExecutionR
         private ITestRunCache testRunCache;
         private ITestCaseEventsHandler testCaseEventsHandler;
 
-        private HashSet<Guid> testCaseEndStatusMap;
-
-        private object testCaseEndStatusSyncObject = new object();
+        /// <summary>
+        /// Contains TestCase Ids for test cases that are in progress
+        /// Start has been recorded but End has not yet been recorded.
+        /// </summary>
+        private HashSet<Guid> testCaseInProgressMap;
+
+        private object testCaseInProgressSyncObject = new object();
 
         /// <summary>
         /// Initializes a new instance of the <see cref="TestExecutionRecorder"/> class.
@@ -43,7 +47,7 @@ public TestExecutionRecorder(ITestCaseEventsHandler testCaseEventsHandler, ITest
             // 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.
-            this.testCaseEndStatusMap = new HashSet<Guid>();
+            this.testCaseInProgressMap = new HashSet<Guid>();
         }
 
         /// <summary>
@@ -69,12 +73,15 @@ public void RecordStart(TestCase testCase)
 
             if (this.testCaseEventsHandler != null)
             {
-                lock (this.testCaseEndStatusSyncObject)
+                lock (this.testCaseInProgressSyncObject)
                 {
-                    this.testCaseEndStatusMap.Remove(testCase.Id);
+                    // Do not send TestCaseStart for a test in progress
+                    if (!this.testCaseInProgressMap.Contains(testCase.Id))
+                    {
+                        this.testCaseInProgressMap.Add(testCase.Id);
+                        this.testCaseEventsHandler.SendTestCaseStart(testCase);
+                    }
                 }
-
-                this.testCaseEventsHandler.SendTestCaseStart(testCase);
             }
         }
 
@@ -94,7 +101,7 @@ public void RecordResult(TestResult testResult)
                 this.testCaseEventsHandler.SendTestResult(testResult);
             }
 
-            // Test Result should always be flushed, even if datacollecter attachement is missing
+            // Test Result should always be flushed, even if datacollecter attachment is missing
             this.testRunCache.OnNewTestResult(testResult);
         }
 
@@ -120,16 +127,16 @@ private void SendTestCaseEnd(TestCase testCase, TestOutcome outcome)
         {
             if (this.testCaseEventsHandler != null)
             {
-                lock (this.testCaseEndStatusSyncObject)
+                lock (this.testCaseInProgressSyncObject)
                 {
-                    // Do not support multiple - TestCaseEnds for a single TestCaseStart
                     // TestCaseEnd must always be preceded by TestCaseStart for a given test case id
-                    if (!this.testCaseEndStatusMap.Contains(testCase.Id))
+                    if (this.testCaseInProgressMap.Contains(testCase.Id))
                     {
-                        this.testCaseEndStatusMap.Add(testCase.Id);
-
                         // Send test case end event to handler.
                         this.testCaseEventsHandler.SendTestCaseEnd(testCase, outcome);
+
+                        // Remove it from map so that we send only one TestCaseEnd for every TestCaseStart.
+                        this.testCaseInProgressMap.Remove(testCase.Id);
                     }
                 }
             }

diff --git a/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs b/test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Adapter/TestExecutionRecorderTests.cs
@@ -120,32 +120,54 @@ public void RecordStartShouldInvokeSendTestCaseStart()
         }
 
         [TestMethod]
-        public void SendTestCaseEndShouldInovkeTestCaseEndEventIfTestCaseStartWasCalledBefore()
+        public void RecordEndShouldInovkeTestCaseEndEventOnlyIfTestCaseStartWasCalledBefore()
         {
+            this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
             this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
 
             this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Once);
         }
 
         [TestMethod]
-        public void SendTestCaseEndShouldNotInvokeTestCaseEndEventInCaseOfAMissingTestCaseStartInDataDrivenScenario()
+        public void RecordEndShouldNotInovkeTestCaseEndEventIfTestCaseStartWasNotCalledBefore()
+        {
+            this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
+
+            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Never);
+        }
+
+        [TestMethod]
+        public void RecordEndShouldNotInvokeTestCaseEndEventInCaseOfAMissingTestCaseStartInDataDrivenScenario()
         {
             this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
             this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
             this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Failed);
 
             this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Once);
+            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Failed), Times.Never);
         }
-
+        
         [TestMethod]
         public void RecordEndShouldInvokeSendTestCaseEndMultipleTimesInDataDrivenScenario()
         {
             this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
             this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
             this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
-            this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Failed);
+            this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
 
-            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Once);
+            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Exactly(2));
+        }
+
+        [TestMethod]
+        public void RecordStartAndRecordEndShouldIgnoreRedundantTestCaseStartAndTestCaseEnd()
+        {
+            this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
+            this.testRecorderWithTestEventsHandler.RecordStart(this.testCase);
+            this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
+            this.testRecorderWithTestEventsHandler.RecordEnd(this.testCase, TestOutcome.Passed);
+
+            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseStart(this.testCase), Times.Exactly(1));
+            this.mockTestCaseEventsHandler.Verify(x => x.SendTestCaseEnd(this.testCase, TestOutcome.Passed), Times.Exactly(1));
         }
 
         [TestMethod]