From 8800f47fa22d95cbe361a886716fce25a75fd606 Mon Sep 17 00:00:00 2001 From: Shreyas R Date: Thu, 22 Nov 2018 15:01:07 +0530 Subject: [PATCH 1/5] Fixed issue where proc dump was not getting terminated on no crash --- .gitignore | 5 ++++ .../BlameCollector.cs | 2 ++ .../Interfaces/IProcessDumpUtility.cs | 7 ++++-- .../ProcessDumpUtility.cs | 18 ++++++++++++-- .../BlameCollectorTests.cs | 24 +++++++++++++++++++ .../ProcessDumpUtilityTests.cs | 22 +++++++++++++++++ 6 files changed, 74 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index fd0b49f4b4..4a52aa889f 100644 --- a/.gitignore +++ b/.gitignore @@ -124,3 +124,8 @@ src/package/sign/sign.nuget.targets *.tr.resx *.zh-Hans.resx *.zh-Hant.resx + +# =========================== +# Vscode files +# =========================== +.vscode/ diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs index b880a66fab..84ab4cf0ea 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs @@ -240,6 +240,8 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args) } } + // Terminate the proc dump process + this.processDumpUtility.TerminateProcessDump(); this.DeregisterEvents(); } diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs index 5aff12e962..5db532e99d 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs @@ -3,8 +3,6 @@ namespace Microsoft.TestPlatform.Extensions.BlameDataCollector { - using System.Collections.Generic; - public interface IProcessDumpUtility { /// @@ -31,5 +29,10 @@ public interface IProcessDumpUtility /// Is full dump enabled /// void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory, bool isFullDump = false); + + /// + /// Terminate the proc dump process + /// + void TerminateProcessDump(); } } diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs index 8fd3d88356..b0fff4c294 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs @@ -44,8 +44,8 @@ public ProcessDumpUtility(IProcessHelper processHelper, IFileHelper fileHelper, }; } - /// - public string GetDumpFile() + /// + public string GetDumpFile() { if (this.procDumpProcess == null) { @@ -95,6 +95,20 @@ public void StartProcessDump(int processId, string dumpFileGuid, string testResu null) as Process; } + /// + public void TerminateProcessDump() + { + try + { + EqtTrace.Info("ProcessDumpUtility : Attempting to kill proc dump process."); + this.processHelper.TerminateProcess(this.procDumpProcess); + } + catch (Exception e) + { + EqtTrace.Info($"ProcessDumpUtility : Failed to kill proc dump process with exception {e}"); + } + } + /// /// Arguments for procdump.exe /// diff --git a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs index 3f1da072fd..f279c50a89 100644 --- a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs +++ b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs @@ -199,6 +199,30 @@ public void TriggerSessionEndedHandlerShouldGetDumpFileIfProcDumpEnabled() this.mockProcessDumpUtility.Verify(x => x.GetDumpFile(), Times.Once); } + /// + /// The trigger session ended handler should ensure proc dump process is terminated when no crash is detected + /// + [TestMethod] + public void TriggerSessionEndedHandlerShouldEnsureProcDumpProcessIsTerminated() + { + // Initializing Blame Data Collector + this.blameDataCollector.Initialize( + this.GetDumpConfigurationElement(), + this.mockDataColectionEvents.Object, + this.mockDataCollectionSink.Object, + this.mockLogger.Object, + this.context); + + // Mock proc dump utility terminate process call + this.mockProcessDumpUtility.Setup(x => x.TerminateProcessDump()); + + // Raise + this.mockDataColectionEvents.Raise(x => x.SessionEnd += null, new SessionEndEventArgs(this.dataCollectionContext)); + + // Verify GetDumpFiles Call + this.mockProcessDumpUtility.Verify(x => x.TerminateProcessDump(), Times.Once); + } + /// /// The trigger session ended handler should not dump files if proc dump was enabled and test host did not crash /// diff --git a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs index 17a8277a62..8377b8f375 100644 --- a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs +++ b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs @@ -292,6 +292,28 @@ public void StartProcessDumpWillStartExeCorrespondingTo32BitTestHostProcessIn64B this.mockProcessHelper.Verify(x => x.LaunchProcess(Path.Combine("D:\\procdump", "procdump.exe"), It.IsAny(), It.IsAny(), null, null, null)); } + /// + /// Ensure terminate process calls terminate on proc dump process + /// + [TestMethod] + public void TerminateProcessDumpShouldCallTerminateOnProcDumpProcess() + { + var processDumpUtility = new ProcessDumpUtility( + this.mockProcessHelper.Object, + this.mockFileHelper.Object, + this.mockPlatformEnvironment.Object, + this.mockNativeMethodsHelper.Object); + + // Mock process helper + this.mockProcessHelper.Setup(x => x.TerminateProcess(It.IsAny())); + + // Raise + processDumpUtility.TerminateProcessDump(); + + // Verify + this.mockProcessHelper.Verify(x => x.TerminateProcess(It.IsAny()), Times.Once); + } + [TestCleanup] public void TestCleanup() { From aa8a36935362ef732dfd091bc7b683a9561c02e8 Mon Sep 17 00:00:00 2001 From: Shreyas R Date: Thu, 22 Nov 2018 15:42:42 +0530 Subject: [PATCH 2/5] resolved pr comments --- .../BlameCollector.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs index 84ab4cf0ea..c6fe208b39 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs @@ -240,8 +240,12 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args) } } - // Terminate the proc dump process - this.processDumpUtility.TerminateProcessDump(); + // Attempt to terminate the proc dump process if proc dump was enabled + if (this.processDumpEnabled) + { + this.processDumpUtility.TerminateProcessDump(); + } + this.DeregisterEvents(); } From 622eb5ff85e0198260bd496b3e65699092b2f62f Mon Sep 17 00:00:00 2001 From: Shreyas R Date: Thu, 22 Nov 2018 15:46:32 +0530 Subject: [PATCH 3/5] resolved pr comments --- .../BlameCollector.cs | 69 ++++++++++--------- 1 file changed, 37 insertions(+), 32 deletions(-) diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs index c6fe208b39..355cb175a5 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs @@ -202,51 +202,56 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args) EqtTrace.Info("Blame Collector : Session End"); } - // If the last test crashes, it will not invoke a test case end and therefore - // In case of crash testStartCount will be greater than testEndCount and we need to write the sequence - // And send the attachment - if (this.testStartCount > this.testEndCount) + try { - var filepath = Path.Combine(this.GetResultsDirectory(), Constants.AttachmentFileName + "_" + this.attachmentGuid); - filepath = this.blameReaderWriter.WriteTestSequence(this.testSequence, this.testObjectDictionary, filepath); - var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, filepath, true); - this.dataCollectionSink.SendFileAsync(fileTranferInformation); - } + // If the last test crashes, it will not invoke a test case end and therefore + // In case of crash testStartCount will be greater than testEndCount and we need to write the sequence + // And send the attachment + if (this.testStartCount > this.testEndCount) + { + var filepath = Path.Combine(this.GetResultsDirectory(), Constants.AttachmentFileName + "_" + this.attachmentGuid); + filepath = this.blameReaderWriter.WriteTestSequence(this.testSequence, this.testObjectDictionary, filepath); + var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, filepath, true); + this.dataCollectionSink.SendFileAsync(fileTranferInformation); + } - if (this.processDumpEnabled) - { - // If there was a test case crash or if we need to collect dump on process exit. - if (this.testStartCount > this.testEndCount || this.collectDumpAlways) + if (this.processDumpEnabled) { - try + // If there was a test case crash or if we need to collect dump on process exit. + if (this.testStartCount > this.testEndCount || this.collectDumpAlways) { - var dumpFile = this.processDumpUtility.GetDumpFile(); - if (!string.IsNullOrEmpty(dumpFile)) + try { - var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true); - this.dataCollectionSink.SendFileAsync(fileTranferInformation); + var dumpFile = this.processDumpUtility.GetDumpFile(); + if (!string.IsNullOrEmpty(dumpFile)) + { + var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true); + this.dataCollectionSink.SendFileAsync(fileTranferInformation); + } + else + { + EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated."); + this.logger.LogWarning(args.Context, Resources.Resources.ProcDumpNotGenerated); + } } - else + catch (FileNotFoundException ex) { - EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated."); - this.logger.LogWarning(args.Context, Resources.Resources.ProcDumpNotGenerated); + EqtTrace.Warning(ex.Message); + this.logger.LogWarning(args.Context, ex.Message); } } - catch (FileNotFoundException ex) - { - EqtTrace.Warning(ex.Message); - this.logger.LogWarning(args.Context, ex.Message); - } } } - - // Attempt to terminate the proc dump process if proc dump was enabled - if (this.processDumpEnabled) + finally { - this.processDumpUtility.TerminateProcessDump(); - } + // Attempt to terminate the proc dump process if proc dump was enabled + if (this.processDumpEnabled) + { + this.processDumpUtility.TerminateProcessDump(); + } - this.DeregisterEvents(); + this.DeregisterEvents(); + } } /// From c4add364cf0d19a10b4f4ef5f4a6cd7aafd2f942 Mon Sep 17 00:00:00 2001 From: Shreyas R Date: Thu, 22 Nov 2018 15:51:50 +0530 Subject: [PATCH 4/5] resolved pr comments --- .../ProcessDumpUtility.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs index b0fff4c294..78b13e941a 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs @@ -105,7 +105,7 @@ public void TerminateProcessDump() } catch (Exception e) { - EqtTrace.Info($"ProcessDumpUtility : Failed to kill proc dump process with exception {e}"); + EqtTrace.Warning($"ProcessDumpUtility : Failed to kill proc dump process with exception {e}"); } } From db054cee16566cfabe49cc94421754a8386d7cfa Mon Sep 17 00:00:00 2001 From: Shreyas R Date: Thu, 22 Nov 2018 15:54:05 +0530 Subject: [PATCH 5/5] incorporated sarab's rename comment --- .../BlameCollector.cs | 2 +- .../Interfaces/IProcessDumpUtility.cs | 2 +- .../ProcessDumpUtility.cs | 2 +- .../BlameCollectorTests.cs | 4 ++-- .../ProcessDumpUtilityTests.cs | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs index 355cb175a5..7674fe69af 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs @@ -247,7 +247,7 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args) // Attempt to terminate the proc dump process if proc dump was enabled if (this.processDumpEnabled) { - this.processDumpUtility.TerminateProcessDump(); + this.processDumpUtility.TerminateProcess(); } this.DeregisterEvents(); diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs index 5db532e99d..e8b78e2000 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs @@ -33,6 +33,6 @@ public interface IProcessDumpUtility /// /// Terminate the proc dump process /// - void TerminateProcessDump(); + void TerminateProcess(); } } diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs index 78b13e941a..9cd039df81 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs @@ -96,7 +96,7 @@ public void StartProcessDump(int processId, string dumpFileGuid, string testResu } /// - public void TerminateProcessDump() + public void TerminateProcess() { try { diff --git a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs index f279c50a89..c7bbb7efd0 100644 --- a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs +++ b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs @@ -214,13 +214,13 @@ public void TriggerSessionEndedHandlerShouldEnsureProcDumpProcessIsTerminated() this.context); // Mock proc dump utility terminate process call - this.mockProcessDumpUtility.Setup(x => x.TerminateProcessDump()); + this.mockProcessDumpUtility.Setup(x => x.TerminateProcess()); // Raise this.mockDataColectionEvents.Raise(x => x.SessionEnd += null, new SessionEndEventArgs(this.dataCollectionContext)); // Verify GetDumpFiles Call - this.mockProcessDumpUtility.Verify(x => x.TerminateProcessDump(), Times.Once); + this.mockProcessDumpUtility.Verify(x => x.TerminateProcess(), Times.Once); } /// diff --git a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs index 8377b8f375..cc7a37cdcd 100644 --- a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs +++ b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs @@ -308,7 +308,7 @@ public void TerminateProcessDumpShouldCallTerminateOnProcDumpProcess() this.mockProcessHelper.Setup(x => x.TerminateProcess(It.IsAny())); // Raise - processDumpUtility.TerminateProcessDump(); + processDumpUtility.TerminateProcess(); // Verify this.mockProcessHelper.Verify(x => x.TerminateProcess(It.IsAny()), Times.Once);