Skip to content

Commit

Permalink
Blame upload on crash even if hang dump started (#2553)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@nohwnd
nohwnd authored Sep 2, 2020
1 parent deb218e commit 95cb80f
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 64 deletions.
60 changes: 24 additions & 36 deletions src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<KeyValuePair<string, string>> environmentVariables = new List<KeyValuePair<string, string>>();
private bool uploadDumpFiles;
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}.");
Expand Down
27 changes: 0 additions & 27 deletions test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,33 +403,6 @@ public void TriggerSessionEndedHandlerShouldEnsureProcDumpProcessIsTerminated()
this.mockProcessDumpUtility.Verify(x => x.DetachFromTargetProcess(It.IsAny<int>()), Times.Once);
}

/// <summary>
/// The trigger session ended handler should not dump files if proc dump was enabled and test host did not crash
/// </summary>
[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<List<Guid>>(), It.IsAny<Dictionary<Guid, BlameTestObject>>(), It.IsAny<string>()))
.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);
}

/// <summary>
/// The trigger session ended handler should get dump files if collect dump on exit was enabled irrespective of completed test case count
/// </summary>
Expand Down

0 comments on commit 95cb80f

Please sign in to comment.