From 95cb80f7a278fc47a65fc3f51e629b5581754c51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Wed, 2 Sep 2020 11:56:25 +0200 Subject: [PATCH] Blame upload on crash even if hang dump started (#2553) * Upload on session end when HangDump started In rare cases where we have a crashing tests and short hang timeout, we get into situation where hang dumper started dumping, but then session ends because of the crash, and dumps are not uploaded because the session end is only uploading when there is crash dump enabled. After this change we look for any dump in any case and upload all the dumps that hang dumper was able to create. * Fix procdump condition * Remove test, because we want to upload all dumps on session end when hangdump and crash dump run into each other, otherwise hang dumps would not get uploaded and the process would just end, leaving them on the local disk. --- .../BlameCollector.cs | 60 ++++++++----------- .../CrashDumperFactory.cs | 2 +- .../BlameCollectorTests.cs | 27 --------- 3 files changed, 25 insertions(+), 64 deletions(-) diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs index 834b410510..3f8ad9bcd3 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs @@ -46,7 +46,6 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier private IInactivityTimer inactivityTimer; private TimeSpan inactivityTimespan = TimeSpan.FromMinutes(DefaultInactivityTimeInMinutes); private int testHostProcessId; - private bool dumpWasCollectedByHangDumper; private string targetFramework; private List> environmentVariables = new List>(); private bool uploadDumpFiles; @@ -223,7 +222,7 @@ private void CollectDumpAndAbortTesthost() } catch (Exception ex) { - ConsoleOutput.Instance.Error(true, $"Blame: Creating hang dump failed with error {ex}."); + this.logger.LogError(this.context.SessionDataCollectionContext, $"Blame: Creating hang dump failed with error.", ex); } if (this.collectProcessDumpOnTrigger) @@ -244,7 +243,6 @@ private void CollectDumpAndAbortTesthost() { if (!string.IsNullOrEmpty(dumpFile)) { - this.dumpWasCollectedByHangDumper = true; var fileTransferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true, this.fileHelper); this.dataCollectionSink.SendFileAsync(fileTransferInformation); } @@ -263,7 +261,7 @@ private void CollectDumpAndAbortTesthost() } catch (Exception ex) { - ConsoleOutput.Instance.Error(true, $"Blame: Collecting hang dump failed with error {ex}."); + this.logger.LogError(this.context.SessionDataCollectionContext, $"Blame: Collecting hang dump failed with error.", ex); } } else @@ -452,47 +450,37 @@ private void SessionEndedHandler(object sender, SessionEndEventArgs args) this.logger.LogWarning(this.context.SessionDataCollectionContext, Resources.Resources.NotGeneratingSequenceFile); } - if (this.collectProcessDumpOnTrigger) + if (this.uploadDumpFiles) { - // If there was a test case crash or if we need to collect dump on process exit. - // - // Do not try to collect dump when we already collected one from the hang dump - // we won't dump the killed process again and that would just show a warning on the command line - if ((this.testStartCount > this.testEndCount || this.collectDumpAlways) && !this.dumpWasCollectedByHangDumper) + try { - if (this.uploadDumpFiles) + var dumpFiles = this.processDumpUtility.GetDumpFiles(); + foreach (var dumpFile in dumpFiles) { - try + if (!string.IsNullOrEmpty(dumpFile)) { - var dumpFiles = this.processDumpUtility.GetDumpFiles(); - foreach (var dumpFile in dumpFiles) + try { - if (!string.IsNullOrEmpty(dumpFile)) - { - try - { - var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true); - this.dataCollectionSink.SendFileAsync(fileTranferInformation); - } - catch (FileNotFoundException ex) - { - EqtTrace.Warning(ex.ToString()); - this.logger.LogWarning(args.Context, ex.ToString()); - } - } + var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true); + this.dataCollectionSink.SendFileAsync(fileTranferInformation); + } + catch (FileNotFoundException ex) + { + EqtTrace.Warning(ex.ToString()); + this.logger.LogWarning(args.Context, ex.ToString()); } } - catch (FileNotFoundException ex) - { - EqtTrace.Warning(ex.ToString()); - this.logger.LogWarning(args.Context, ex.ToString()); - } - } - else - { - EqtTrace.Info("BlameCollector.CollectDumpAndAbortTesthost: Custom path to dump directory was provided via VSTEST_DUMP_PATH. Skipping attachment upload, the caller is responsible for collecting and uploading the dumps themselves."); } } + catch (FileNotFoundException ex) + { + EqtTrace.Warning(ex.ToString()); + this.logger.LogWarning(args.Context, ex.ToString()); + } + } + else + { + EqtTrace.Info("BlameCollector.CollectDumpAndAbortTesthost: Custom path to dump directory was provided via VSTEST_DUMP_PATH. Skipping attachment upload, the caller is responsible for collecting and uploading the dumps themselves."); } } finally diff --git a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs index 2adb34df10..469885579b 100644 --- a/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs +++ b/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs @@ -44,7 +44,7 @@ public ICrashDumper Create(string targetFramework) // This proven to be working okay while net5.0 could not create dumps from Task.Run, and I was using this same technique // to get dump of testhost. This needs PROCDUMP_PATH set to directory with procdump.exe, or having it in path. var procdumpOverride = Environment.GetEnvironmentVariable("VSTEST_DUMP_FORCEPROCDUMP")?.Trim(); - var forceUsingProcdump = string.IsNullOrWhiteSpace(procdumpOverride) && procdumpOverride != "0"; + var forceUsingProcdump = !string.IsNullOrWhiteSpace(procdumpOverride) && procdumpOverride != "0"; if (forceUsingProcdump) { EqtTrace.Info($"CrashDumperFactory: This is Windows on {targetFramework}. Forcing the use of ProcDumpCrashDumper that uses ProcDump utility, via VSTEST_DUMP_FORCEPROCDUMP={procdumpOverride}."); diff --git a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs index fffd563384..880e742290 100644 --- a/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs +++ b/test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs @@ -403,33 +403,6 @@ public void TriggerSessionEndedHandlerShouldEnsureProcDumpProcessIsTerminated() this.mockProcessDumpUtility.Verify(x => x.DetachFromTargetProcess(It.IsAny()), Times.Once); } - /// - /// The trigger session ended handler should not dump files if proc dump was enabled and test host did not crash - /// - [TestMethod] - public void TriggerSessionEndedHandlerShouldNotGetDumpFileIfNoCrash() - { - // Initializing Blame Data Collector - this.blameDataCollector.Initialize( - this.GetDumpConfigurationElement(), - this.mockDataColectionEvents.Object, - this.mockDataCollectionSink.Object, - this.mockLogger.Object, - this.context); - - // Setup - this.mockProcessDumpUtility.Setup(x => x.GetDumpFiles()).Returns(new[] { this.filepath }); - this.mockBlameReaderWriter.Setup(x => x.WriteTestSequence(It.IsAny>(), It.IsAny>(), It.IsAny())) - .Returns(this.filepath); - - // Raise - this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); - this.mockDataColectionEvents.Raise(x => x.SessionEnd += null, new SessionEndEventArgs(this.dataCollectionContext)); - - // Verify GetDumpFiles Call - this.mockProcessDumpUtility.Verify(x => x.GetDumpFiles(), Times.Never); - } - /// /// The trigger session ended handler should get dump files if collect dump on exit was enabled irrespective of completed test case count ///