-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancing blame data collector options #1682
Changes from 4 commits
3279dd3
6120211
98dadc3
283cabe
08d56e1
e0254ac
6e33e89
a2df30d
8570979
15d72a1
5612d6a
407442f
81d6b30
56c701f
186df0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier | |
private int testStartCount; | ||
private int testEndCount; | ||
private bool processDumpEnabled; | ||
private bool alwaysCollectProcessDump; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If alwaysCollectProcessDump is false but collect dump is present, then we are creating dump on process crash. As we are creating dump, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a RFC for this as this is experience change? |
||
private bool processFullDumpEnabled; | ||
private string attachmentGuid; | ||
|
||
/// <summary> | ||
|
@@ -97,7 +99,13 @@ public override void Initialize( | |
|
||
if (this.configurationElement != null) | ||
{ | ||
this.processDumpEnabled = this.configurationElement[Constants.DumpModeKey] != null; | ||
var collectDumpNode = this.configurationElement[Constants.DumpModeKey]; | ||
this.processDumpEnabled = collectDumpNode != null; | ||
if (this.processDumpEnabled) | ||
{ | ||
this.alwaysCollectProcessDump = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpAlwaysKey]?.Value, "true", StringComparison.OrdinalIgnoreCase); | ||
this.processFullDumpEnabled = string.Equals(collectDumpNode.Attributes[Constants.DumpTypeKey]?.Value, "full", StringComparison.OrdinalIgnoreCase); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can move There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
} | ||
|
||
this.attachmentGuid = Guid.NewGuid().ToString().Replace("-", string.Empty); | ||
|
@@ -159,23 +167,26 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args) | |
|
||
if (this.processDumpEnabled) | ||
{ | ||
try | ||
if (this.testStartCount > this.testEndCount || this.alwaysCollectProcessDump) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if process throws before test cases start? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have an option to collect dump no matter the number of test cases. This check is just the default behaviour. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add comments here? Example |
||
{ | ||
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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be logger warning as well as user was expecting a dump file but was not created. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
} | ||
else | ||
catch (FileNotFoundException ex) | ||
{ | ||
EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated."); | ||
this.logger.LogWarning(args.Context, ex.Message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets put this in trace as well. |
||
} | ||
} | ||
catch (FileNotFoundException ex) | ||
{ | ||
this.logger.LogWarning(args.Context, ex.Message); | ||
} | ||
} | ||
|
||
this.DeregisterEvents(); | ||
|
@@ -195,7 +206,7 @@ private void TestHostLaunched_Handler(object sender, TestHostLaunchedEventArgs a | |
|
||
try | ||
{ | ||
this.processDumpUtility.StartProcessDump(args.TestHostProcessId, this.attachmentGuid, this.GetResultsDirectory()); | ||
this.processDumpUtility.StartProcessDump(args.TestHostProcessId, this.attachmentGuid, this.GetResultsDirectory(), this.processFullDumpEnabled); | ||
} | ||
catch (TestPlatformException e) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,5 +42,15 @@ internal static class Constants | |
/// Configuration key name for dump mode | ||
/// </summary> | ||
public const string DumpModeKey = "CollectDump"; | ||
|
||
/// <summary> | ||
/// Configuration key name for coolect dump always | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spell |
||
/// </summary> | ||
public const string CollectDumpAlwaysKey = "AlwaysCollectDump"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment. CollectDumpOnProcessAbort/CollectDumpOnProcessCrash/CollectDumpOnProcessExit is more accurate. |
||
|
||
/// <summary> | ||
/// Configuration key name for dump type | ||
/// </summary> | ||
public const string DumpTypeKey = "DumpType"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,14 +71,14 @@ public string GetDumpFile() | |
} | ||
|
||
/// <inheritdoc/> | ||
public void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory) | ||
public void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory, bool isFullDump = false) | ||
{ | ||
this.dumpFileName = $"{this.processHelper.GetProcessName(processId)}_{processId}_{dumpFileGuid}"; | ||
this.testResultsDirectory = testResultsDirectory; | ||
|
||
this.procDumpProcess = this.processHelper.LaunchProcess( | ||
this.GetProcDumpExecutable(), | ||
ProcessDumpUtility.BuildProcDumpArgs(processId, this.dumpFileName), | ||
ProcessDumpUtility.BuildProcDumpArgs(processId, this.dumpFileName, isFullDump), | ||
testResultsDirectory, | ||
null, | ||
null, | ||
|
@@ -94,13 +94,24 @@ public void StartProcessDump(int processId, string dumpFileGuid, string testResu | |
/// <param name="filename"> | ||
/// Filename for dump file | ||
/// </param> | ||
/// <param name="isFullDump"> | ||
/// Is full dump enabled | ||
/// </param> | ||
/// <returns>Arguments</returns> | ||
private static string BuildProcDumpArgs(int processId, string filename) | ||
private static string BuildProcDumpArgs(int processId, string filename, bool isFullDump = false) | ||
{ | ||
// -accepteula: Auto accept end-user license agreement | ||
// -t: Write a dump when the process terminates. | ||
// This will create a minidump of the process with specified filename | ||
return "-accepteula -t " + processId + " " + filename + ".dmp"; | ||
if (isFullDump) | ||
{ | ||
// This will create a fulldump of the process with specified filename | ||
return "-accepteula -t -ma " + processId + " " + filename + ".dmp"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this args needs to change. Please check with @abhishkk . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I'm doing this in next task. Current PR is just for the additional collector attributes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When discussed args are added, we need to test it for all known crashes, for 32 bit and 64 bit process with 32 and 64 bit OS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than creating separate if else, we can create a stringbuilder and not add -ma in case its mini dump. This allows us to enahance this argument creation based on any new functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deferred to next refactor in line |
||
} | ||
else | ||
{ | ||
// This will create a minidump of the process with specified filename | ||
return "-accepteula -t " + processId + " " + filename + ".dmp"; | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,11 @@ public static class Constants | |
/// </summary> | ||
public const string BlameCollectDumpKey = "CollectDump"; | ||
|
||
/// <summary> | ||
/// Name of collect dump option for blame. | ||
/// </summary> | ||
public const string BlameCollectDumpOnlyOnAbortKey = "CollectDumpOnlyOnAbort"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is it being used ? |
||
|
||
/// <summary> | ||
/// Name of data collection settings node in RunSettings. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,21 @@ | |
namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors | ||
{ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Xml; | ||
|
||
using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources; | ||
using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities; | ||
using Microsoft.VisualStudio.TestPlatform.Common; | ||
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces; | ||
using Microsoft.VisualStudio.TestPlatform.Common.Utilities; | ||
using Microsoft.VisualStudio.TestPlatform.ObjectModel; | ||
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities; | ||
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces; | ||
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions; | ||
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces; | ||
using Microsoft.VisualStudio.TestPlatform.Utilities; | ||
|
||
using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources; | ||
|
||
internal class EnableBlameArgumentProcessor : IArgumentProcessor | ||
{ | ||
/// <summary> | ||
|
@@ -30,7 +33,7 @@ internal class EnableBlameArgumentProcessor : IArgumentProcessor | |
/// <summary> | ||
/// Initializes a new instance of the <see cref="EnableBlameArgumentProcessor"/> class. | ||
/// </summary> | ||
public EnableBlameArgumentProcessor() | ||
public EnableBlameArgumentProcessor() | ||
{ | ||
} | ||
|
||
|
@@ -133,7 +136,8 @@ public void Initialize(string argument) | |
{ | ||
bool isDumpEnabled = false; | ||
|
||
if (!string.IsNullOrWhiteSpace(argument) && argument.Equals(Constants.BlameCollectDumpKey, StringComparison.OrdinalIgnoreCase)) | ||
var parseSucceeded = LoggerUtilities.TryParseLoggerArgument(argument, out string loggerIdentifier, out Dictionary<string, string> parameters); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is blame argumetn. TryParseLoggerArgument should not be used to parse this. Not only its wrong from code per-se, its wrong from experience per-se because in case of any invalid arg, logger will throw its own error rather than of blame specific. As underlying code of logger is generic, i have made this code common in this PR: #1681 You can use these changes once PR is checked in. I am trying to check it in by today or tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, please let me know once you push in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it seems like now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to have validation here in case any invalid args, parameters passed. Check comment in UTs for detail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update Blame usage text with these new key value pair info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
if (parseSucceeded && loggerIdentifier.Equals(Constants.BlameCollectDumpKey, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
if (this.environment.OperatingSystem == PlatformOperatingSystem.Windows && | ||
this.environment.Architecture != PlatformArchitecture.ARM64 && | ||
|
@@ -195,12 +199,21 @@ public void Initialize(string argument) | |
if (isDumpEnabled) | ||
{ | ||
var dumpNode = XmlDocument.CreateElement(Constants.BlameCollectDumpKey); | ||
if (parameters != null && parameters.Count > 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We can move this logic to new method like AddBlameToRunSettings |
||
{ | ||
foreach (KeyValuePair<string, string> entry in parameters) | ||
{ | ||
var attribute = XmlDocument.CreateAttribute(entry.Key); | ||
attribute.Value = entry.Value; | ||
dumpNode.Attributes.Append(attribute); | ||
} | ||
} | ||
outernode.AppendChild(dumpNode); | ||
} | ||
|
||
foreach (var item in dataCollectionRunSettings.DataCollectorSettingsList) | ||
{ | ||
if( item.FriendlyName.Equals(BlameFriendlyName)) | ||
if (item.FriendlyName.Equals(BlameFriendlyName)) | ||
{ | ||
item.Configuration = outernode; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,14 +63,14 @@ public BlameCollectorTests() | |
public void InitializeShouldThrowExceptionIfDataCollectionLoggerIsNull() | ||
{ | ||
Assert.ThrowsException<ArgumentNullException>(() => | ||
{ | ||
this.blameDataCollector.Initialize( | ||
this.configurationElement, | ||
this.mockDataColectionEvents.Object, | ||
this.mockDataCollectionSink.Object, | ||
null, | ||
null); | ||
}); | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All kind of garbage value checks unit tests will come here. |
||
this.blameDataCollector.Initialize( | ||
this.configurationElement, | ||
this.mockDataColectionEvents.Object, | ||
this.mockDataCollectionSink.Object, | ||
null, | ||
null); | ||
}); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -142,6 +142,63 @@ public void TriggerSessionEndedHandlerShouldGetDumpFileIfProcDumpEnabled() | |
|
||
// Setup | ||
this.mockProcessDumpUtility.Setup(x => x.GetDumpFile()).Returns(this.filepath); | ||
this.mockBlameReaderWriter.Setup(x => x.WriteTestSequence(It.IsAny<List<TestCase>>(), It.IsAny<string>())) | ||
.Returns(this.filepath); | ||
|
||
// Raise | ||
this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); | ||
this.mockDataColectionEvents.Raise(x => x.TestCaseStart += null, new TestCaseStartEventArgs(new TestCase())); | ||
this.mockDataColectionEvents.Raise(x => x.SessionEnd += null, new SessionEndEventArgs(this.dataCollectionContext)); | ||
|
||
// Verify GetDumpFiles Call | ||
this.mockProcessDumpUtility.Verify(x => x.GetDumpFile(), 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.GetDumpFile()).Returns(this.filepath); | ||
this.mockBlameReaderWriter.Setup(x => x.WriteTestSequence(It.IsAny<List<TestCase>>(), 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.GetDumpFile(), Times.Never); | ||
} | ||
|
||
/// <summary> | ||
/// The trigger session ended handler should get dump files if collect dump always was enabled irrespective of completed test case count | ||
/// </summary> | ||
[TestMethod] | ||
public void TriggerSessionEndedHandlerShouldGetDumpFileIfCollectDumpAlwaysIsEnabled() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add UTs for scenarios like:
|
||
{ | ||
// Initializing Blame Data Collector | ||
this.blameDataCollector.Initialize( | ||
this.GetDumpConfigurationElement(alwaysCollectDump: true), | ||
this.mockDataColectionEvents.Object, | ||
this.mockDataCollectionSink.Object, | ||
this.mockLogger.Object, | ||
this.context); | ||
|
||
// Setup | ||
this.mockProcessDumpUtility.Setup(x => x.GetDumpFile()).Returns(this.filepath); | ||
this.mockBlameReaderWriter.Setup(x => x.WriteTestSequence(It.IsAny<List<TestCase>>(), It.IsAny<string>())) | ||
.Returns(this.filepath); | ||
|
||
// Raise | ||
this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); | ||
|
@@ -166,8 +223,11 @@ public void TriggerSessionEndedHandlerShouldLogWarningIfGetDumpFileThrowsFileNot | |
this.context); | ||
|
||
// Setup and raise events | ||
this.mockBlameReaderWriter.Setup(x => x.WriteTestSequence(It.IsAny<List<TestCase>>(), It.IsAny<string>())) | ||
.Returns(this.filepath); | ||
this.mockProcessDumpUtility.Setup(x => x.GetDumpFile()).Throws(new FileNotFoundException()); | ||
this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); | ||
this.mockDataColectionEvents.Raise(x => x.TestCaseStart += null, new TestCaseStartEventArgs(new TestCase())); | ||
this.mockDataColectionEvents.Raise(x => x.SessionEnd += null, new SessionEndEventArgs(this.dataCollectionContext)); | ||
|
||
// Verify GetDumpFiles Call | ||
|
@@ -192,7 +252,28 @@ public void TriggerTestHostLaunchedHandlerShouldStartProcDumpUtilityIfProcDumpEn | |
this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); | ||
|
||
// Verify StartProcessDumpCall | ||
this.mockProcessDumpUtility.Verify(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>())); | ||
this.mockProcessDumpUtility.Verify(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>(), false)); | ||
} | ||
|
||
/// <summary> | ||
/// The trigger test host launched handler should start process dump utility for full dump if full dump was enabled | ||
/// </summary> | ||
[TestMethod] | ||
public void TriggerTestHostLaunchedHandlerShouldStartProcDumpUtilityForFullDumpIfFullDumpEnabled() | ||
{ | ||
// Initializing Blame Data Collector | ||
this.blameDataCollector.Initialize( | ||
this.GetDumpConfigurationElement(isFullDump: true), | ||
this.mockDataColectionEvents.Object, | ||
this.mockDataCollectionSink.Object, | ||
this.mockLogger.Object, | ||
this.context); | ||
|
||
// Raise TestHostLaunched | ||
this.mockDataColectionEvents.Raise(x => x.TestHostLaunched += null, new TestHostLaunchedEventArgs(this.dataCollectionContext, 1234)); | ||
|
||
// Verify StartProcessDumpCall | ||
this.mockProcessDumpUtility.Verify(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>(), true)); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -211,7 +292,7 @@ public void TriggerTestHostLaunchedHandlerShouldCatchTestPlatFormExceptionsAndRe | |
|
||
// Make StartProcessDump throw exception | ||
var tpex = new TestPlatformException("env var exception"); | ||
this.mockProcessDumpUtility.Setup(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>())) | ||
this.mockProcessDumpUtility.Setup(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>(), false)) | ||
.Throws(tpex); | ||
|
||
// Raise TestHostLaunched | ||
|
@@ -237,7 +318,7 @@ public void TriggerTestHostLaunchedHandlerShouldCatchAllUnexpectedExceptionsAndR | |
|
||
// Make StartProcessDump throw exception | ||
var ex = new Exception("start process failed"); | ||
this.mockProcessDumpUtility.Setup(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>())) | ||
this.mockProcessDumpUtility.Setup(x => x.StartProcessDump(1234, It.IsAny<string>(), It.IsAny<string>(), false)) | ||
.Throws(ex); | ||
|
||
// Raise TestHostLaunched | ||
|
@@ -253,13 +334,26 @@ public void CleanUp() | |
File.Delete(this.filepath); | ||
} | ||
|
||
private XmlElement GetDumpConfigurationElement() | ||
private XmlElement GetDumpConfigurationElement(bool isFullDump = false, bool alwaysCollectDump = false) | ||
{ | ||
var xmldoc = new XmlDocument(); | ||
var outernode = xmldoc.CreateElement("Configuration"); | ||
var node = xmldoc.CreateElement(BlameDataCollector.Constants.DumpModeKey); | ||
outernode.AppendChild(node); | ||
node.InnerText = "Text"; | ||
if (isFullDump) | ||
{ | ||
var fulldumpAttribute = xmldoc.CreateAttribute(BlameDataCollector.Constants.DumpTypeKey); | ||
fulldumpAttribute.Value = "full"; | ||
node.Attributes.Append(fulldumpAttribute); | ||
} | ||
|
||
if (alwaysCollectDump) | ||
{ | ||
var fulldumpAttribute = xmldoc.CreateAttribute(BlameDataCollector.Constants.CollectDumpAlwaysKey); | ||
fulldumpAttribute.Value = "true"; | ||
node.Attributes.Append(fulldumpAttribute); | ||
} | ||
|
||
return outernode; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can rename this to processMiniDumpEnabled for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be true irrespective of minidump or full dump is enabled. Code logic wise this is more readable. We can discuss if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this makes more sense. But you are also using processFullDumpEnabled bariable to know whether full dump is enabled or not. Rather than processFullDumpEnabled it shoule take string or enum type 'full' or 'mini'. Today we are implictly deciding that if processFullDumpEnabled is false but processEnabled is true, then that means mini dump is enabled. This should be explicit.